Stepping back. Eigen::Affine3f is better than the recently introduced changes in #20

[facontidavide]

Hi,

with the recent PR I wanted to test that there could be a considerable improve in execution time avoiding useless calculations of cos and sin.

Introducing rotateX, rotateY and rotateZ, I believe that the code become more readable.

But thinking twice, I think that the way I would do it myself would be to create an affine transform once and use it for each of the points, instead of doing multiple, simple rotations and translations.

Before doing this change, I wanted to get some feedback from the other users and the author...

[StefanGlaser]

Hi again ;)

I also thought about this issue but haven't looked at it into detail. My thought is that the input cloud is already expected to arrive in order. Using this fact, we could transform all 16 / 32 / 64 scan rings simultaneously, using the same transform for all rings instead of creating a transformation per point. This would reduce transformation creations by the factor of scan rings.

[facontidavide]

To be fair, as you can see in the picture, the operation pointAssociateToMap isn't very important in terms of CPU usage, in laserMapping.

So, it is mostly a matter of code readability, not performance.

On the other hand, the contribution to total calculation in TransformToEnd is more significant

Can anyone explain to me what this line of code is supposed to mean?

     float s = 10 * (pi->intensity - int(pi->intensity));

[StefanGlaser]

Very interesting figures! Sometimes there is an option to optimize, but the effect is negligible. In this case I would prefer readability, too.

Regarding the code line you asked about: The scanRegistration component sets the intensity of a point to its ring number + a weighted time factor based on its horizontal angle. The idea is to encode the scan index and relative point time within the point intensity. This way you can access the scan index by:

int scanIdx = int(point.intensity);

and its relative scan time via:

float s = (1 / weight) * (point.intensity - int(point.intensity));

However, there are currently some issues in the calculation of the involved angles, resulting in wrong interpolations of the relative times as I mentioned in my other issue. Even though it still seems to work somehow...

Btw.: The magic constant here (10) is the result of (1 / SCAN_PERIOD) in my opinion.

[alittleharry]

@facontidavide i agree with @StefanGlaser about encoding the scan index and relative point time within the point intensity. to explain this line we must know that, all points arrivie not in same time, so the sensor have different pose(transform) for different point. Here, the author assume that each frame of data acquisition time is 0.1 seconds(VLP16's default period), for every point use its horizontal angle to compute its time from start of this frame and use the relative percentage（time/0.1） with the end transform of this frame to compensate for distortions. So i don't think the effect is negligible, think about the case when the sensor is moving at speed of 10m/s, for a frame of pointcloud from the lidar driver, the first point and the last point will have approximate coordinates in sensor frame, in fact the two points shoud have 1m away in fixed frame(laser_odom frame). For above reason, in transformToEnd() function, every point have its own transform base its relative percentage to compensate the distortions. Of course, if the sensor is moving vely slow, the effect is negligible and transformToStart() and transformToEnd() not needed.

[alittleharry]

@facontidavide what software you use in above pitcure?

[facontidavide]

Hotspot from kdab. You can find it on github. It is a graphical interface to visualize the results of perf

[StefanGlaser]

@alittleharry I totally agree with you. I never argued that the correction of the cloud distortion is negligible. I was referring to some code optimizations, which - according to the performance figures - do not really contribute to the overall runtime.

[heroacool]

Hi,

with the recent PR I wanted to test that there could be a considerable improve in execution time avoiding useless calculations of cos and sin.

Introducing rotateX, rotateY and rotateZ, I believe that the code become more readable.

But thinking twice, I think that the way I would do it myself would be to create an affine transform once and use it for each of the points, instead of doing multiple, simple rotations and translations.

Before doing this change, I wanted to get some feedback from the other users and the author...

It's absolutely useful!