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/set utm map frame change #1

Closed
wants to merge 3 commits into from

Conversation

MCFurry
Copy link
Member

@MCFurry MCFurry commented Aug 31, 2023

Proposal for fixing the issue where a user-utm zone does not reset the transform of the map-frame and thus gives issues.

Also added a test for this. Please review and when approved we can move this PR to the forked repo instead and apply similar diffs to the ROS2 branch.

@MCFurry MCFurry requested a review from Timple August 31, 2023 06:57
@MCFurry MCFurry self-assigned this Aug 31, 2023
@@ -210,6 +210,10 @@ class NavSatTransform
//!
bool use_local_cartesian_;

//! @brief Whether we want to have our UTM zone fixed, and not change it if we move outside of it
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like an option. But I don't see where we can set the option :)

Copy link
Member

Choose a reason for hiding this comment

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

Actually I wouldn't implement it as optional unless requested by a reviewer for backward compatibility. This will always lead to issues right?

Copy link
Member Author

Choose a reason for hiding this comment

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

??

Copy link
Member

Choose a reason for hiding this comment

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

Whether we want to have our UTM zone fixed

Is there any valid reason not to want it fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm maybe I should rephrase the comment.

Default behavior of navsat_transform is:

  1. Wait for valid gps
  2. Determine utm zone from gps coordinates (lat/lon)
  3. Determine map transform based on utm zone AND lat/lon
  4. Use this zone forever

Proposed behavior for setUTMZone:

  1. Let user force set a UTM zone
  2. Determine map transform based on user utm zone
  3. Use this zone forever

So yes, the zone is indeed always fixed. The flag is just used in the setTransformGps() call such that the user UTM zone is taken and the correct utm_meridian_convergence_degrees is calculated from that.
Otherwise the call to setTransformGps() uses lat/lon as input and might conclude a different UTM zone and different utm_meridian_convergence_degrees.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then yes, please rephrase the comment. This sounds like we can choose not to fix stuff.

@@ -210,6 +210,10 @@ class NavSatTransform
//!
bool use_local_cartesian_;

//! @brief Whether we want to have our UTM zone fixed, and not change it if we move outside of it
//!
bool force_user_utm_;
Copy link
Member

Choose a reason for hiding this comment

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

user? It's typically robots 🙂

But it sounds like you don't force much. reset_utm_?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's similar to when a user calls the /datum service, from that point on a custom datum is enforced.
Now, when /setUTMzone is called, from that point on a utm datum is enforced.

@Timple
Copy link
Member

Timple commented Aug 31, 2023

Good to go!

@MCFurry
Copy link
Member Author

MCFurry commented Sep 1, 2023

Closing this one as I created one to the forked repo: cra-ros-pkg#830

@MCFurry MCFurry closed this Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants