fix splitting sharp edges with active cell scalars

[akaszynski]

Splitting sharp edges fails for polydata when the active scalars are cells:

import pyvista as pv

cube = pv.Cube()
cube.cell_data['cell_id'] = range(cube.n_cells)
cube = cube.triangulate().subdivide(2)

cube.plot(smooth_shading=True, split_sharp_edges=True)

File ~/python/pyvista/pyvista/core/datasetattributes.py:769, in DataSetAttributes._prepare_array(self, data, name, deep_copy)
    767     data = tmparray
    768 if data.shape[0] != array_len:
--> 769     raise ValueError(f'data length of ({data.shape[0]}) != required length ({array_len})')
    771 if data.dtype == np.bool_:
    772     self.dataset.association_bitarray_names[self.association.name].add(name)

ValueError: data length of (192) != required length (98)

The fix is to simply not track cell indices since they do not change when splitting sharp edges.

[codecov]

Codecov Report

Merging #2695 (264262d) into main (e731df4) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2695      +/-   ##
==========================================
+ Coverage   93.70%   93.81%   +0.10%     
==========================================
  Files          75       76       +1     
  Lines       16178    16185       +7     
==========================================
+ Hits        15160    15184      +24     
+ Misses       1018     1001      -17     

[akaszynski]

This broke our docs at PyMAPDL documentation and I'd like to have this out for the next release (end of June?). Appreciate a review when anyone gets a chance.

[adeak]

I've added a few small changes, please check. Otherwise this PR looks good to me. That being said, two points:

1. in prepare_smooth_shading we talk about Phong shading, but in compute_normals we talk about Gouraud. Shouldn't these refer to the same method? I know there was a recent-ish switch from one to the other... but perhaps that was only for PBR?
2. right now we use np.int32 for the original point IDs, presumably to spare memory But np.iinfo(np.int32).max says this goes up to 2147483647, i.e. 2 billion points. If I estimated correctly, even with double-precision coordinates such a mesh (sans data) would amount to 48 GB of memory, which is arguably not that far-fetched these days. How about we make use of the fact that point indices are non-negative, and use np.uint32 instead? Or np.uint64 but I can see how double the memory for this auxiliary array can become a problem... Also cc @MatthewFlamm who usually has strong points about memory.

[MatthewFlamm]

I think minimizing memory usage is very important when talking about adding/modifying cell or point data. I speculate that a mesh of 2 billion points is likely to be a volumetric one, e.g. UnstructuredGrid, and thus required to extract a surface first. I would also suspect that a PolyData mesh with 2 billion points would not need this plotting option to create sharp looking edges since the mesh is so fine. Also importantly the additional data in compute_normals is opt-in. So I think this is fine, but if unsigned integers works why not?

[adeak]

Great points, thanks @MatthewFlamm. So the only question is whether anything can break with uint32; I expect pyvista to work seamlessly with that, question is whether VTK can do something funny with more esoteric dtypes.

[akaszynski]

1. in prepare_smooth_shading we talk about Phong shading, but in compute_normals we talk about Gouraud. Shouldn't these refer to the same method? I know there was a recent-ish switch from one to the other... but perhaps that was only for PBR?

Good catch. We're using prop.SetInterpolationToPhong() and switched from Gouraud shading a while back. In fact, there's inconsistent documentation within plotter.py that I removed in 962537a.

So I think this is fine, but if unsigned integers works why not?

That's a reasonable point, and as much as I'd like to improve the memory efficiency of pyvista, I don't want to stay too far from the way that VTK does things.

Speaking of that, I picked np.int32 because of memory efficiency, but as it turns out, VTK uses int32 for Windows and int64 for Linux. This is the int type used for vtk.vtkIdTypeArray and we obtain it from pyvista.utilities.misc._get_vtk_id_type. For the sake of consistency, I'm going to use that type, even if it's unnecessarily large. As @MatthewFlamm, it's an opt-in.

Implemented that change in 8896330.

[adeak]

That's a reasonable point, and as much as I'd like to improve the memory efficiency of pyvista, I don't want to stay too far from the way that VTK does things.

That's fair.

Speaking of that, I picked np.int32 because of memory efficiency, but as it turns out, VTK uses int32 for Windows and int64 for Linux.

For what it's worth this sounds a lot like just "numpy default int". Int the size of the word size (int32 on 32-bit systems and int64 on 64-bit systems) except 64-bit Windows which also has int32. And the reason is dark history involving the size of long.

[akaszynski]

Note: I'm skipping a test for MacOS. Now that we're running image cache verification, I expect to run into these issues more often, especially on MacOS.