I would like to add a vtkDataSet::GetPoints function that computes the points array and return it.
I would like to do that to avoid using GetPoint() since it performs a computation at each call for vtkImagaData and vtkRectilinearGrid, while GetPoints would allow me to compute the points once and then query them more than once.
There are 2 options for doing this
vtkSmartPointer vtkDataSet::GetPoints()
void vtkDataSet::GetPoints(vtkPoints* points) like void vtkRectilinearGrid::GetPoints(vtkPoints* points)
The first solution would have a conflict with vtkPoints* vtkPointSet::GetPoints() but it would have 1 function for both.
The second solution would have no conflict with vtkPoints* vtkPointSet::GetPoints() but it would have 2 functions for the same thing for vtkPointSet subclasses.
Where would you use the new GetPoints() function? Memory-wise, implicit geometry a major feature of vtkImageData and vtkRectilinearGrid, and any filter that generates vtkPoints for these dataset types is going to consume a lot of memory. The generated vtkPoints array will consume anywhere from 3 times up to more than 10 times as much memory as the vtkDataArray that holds the scalars.
Yes, it does make more sense if you have a vtkImplicitArray that computes the coordinates on demand. For some of my own algorithms, I wrote vtkImagePointIterator to make it easier to write image-processing code that uses points. An implicit array could do pretty much the same thing, but with random-access.
Why not hosting a vtkPoints inside each subclass or vtkDataSet and update it’s implicit definition upon chaning extents / origin, etc? GetPoints would become virtual.
Problem now: you’re breaking backward compatibility for people who have custom vtkDataSet, so I think we should probably host a vtkPoints in vtkDataSet as well that would call GetPoint under the cover, and overwrite its behavior in subclasses.
What I’m afraid of is people that would assume that vtkDataSet::GetPoints returning nullptr means that there are no points, which would not be the case for custom subclasses. Similarly it would mean that we would still need to write a specific path using the GetPoint API in filters accepting vtkDataSet as input so they still work in all cases.
On the contrary, if GetPoints returns a legit implicit array returning the points that GetPoint would return, then I feel like we are getting ourselves away from the caveats I just described.
I agree. If you change the behavior of a pre-existing method name like GetPoints(), there are certain expectations. It shouldn’t return nullptr unless there are no points. People will not like it if vtkExtractGeometry and other filters suddenly stop working for their data.
It’s better to have a pure virtual method than a default implementation that returns nullptr. With a pure virtual method, people see the problems at compile-time and they know what they have to fix.
A default implementation that uses vtkDataSet::GetPoint() sounds like a reasonable solution.
GetPoints could also potentially throw a warning advising overriding this method. I don’t think we would spam the terminal because it doesn’t get called that often, and developers would be aware of this API enhancement and could optimize their class if they feel like it. Explicitly talking about implicit arrays could point them toward the right path easily.
Array ranges with implicit arrays have a performance hit of ~25% if I remember correctly because they use the generic implementation. If we want performance, in addition of enhancing those core component APIs, it might be worth specializing array ranges for implicit arrays so we don’t have this performance hit anymore.
I have a couple of updates in my MR but they can be summarized as below:
When vtkImageData/vtkRectilinearGrid/vtkStructuredGrid call GetPoint, GetPointCells, GetCellType queries, they perform a calculation on demand. Parts of this calculation are repeated for each query. To improve the performance of these we can precompute those parts and utilize them inside implicit Points, Cells and CellTypes structures.
Using the vtkImplicitArray capability, we have added implicit Points, Cells, and CellTypes structures. vtkStructuredData now has 3 new methods to create these implicit structures:
vtkSmartPointer<vtkPoints> GetPoints(vtkDataArray* x, vtkDataArray* y, vtkDataArray* z, int dim[3])
vtkPoints contains an vtkImplicitArray as its data which utilizes, x, y, and z arrays and the dims to compute the implicit Points structure.
vtkImplicitCellArray is a new class to defines an implicit Cells structure for structured data. It utilizes the vtkImplicitArray capability. vtkAbstractCellArray (also a new class) is the parent class of both vtkCellArray and vtkImplicitCellArray.
vtkConstantArray<int> is a vtkImplicitArray that is used to define an implicit CellTypes structure for structured data.
The above structures are used by:
vtkImageData uses the implicit Points, Cells, and CellTypes structures to implement GetPoint, GetPointCells, and GetCellType queries.
vtkRectilinearGrid uses the implicit Points, Cells, and CellTypes structures to implement GetPoint, GetPointCells, and GetCellType queries.
vtkStructuredGrid uses and CellTypes structures to implement GetPointCells, and GetCellType queries.
Thanks to the addition, the highly performant and specialized code for structured data in vtkTableBasedClipDataSet can be removed since these new implicit structures implement the same performant functionality.
Moreover, there is a new vtkPoints* vtkDataSet::GetPoints() function, which for vtkImageData, and vtkRectilinearGrid return their implicit structure. If the vtkDataSet subclass does not define override this function then an internal copy is created and returned which should not be modified.
Lastly, because of this implicit points structure, void vtkRectilinearGrid::GetPoints(vtkPoints*) has been deprecated.