Skip to content

Use pyvista's public API and int32 face indices - #80

Merged
akaszynski merged 6 commits into
mainfrom
fix/use-pyvista-public-api
Aug 15, 2026
Merged

Use pyvista's public API and int32 face indices#80
akaszynski merged 6 commits into
mainfrom
fix/use-pyvista-public-api

Conversation

@akaszynski

Copy link
Copy Markdown
Member

Summary

  • Build meshes with PolyData.from_regular_faces instead of hand-assembling a vtkCellArray, which removes the direct vtkmodules import.
  • Use int32 face indices end to end, including the C++.
  • Raise pyvista>=0.42.0, needed for from_regular_faces and regular_faces.

The binding bug

clustering.py imported vtkCellArray from vtkmodules and handed it to a pyvista PolyData. That only holds while pyvista is built on the stock wheel — on another binding it is a different C++ type and SetPolys rejects it with a bare TypeError: SetPolys argument 1:. Rather than route the import through pyvista, from_regular_faces removes the need for it, so there is nothing left to mismatch. A ruff TID251 rule now bans vtk and vtkmodules across the package so it cannot come back.

It also picks fixed-size cell storage when the VTK build supports it, so on pyvista 0.49 the offsets array disappears. On 0.42 to 0.48 the behaviour is unchanged.

int32 faces

clustering.cpp hard-coded int64_t for faces while every other index array crossing the boundary — neighbours, edges, clusters — was already 32-bit. Since nanobind converts a mismatched dtype with a copy rather than erroring, two call sites were already paying that copy silently. Faces are now int32 throughout and subdivision returns int32.

Measured on a sphere with 3 subdivisions and 20000 clusters, the intermediate arrays halve: the faces array 10.7 MB to 5.3 MB, the subdivision output 42.8 MB to 21.4 MB. The returned mesh is unchanged, because create_mesh(clean=True) runs VTK's clean filter, which re-widens it. That is #79.

int32 indices cap input at np.iinfo(np.int32).max points, so _tri_faces_from_poly raises rather than letting indices wrap, and SubdivideTriangles throws std::overflow_error if it would emit more points than an int32 can address. Two pre-existing 32-bit truncations in the same functions are fixed alongside: an int * int allocation size in PointNormals and a uint32_t subscript in RayTrace.

Remeshing output is bit-identical to main — sphere and cylinder, points and faces, maxdiff 0.0. The only difference anywhere is subdivision's faces dtype.

Behaviour change

_tri_faces_from_poly now rejects non-triangular meshes with the same message Clustering.__init__ already uses. Previously a quad mesh whose connectivity happened to divide by three returned silent garbage rather than raising, so this can surface an error for anyone feeding quads without noticing.

AI Usage

Drafted with Claude Opus 5, reviewed by me before pushing. A second Claude reviewed the branch cold and found that the overflow guard I added could never fire — nface_new was an int accumulating to 4 * nface, so it wrapped before the guard read it, and being UB the compiler could drop the check entirely. It also caught that the new triangle check regressed empty meshes to an IndexError. Both fixed here.

akaszynski and others added 4 commits August 15, 2026 08:47
`polydata_from_faces` imported `vtkCellArray` from `vtkmodules` and hand-built
an offsets array. pyvista is not always built on the stock `vtkmodules` wheel,
and a cell array from a different binding is a different C++ type that
`SetPolys` rejects with a bare `TypeError`.

Use `PolyData.from_regular_faces` instead. As a side effect pyacvd now gets
VTK's fixed size cell storage (no offsets array) and keeps an int32 faces
array int32 rather than widening it, on pyvista releases that support it.

Also replace the private `mesh._connectivity_array.reshape(-1, 3)` with the
public `mesh.regular_faces` (pyvista/pyvista#8913 deprecates the former).

Both APIs require pyvista >= 0.42.0, so bump the floor.
`_connectivity_array.reshape(-1, 3)` used to fail on a quad mesh only by
accident, because the reshape did not divide evenly. `regular_faces` returns
an (n, 4) array instead, which the compiled functions would read as triangles
and silently produce wrong results. Check the face size instead.
Faces were the only index arrays crossing the nanobind boundary as int64.
Neighbors, neighbor offsets, edges and clusters were already int32, and two
call sites (`face_normals` via `f_clus`, and the first `subdivision` pass)
already passed int32, which nanobind silently converted with a full copy
because `NDArray` fixes the dtype.

Convert the face arrays in the extension to int32, so `subdivision` also
returns int32 faces, and narrow the faces once in `_tri_faces_from_poly`
instead. Point indices scaled by three no longer fit an int32, so the derived
index variables are widened to `size_t` rather than left to overflow, and
`SubdivideTriangles` rejects a subdivision whose point count would not fit an
int32 index.

int32 indices cap a mesh at `np.iinfo(np.int32).max` points, so
`_tri_faces_from_poly` raises rather than letting the indices wrap.
`_subdivide` now uses it too, which also drops a reshape of the padded legacy
faces array.

A sphere, a cylinder and the bunny remesh to bit identical points and faces
before and after, on pyvista 0.48.4 and 0.49.dev0.
The guard added in "Use int32 face indices end to end" could not fire.
``nface`` was an ``int`` and ``nface_new`` accumulated ``+= 3`` up to
``4 * nface``, which is signed overflow once ``nface > INT_MAX / 4``, so
``max_points`` was computed from an already wrapped value and the
``std::overflow_error`` never threw. ``nvert``, ``nface``, ``nface_new``
and the loop counter are now ``int64_t``, which also drops the
``int nface = shape(0)`` truncation the narrowing had made reachable.

``_tri_faces_from_poly`` raised ``IndexError`` on an empty ``PolyData``,
a regression against main, because ``regular_faces`` has shape ``(0,)``
there. It also duplicated the guard ``Clustering.__init__`` already has
with a weaker message, and missed mixed tri/quad meshes entirely, since
``regular_faces`` raises pyvista's own error before the shape is read.
It now uses the ``is_all_triangles`` check the repo already uses, and
returns an empty ``(0, 3)`` array for a mesh with no cells.

Face index locals are ``int64_t`` throughout rather than a mix of
``size_t`` and ``int32_t``. The ``.view()`` paths were relying on
nanobind casting indices to ``int64_t`` before applying strides, which
holds in 2.14.0 but is not guaranteed by the ``nanobind >= 1.3.2`` build
floor. This also restores main's local types, so the C++ diff is now
just the change of input dtype.

Two pre-existing 32-bit truncations in the functions touched above:

* ``PointNormals`` sized its scratch array with ``AllocateArray<T>(
  n_faces * 3)``, an ``int * int`` product that overflows above roughly
  715M faces.
* ``RayTrace`` indexed with ``f[ind * 3 + 0]``, where ``ind`` is a
  ``uint32_t``, so the product wraps above roughly 1.43B faces.

Both now scale through an explicit ``static_cast<size_t>``.

The AST import test only inspected module level imports in
``clustering.py``, so it passed if the import moved into a function body
or into ``_accessor.py``. Replaced with ruff's ``TID251`` banned-api
rule, which covers the whole package and runs in pre-commit where this
repo already enforces its rules.

The overflow guard test monkeypatched ``PolyData.n_points``, which is
defined on ``DataSet``, so undoing the patch left a copy installed on
``PolyData`` for the rest of the session. It patches ``MAX_POINTS``
instead, which also proves the comparison uses it.

The version gated tests keyed off ``_SUPPORTS_FIXED_SIZE_STORAGE``, a
private pyvista symbol that can be renamed without deprecation, after
which both would silently stop running. They now probe the behaviour.

Also drops a comment claiming ``PolyData.from_regular_faces`` preserves
an int32 faces array; on pyvista 0.48.4 the result is int64.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pv46gGGU77uTBDVHSdJRtY
Comment thread src/clustering.cpp
Comment thread src/pyacvd/clustering.py Outdated
Comment thread tests/test_clustering.py
@akaszynski
akaszynski marked this pull request as ready for review August 15, 2026 16:04
akaszynski and others added 2 commits August 15, 2026 10:07
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pv46gGGU77uTBDVHSdJRtY
pyvista's from_regular_faces is shallow by default and VTK holds a
reference to the buffers, so the copy only cost memory and time.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pv46gGGU77uTBDVHSdJRtY
@akaszynski
akaszynski merged commit dba5f3e into main Aug 15, 2026
13 checks passed
@akaszynski
akaszynski deleted the fix/use-pyvista-public-api branch August 15, 2026 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant