Removed the workaround LOCAL2 coordinate system from SphericalCoordinates.

[peci1]

🦟 Bug fix

Fixes #235 (comment)

Summary

PR #235 introduced a workaround coordinate system LOCAL2 that fixed a bug regarding the LOCAL system. This workaround was only intended to be kept for a tick-tock cycle, but it slipped from gz-math6 to gz-math7, too. This PR removes it from gz-math8.

All operations with the LOCAL coordinate system will now correctly use ENU as advertised.

Note that this breaks contract to code programmed for gz-math7 and earlier, where LOCAL system was actually WSU instead of ENU (rotated by 180 degrees heading).

There is one thing I'm not sure about. The LOCAL2 system had index 5 in the enum. Now I removed it and we're again left with just LOCAL with value 4. Maybe it would be more practical to specify LOCAL = 5 so that it is easier to spot cases in which users just supplied the value instead of the enum constant. But I'm not sure how probable that is.

This is a breaking change and will require downstream changes at least in https://github.com/gazebosim/gz-sim/blob/40aaddc7eb4ca12a03b64e18ffc101db9f9c24ec/src/Util.cc#L694 .

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[scpeters]

With this approach, downstream code using LOCAL2 will fail to compile and code using LOCAL will have a behavior change that may not be discoverable unless they read the release notes. Breaking code is at least discoverable, but I'm concerned about silent behavior changes.

• If the goal is to get rid of the buggy behavior in a discoverable way, then we should remove the LOCAL enum.
• If we want a nicer name than LOCAL2, then we should make a new name like LOCAL_Z_UP or something and deprecate LOCAL2 and/or print a warning when it is used.

What do you think?

[peci1]

Thanks for the comments. This PR was meant as a kickoff for discussion.

I support the idea of removing LOCAL and renaming LOCAL2. LOCAL_Z_UP sounds doable, though not optimal. But I don't have anything better right now.

The question is what to do with SphericalFromLocalPosition. The best way I see would also be to rename it. Agree? E.g. SphericalFromLocalZUpPosition.

One practical question: should this PR towards main branch introduce the deprecations, or should it represent the "final state" with all wrong functions completely removed, and the deprecations would be done when splitting out next major version?

[arjo129]

Hi @peci1, was wondering what the status on this was. Do we intend shipping this change with ionic (code freeze 26/8)? If we can't come to a consensus by then I will remove the beta label (I personally would prefer seeing it in this release).

Perhaps going the deperecation route is best as we have limited time to release.

[peci1]

I think it would be beneficial to have this in Ionic. I'll have a closer look at it next week.

[peci1]

Okay, I'll try renaming LOCAL2 to LOCAL_TANGENT and deprecating LOCAL.

Regarding renaming of the functions, I don't think it's necessary as it will be obvious what they do.

[peci1]

So, the PR is updated.

I've kept LOCAL and LOCAL2 and deprecated them. I've added LOCAL_TANGENT with the same numerical value as LOCAL.

I've also deprecated all *From*(Position|Velocity) and created *(Position|Velocity)From* instead. The new functions do the conversion using LOCAL_TANGENT, while the deprecated ones do them via LOCAL.

I've also added tests that verify the behavior of the deprecated functions so that some future updated doesn't break them before they're removed.

[peci1]

Continued as a part of #616