search

swri-robotics / marti_common

Common utility functions for MARTI
BSD 3-Clause "New" or "Revised" License
54 stars 63 forks source link

local_xy_util doesn't choose shortest distance when crossing longitude of 180 degrees #644

Closed tothgl closed 1 year ago

tothgl commented 2 years ago

If the local xy origin and the current location are on opposite sides of the longitude 180/-180 line, the function LocalXyWgs84Util::ToLocalXY() calculates the position difference the long way around the globe instead of the short distance across the 180 degree boundary.

pjreed commented 2 years ago

Good catch, this is probably something @kriskozak will want to know about.

tothgl commented 2 years ago

FYI, I forked the repo and checked in the code changes and test cases I made on the "PR644" branch. I can create a pull request if desired.

danthony06 commented 2 years ago

We're always happy for pull requests.

tothgl commented 2 years ago

I made my changes to version 2.14.2 which was the version released with kinetic and noetic. It looks like master and 2.15.0 have a lot of changes (none to the two files I changed) and I haven't been able to get either of them to compile yet. Are they targeted for ROS2? Should my pull request be against 2.14.2 or master (or something else)?

danthony06 commented 2 years ago

Right now, we're releasing ROS1 based on the master branch, and ROS2 based on the ros2-devel branch. Sorry for the confusion about the releases, but so far we've been trying to release both versions out of one repo.

danthony06 commented 2 years ago

If you can apply the fix to whichever branch you feel most comfortable with, we can port it over to the other branch.

kriskozak commented 2 years ago

Good catch, this is probably something @kriskozak will want to know about.

Thanks for the heads up. The issue at 180 degrees is not a surprise, and I don't recall exactly, but it may have been known of previously and ignored because there aren't many practical applications that would trigger the issue. Given that there is a simple fix, though, it makes sense to just fix it.

Out of curiosity @tothgl, what led you to discover this issue, and does it involve a boat :-)?

While we're at it, I'll note that there will also be issues with this Cartesian mapping near the poles, but I don't think there is a similarly simple fix for that.

danthony06 commented 2 years ago

Okay, this is now fixed in the ROS1 branch, and we just need to bring it into the ros2-devel branch. Thank you for the good PR addressing this.

tothgl commented 2 years ago

@kriskosak No, I'm afraid there was no boat involved nor did I get to go to Fiji :-) I was just writing test cases for a gps driver using the worst cases I could think of. I thought about the pole issue and didn't have any ideas for that either. I wanted to let the issue go, but it kept nagging at me...

danthony06 commented 1 year ago

It is now fixed in ROS 2 as well.