Copy cloud in cloud

[nizar-sallem]

Add ability to copy a cloud inside another one while interpolating borders.

[nizar-sallem]

@jspricke can we pull this ?

[nizar-sallem]

@taketwo, can you have a look at it have some stuff pending that require it ;)

[taketwo]

I see you have something adapted from OpenCV. As far as I know we already have some other stuff from OpenCV in module 2d. I am not sure, but maybe it makes sense to transfer interpolate here and put in its own file? I guess 'common/utils.h' is a bit obscure location anyways.

[nizar-sallem]

hmm, well I didn't put it in 2d since it is meant for interpolating a border and not only in 2d.
Maybe putting in in some pcl::details namespace inside io.h would be better ?

[taketwo]

But where would InterpolationType enum go, to 'io.h'? Also strange...
What about putting enum and the function in 'common/iterpolation.h'?

[taketwo]

Regarding the new exception type you introduced, I think BadArgumentException (singular) is more common than BadArgumentsException (plural) with 678k vs. 13k hits in Google.

[taketwo]

BORDER_TRANSPARENT does not seem to be handled by interpolate()

[jspricke]

@taketwo It's used in copyPointCloud. @nizar-sallem can you add a comment about that on in the Doxygen?

[taketwo]

I am not sure, does you function require organized point clouds? If this is the case would be nice to mention in docs and also make a run-time check.

Also there are a few places where a space is missing (near memcpy calls).

I think this is pretty much everything from my side.

[jspricke]

I would vote for putting interpolate() into io.hpp (and rename it to interpolate_point_index; same for the enum). Reson: a utils.cpp is always a file for lot's of random stuff, which is hard to maintain and and to structure. Also, Interpolate is not really specific and could lead people to wrong ideas).

[taketwo]

I am wondering if this new copyPointCloud() is actually a copy point cloud function like the others in 'io.hpp'. I think it is more about adding borders than copying points and, as such, does not belong to IO.

[nizar-sallem]

Well it really copies a cloud inside another bigger cloud that is what it does independently of the border stuff.
It doesn't require that the cloud is organised, if not organized you can still copy it to bigger cloud.
+1 for the exception
+1 for renaming interpolate and putting it in io.h

[taketwo]

As far as I see you create a new cloud which is the same size as the input plus the borders:

cloud_out.width    = cloud_in.width + left + right;
cloud_out.height   = cloud_in.height + top + bottom;
cloud_out.points.resize (cloud_out.width * cloud_out.height);

Also height is always 1 for non-organized point clouds.

[nizar-sallem]

Yes, but from a semantic point of view you are copying a cloud A inside a bigger cloud B while interpolating border or not.

[taketwo]

Well I will agree with you, as an author you know better :)

But I am still concerned about non-organized clouds. Suppose you have a 10x10 cloud and want 1 point border in each direction. If it is organized, then width = 10, height = 10, and output cloud will have width = 10 + 1 + 1 = 12, height = 10 + 1 + 1 = 12, which gives a total of 144 points. On the other hand, if the input cloud is not organized, then width = 100, height = 1, and output cloud will have width = 100 + 1 + 1 = 102, height = 1 + 1 + 1 = 3, which gives a total of 306 points.

[taketwo]

Actually, discard my calculations, they are valid, but do not prove anything.
I simply mean that for non-organized clouds this border thing does not make sense.

[nizar-sallem]

Well it makes sense if you think about it as a single line from a bigger cloud, no ?
If you have some really huge cloud and would like to do the interpolation without exploding the memory you can put each line on a cloud and do the interpolation+copy. At least that is what I have in mind.

[taketwo]

Okay, again you probably know better :)

I guess this whole discussion happened simply because I do not have a slightest idea of what is the use-case of the function :)

[nizar-sallem]

Just wait then you will get your answer Sir :) next PR uses it extensively.

[taketwo]

Haha I guess so, but that's a chicken and egg problem in a sense. You can not make the next PR without merging this one, we can not adequately discuss this one without seeing the next one. Doh :)

Gathering together Jochen's and my suggestions:

• exception
• rename/move to io
• few missing spaces
• doxygen comment

And then we are good to go.

[nizar-sallem]

Yup :)
I am recompiling right now will push as soon as tests pass

[nizar-sallem]

Done

[nizar-sallem]

@taketwo where are the missing spaces please ?

[taketwo]

Lines 409, 436.

[taketwo]

and rename it to interpolate_point_index; same for the enum

@jspricke I think it was you who suggested this name. Is there any reason why you used snake-case?

[jspricke]

No :). Just wanted to point out a better name.

[nizar-sallem]

lol I will rename it to PCL rules.
@taketwo thanks for the whitespaces didn't notice them ;)

[nizar-sallem]

Done