RoboSense-LiDAR / ros_rslidar

ROS drvier for RS-LiDAR-16 and RS-LiDAR-32
Other
175 stars 117 forks source link

Optimization opportunity in rawdata.cc #20

Closed facontidavide closed 5 years ago

facontidavide commented 5 years ago

Hi,

reviewing the code I noticed that there is the opportunity to optimize the code in RawData::unpack_RS32 and RawData::unpack.

The operations cos() and sin() are relatively expensive. In your case, once you load the calibration data, I am pretty sure that you can precompute only once these values:

cos(arg_vert)
cos(arg_horiz)
cos(arg_horiz_orginal);
sin(arg_horiz)
sin(arg_horiz_orginal);
sin(arg_vert)

In fact, these angles do not depend on the actual readings, therefore they should not be recalculated any single time unpack() is called.

Cheers

facontidavide commented 5 years ago

For the records, in a case like this, I usually send a PR myself, but I don't have physically any of your sensors right now ;)

Tony-HIT commented 5 years ago

Thank you. The arg_vet can be precompute, but the arg_horiz is changing.

facontidavide commented 5 years ago

We will order a sensor soon, hopefully then I will be able to contribute with a Pull Request ;)

facontidavide commented 5 years ago

One more thing: https://github.com/RoboSense-LiDAR/ros_rslidar/blob/ea6da53a7adb4915a65ddb92bc58abccb2a611b7/rslidar_pointcloud/src/rawdata.cc#L833-L858

At line 834:

       float arg_horiz_orginal = arg_horiz;

and none of these two variables are modified afterward. Why are we calculating the cos() and sin() for both arg_horiz_orginal and arg_horiz if they are exactly the same number?

      point.x = distance2 * cos(arg_vert) * cos(arg_horiz) + R1_ * cos(arg_horiz_orginal);
      point.y = -distance2 * cos(arg_vert) * sin(arg_horiz) - R1_ * sin(arg_horiz_orginal);   
facontidavide commented 5 years ago

Improvement submitted in PR #25

facontidavide commented 5 years ago

Solved by commit f1043b830d4ba5388df0b2cac17a74e5e53908ce