Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix raytracing origin #1850

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ValerioMagnago
Copy link

@ValerioMagnago ValerioMagnago commented Aug 13, 2021

Adds the laser ray origin to RangefinderPoint to enable
correct raytracing of range data misses into a 2D probability
grid also after scan accumulation.

Previously, all rays were casted from their hit point to the
tracking frame. This is wrong for any setup where the tracking
frame is not the sensor frame and leads to artifacts in the map.

Fixes: #947

Below an example of a map generated from a multiple 2D laser scanners configuration, which are not centered in the tracking frame, before and after the proposed fix. It can be seen clearly that the previous version of the software was cutting and cleaning the corners of the map due to the imprecise origin used for raytracing. This caused the exclusion of large structures from the map.
image
image

@tulku
Copy link

tulku commented Oct 14, 2021

@ValerioMa this is great to see! One question though, do you have a matching version of cartographer-ros that works with these changes? Maybe I did something wrong, but I failed to build the assets_writer.cc while trying this change.

@ValerioMagnago
Copy link
Author

@tulku yes we have it.
I have created the related PR on cartographer_ros (here), let me know if works for you.

@tulku
Copy link

tulku commented Oct 19, 2021

Thanks @ValerioMa I was using a very similar fix.

I am not very familiar with cartographer code, however, I was wondering why you choose to not add the origin to TimedRangefinderPoint also, and then keep the current idea of the Timed one being a RangefinderPoint + time. Instead you are adding the origin when converting from Timed to a non timed one in sensor::ToRangefinderPoint, which then needs to look for the origin of that point somewhere else.

Also, it looks like you should updated inline bool operator==(const RangefinderPoint& lhs, const RangefinderPoint& rhs) to also check for the origin.

Adds the laser ray origin to `RangefinderPoint` to enable
correct raytracing of range data misses into a 2D probability
grid also after scan accumulation.

Previously, all rays were casted from their hit point to the
tracking frame. This is wrong for any setup where the tracking
frame is not the sensor frame and leads to artifacts in the map.

Fixes: cartographer-project#947
Signed-off-by: Valerio Magnago <[email protected]>
Before the origin of the RangefinderPoint was not always initialized.
This generate instable test results. This commit fixes this problem
by initializing properly the origin to (0,0,0).

Signed-off-by: Valerio Magnago <[email protected]>
Before the origin was not taken into account when comparing RangefinderPoint.
This commit add the comparison of the origin when comparing two RangefinderPoint.

Signed-off-by: Valerio Magnago <[email protected]>
@ValerioMagnago
Copy link
Author

@tulku thanks for your suggestion and sorry delay!
Now the origin is added to the operator == and I also some tests that were not initializing the origin are now fixed.

Regarding the TimedPointCloud I left it without the origin because for that purpose we have the TimedPointCloudOridinData.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong point cloud origin when accumulating data of different sensors
2 participants