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

add Geocentric and Local Cartesian projectors #244

Conversation

mantkiew
Copy link
Contributor

The UTM projector creates shifts in projected maps relative to actual world positions.

Added Geocentric (ECEF) and LocalCartesian projectors from geographic lib as available projectors.

These projectors have the advantage over the UTM projector in that they properly use lat/lon/alt to compute XYZ using WGS84 ellipsoid, whereas the UTM projector only uses lat/lon to convert into X/Y/Z on the ellipsoid and just adds elevation, which is incorrect.

image

In the image above, the point P' is the shifted point created by the UTM projector, compared to the correct point P.

@m-naumann
Copy link
Member

@mantkiew thanks! Briefly browsing through: would you please add tests?

@mantkiew
Copy link
Contributor Author

@m-naumann I added the tests and mentioned the new projectors in the README and CHANGELOG as well.

BTW. We are using these extensions/fixes from my PRs in this project:

https://uwaterloo.ca/waterloo-intelligent-systems-engineering-lab/news/avin-lanelet2-mapping-project-completed

Additionally, the WISE ADS now also depends on these projectors

https://uwaterloo.ca/waterloo-intelligent-systems-engineering-lab/projects/wise-automated-driving-system

We previously used Lanelet1 but we're now migrating to use Lanelet2.

@mantkiew
Copy link
Contributor Author

Just wanted to comment about the need for alternative projectors: in our WISE ADS stack, we always use Geocentric and LocalCartesian throughout our system to define a few frames in the TF tree. If the map is loaded using the UTM projector, it is shifted as compared to using LocalCartesian because of the way it treats the elevation. Elevation is important to us for various projection tasks (e.g., projecting map lines into the camera frame).

@m-naumann
Copy link
Member

@m-naumann I added the tests and mentioned the new projectors in the README and CHANGELOG as well.

BTW. We are using these extensions/fixes from my PRs in this project:

https://uwaterloo.ca/waterloo-intelligent-systems-engineering-lab/news/avin-lanelet2-mapping-project-completed

Additionally, the WISE ADS now also depends on these projectors

https://uwaterloo.ca/waterloo-intelligent-systems-engineering-lab/projects/wise-automated-driving-system

We previously used Lanelet1 but we're now migrating to use Lanelet2.

Thanks a lot! And nice to see the usage. You'll certainly not regret the update to lanelet2 ;) And at least I personally can back your call for correct elevation consideration.

Could you also add tests for the python bindings (even though we didn't provide them initially, we should try to increase as much covered code as possible).

And please bear with us for a bit, we need to get a public CI pipeline up and running again, such that static code analysis is available on github and there is no longer a need to report issues from the internal pipeline.

@mantkiew
Copy link
Contributor Author

@m-naumann I have added the tests. Note that the values coming back from the projectors have to be rounded for comparison. The maximum number of decimals that work is 8. The tests pass, of course.

I believe that is ready to be merged.

@mantkiew
Copy link
Contributor Author

@m-naumann on the second thought: maybe we should round the values to the eight decimal ourselves in the API? That would take away the burden from the user.

@github-actions
Copy link

This PR is stale because it has been open for 90 days with no activity. Is this PR still work in progress? If yes, mark it as "WIP" or comment, otherwise it will be closed in 30 days.

@github-actions github-actions bot added the Stale label Oct 19, 2022
@m-naumann
Copy link
Member

And please bear with us for a bit, we need to get a public CI pipeline up and running again, such that static code analysis is available on github and there is no longer a need to report issues from the internal pipeline.

This is now done, @mantkiew would you please merge master into your branch to have the pipeline running for this PR?

@mantkiew
Copy link
Contributor Author

Wonderful! Glad to finish this up. On it....

@mantkiew
Copy link
Contributor Author

@m-naumann All GitHub actions have successfully completed in my repository. You have to allow them here.

@mantkiew
Copy link
Contributor Author

@m-naumann the test job on noetic fails with some nonsense Error: sha is not defined. Could you please restart it? All jobs passed on my repository (see https://github.com/wiselabuw/Lanelet2/actions/runs/3281889493)

@immel-f
Copy link
Contributor

immel-f commented Oct 19, 2022

@m-naumann the test job on noetic fails with some nonsense Error: sha is not defined. Could you please restart it? All jobs passed on my repository (see https://github.com/wiselabuw/Lanelet2/actions/runs/3281889493)

This is a bug in the new CI, once #259 is merged it should also pass here.

@github-actions github-actions bot removed the Stale label Oct 20, 2022
Copy link
Contributor

@poggenhans poggenhans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a few comments. Sorry for taking so long. Once these points are resolved, we can finally merge this PR.

@mantkiew
Copy link
Contributor Author

@poggenhans ok, this is now ready to go.

@mantkiew
Copy link
Contributor Author

mantkiew commented Nov 1, 2022

@poggenhans hi, anything preventing this to be merged?

@immel-f immel-f merged commit a5ecd48 into fzi-forschungszentrum-informatik:master Nov 2, 2022
@mantkiew mantkiew deleted the add_geocentric_and_local_cartesian_projectors branch November 2, 2022 14:02
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.

None yet

4 participants