Fix rendering of pointclouds in PCLVisualizer

[larshg]

Fixes #4040.

The culprit was the "memory leak" fixed in #3687.

Note:

Can we in some way avoid the memcpy - or maybe concatenate them to a single copy?
The XYZ will always be aligned?

Are we preparing to change at compile time, the underlaying datatype, ie. switch between float and double. Then this would require a bit more revamp I guess :)

[kunaltyagi]

The XYZ will always be aligned?

This is not a guarantee (and stated explicitly in PCL's codebase)

to change at compile time, the underlaying datatype

In future maybe. I don't have any plans for that right now.

The culprit was the "memory leak" fixed in #3687

The leak was needed, as in SetArray takes ownership of the array?

[kunaltyagi]

LGTM. It's simpler even.

[kunaltyagi]

Adding to milestone since it is a fix to regression in a key feature (and PCL 1.11.1 isn't planned, at least right now)

[larshg]

The XYZ will always be aligned?

This is not a guarantee (and stated explicitly in PCL's codebase)

So best to keep the single copy for each component I guess.

to change at compile time, the underlaying datatype

In future maybe. I don't have any plans for that right now.

Oki, I won't overthink this then and keep it as is.

The culprit was the "memory leak" fixed in #3687

The leak was needed, as in SetArray takes ownership of the array?

Yes, the 0 (save) indicates that the array should be deleted by VTK. Also, since no delete method is explicit set (overload of SetArray), its default is free().

I'm unsure exactly how VTK expands its array, with the AddNextTuple vs. us allocating the full array, so a benchmark would have been nice :)

But yes, I also think its simpler to read now.

There are places in the color handles that has the same procedure, which could be simplified, though again I'm not sure if it gives a small performance/memory degradation.

[larshg]

I think if it adds a point, which there is no space for, it increase its array with 1 / numberofcomponents, which is 3 (XYZ), ie 1/3 of its current index count, each time it expands its array.
So this will eventually leads to overshoot of array sizes.

Maybe, go back to the array/malloc approach.

[larshg]

Fixed it, with adding call to resize before adding values to the array.

New code added

[themightyoarfish]

Confirmed to fix #4040 on osx.

[SergioRAgostinho]

Once the variable scope thing is addressed feel free to merge.

[kunaltyagi]

Pending CI