ENH: KernelTransform's PointSet should use minimal point data

[dzenanz]

This data is not used, so it is better to have a lighter-weight char, instead of the heavier PointType there. Having PointType there might also confuse someone into thinking that point coordinates should be provided there, or might be retrieved from it.

PR Checklist

• No API changes were made (or the changes have been approved)
• No major design changes were made (or the changes have been approved)

[N-Dekker]

Ideally you would use PointSet<void, VDimension, PointSetTraitsType>, right?

For what it's worth, I noticed that sizeof(PointSet<unsigned char>) == sizeof(PointSet<long long>), so for the size it does not really seem to matter.

Do you have an idea why KernelTransform doesn't simply use an std::vector of points?

[dzenanz]

PointSet<void, VDimension, PointSetTraitsType> does not compile, unfortunately. That was my first idea 😄

Do you have an idea why KernelTransform doesn't simply use an std::vector of points?

Maybe to make it more convenient to cooperate with meshes?

[dzenanz]

sizeof(PointSet<>) is unaffected, as it is independent of data associated with each point.

[N-Dekker]

Maybe to make it more convenient to cooperate with meshes?

Possible 🤔 But then, would your change break old use cases of cooperation with meshes of the original "point data" type?

I'm not opposed to this PR, I'm just curious. I see that that "point data type" can be problematic.

[dzenanz]

Before this PR, there were two places for PointType (i.e. point coordinates). One which was envisioned for it (in PointsContainer) and another one in PointDataContainer, which was envisioned for other things (such as point color, intensity, importance, whatever). This PR changes PointData to be an unsigned char.

[thewtex]

sizeof(PointSet) == sizeof(PointSet), so for the size it does not really seem to matter.

This does seem potentially disruptive without convincing benefit if there is not a size difference and the point data is not allocated. I am not sure what unsigned char would be used for. I could see a use case for putting the transform points in the PointData.

[N-Dekker]

If I understand correctly, KernelTransform ignores the "associated data" of the PointSet. So ideally, it should support any type of PointSet, independent of its "associated data". Right? Just asking, for my understanding.

[dzenanz]

Exactly!

[thewtex]

If there is a use case for supporting any type with the Kernel Transform, then the point set or its pixel type would need to be a template parameter. But I am not seeing that use case, at least in any tests in this patch. And the change would not be to always unsigned char.