Stick to the global utm_zone_ when transforming gps to UTM #627
Conversation
Timple
force-pushed
the
support-multiple-utm-zones
branch
from
33519ed to
35980a0
Compare
| NavsatConversions::LLtoUTM(latitude, longitude, cartesian_y, cartesian_x, utm_zone_tmp); | ||
| int zone_tmp; | ||
| bool nortp_tmp; | ||
| GeographicLib::UTMUPS::Forward(latitude, longitude, zone_tmp, nortp_tmp, cartesian_x, cartesian_y, utm_zone_); |
There was a problem hiding this comment.
Why can't we keep using LLtoUTM here? Doesn't your other PR just wrap to the GeographicLib call anyway?
There was a problem hiding this comment.
Sorry for the confusion.
I started on the other PR. But since that makes the whole conversion functions almost empty, one might as well use the functions directly, removing all custom functions. Actually I would want to remove the whole conversions file in this PR. But I don't know who might be using it.
The reason that this is also needed is because the library uses a boolean northp instead of string parsing the utm_zone_ variable. It's much easier to track the boolean than to keep the information in a string and encode/decode that.
There was a problem hiding this comment.
OK, sounds good. As I said in the other PR, we'll remove that file for ROS2 Galactic onward.
|
Clarifying note: As the diff shows, instead of These two variables where first 'combined' in a string and had to be encoded/decoded |
|
@Timple, would you mind rebasing and fixing the conflict? Thanks. |
Timple
force-pushed
the
support-multiple-utm-zones
branch
from
40b359a to
dc7146b
Compare
|
Done |
|
Thank you for the tests. It makes me feel better about not having a live robot with a GPS for testing. :-/ |
|
I actually created the test first, because that was initially less work than dragging the live robot outside :) |
|
Would it be possible to get a melodic and noetic release with this fix/feature? |
|
Yes, will bloom this today. |
|
@Timple, do you need this to go into melodic as well? |
|
Yes please, we're migrating to Noetic, but the actual machine is still on melodic. |
|
OK, will cherry-pick this change. |
|
Does that mean you also want this in melodic? |
|
Yes to both. Simulator is already on Noetic while actual machine is Melodic. edit: I now realize melodic has it's own branch without this patch. If this is too much hassle we can simply build from source until the migration is finished. One more reason to push for Noetic 🙂 |
|
OK. I'll just go back through the recent noetic commits and find anything that should be backported. Thanks. |
* Stick to the global utm_zone_ when transforming gps to UTM
|
@Timple, if you have some time, would you mind trying out the |
|
Yup, seems to work! |
|
Should I also port this to the ROS2 branch? And a subtle ping for the bloom release 🙂 |
|
Bloom release was done a few weeks ago. Did they do a sync recently? EDIT: Melodic, too. |
|
Ow, whups. I only looked at the latest entry in https://github.com/cra-ros-pkg/robot_localization/blob/noetic-devel/CHANGELOG.rst But the last version is not on top it seems... |
|
Odd! I'll correct that in the next one. |
Fixes #222 partly.
It doesn't handle multiple UTM zones but at least sticks to the initial one. Preventing the jump in position.
For the actual multiple UTM solution #309 would need to be solved.