RobotecAI / RGLGazeboPlugin

Other
61 stars 11 forks source link

Add Laserscan support #28

Closed ajsampathk closed 9 months ago

ajsampathk commented 11 months ago

Issue link: https://github.com/RobotecAI/RGLGazeboPlugin/issues/26

Testing

Tested on Ubuntu 22.04.

Additions

msz-rai commented 11 months ago

@ajsampathk

The LaserScan messages need to have an intensity or the ros2_gz_bridge will not be happy and seg faults. I have set a static 100.0 value to all the intensity value here but we can easily get that from the API as well with the rgl_node_points_yield API call.

Thank you for noticing it. This is fine for now :+1:

However, this relates to Request to add "intensity" field to the PointCloud2 message #15, So I have excluded adding the RGL_FILED_INTENSITY_F32 to the yieldFields to obtain the intensity value here. (will be happy to come up with a PR for this as well if there is nothing planned for it yet)

We haven't planned this one shortly. We will be always happy for the contribution!

ajsampathk commented 10 months ago

@msz-rai Sorry about the delay, but I think I managed to apply most of the suggestions.

One thing regarding:

These values could be also retrieved from the lidarPattern container:

  • scanHMin, scanHMax - the yaw angle of the first and the last ray in the container (it is needed to convert rgl_mat3x4f to angles, see AnglesToRglMat3x4f for reference).
  • scanHSamples - the size of the container.

It was a cool idea which I tried out, which you can check in commit ddd926c, and ultimately decided to only do this for scanHSamples, My reasoning is this:

The number of samples however is a straightforward and is a good substitute to extracting it from SDF.

Let me know what you think! Ready for review once again.

Thanks

msz-rai commented 9 months ago

@ajsampathk Would you like to add something more to this PR or should I merge it now?

ajsampathk commented 9 months ago

@msz-rai Just working on the publishMessageType to remove the sdf->HasElement inside the configuration function. But it can honestly go in through a different PR.

I'm okay merging this if you are!