SphericalCoordinates: Added SPHERICAL_DEG and SPHERICAL_RAD, deprecated SPHERICAL

[peci1]

🦟 Bug fix

Fixes #235 (comment)

Summary

This change is made to unify clarify the units that are inputs/outputs to the coordinate transforms.

This PR deprecates enum value SPHERICAL used downstream 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]

I'm concerned about the behavior change here if people don't read the release notes, trying to think if there are better alternatives

[peci1]

Yes, I agree this would be impractical. This PR is open for discussion. One obvious solution would be to rename the function, e.g. to TransformPosition (and, for consistency, also rename VelocityTransform). That would mean much more "unrelated" downstream changes, but at least no code would silently fail.

[arjo129]

Maybe use a new CoordinateType for SPHERICAL_DEGREES? We can deprecate SPHERICAL if need be.

[peci1]

Renaming the enum constant and deprecated the old one sounds like a good way to go. At least, it wouldn't affect downstream uses which only use metric coords. And it would issue a deprecation warning on all uses of spherical conversions. I'll try to sketch an update of this PR with this rename.

Thinking about it a bit more, the problem this PR resolves is caused by the fact that gz math is missing a Coordinate3 class, which could hold either metric or spherical coords, but different from Vector3, it would know which ones are stored. And it could have accessors that check the type of the stored values. Would something like that be a welcome PR?

[arjo129]

It does sound like a great idea. I'd be happy to review it. But given the tight timeline for feature and code freeze I think we will have to merge it after September and backport.

[peci1]

I've drafted #616 adding the CoordinateVector3 class. I guess this falls behind the feature freeze, so it is waiting for Gazebo 9.

The only thing that I don't like is that if we choose to go this way, it would again start to make sense to have SPHERICAL constant, while SPHERICAL_DEG and SPHERICAL_RAD would lose sense. I don't really know what to do with this.

[arjo129]

Hmmm one alternative is to merge #616 into this PR and deprecate these functions altogether. In my mind that would be the cleanest way from the end user perspective.

[peci1]

I can't say I fully undestand your proposal. I'll try to summarize and please tell if I understood you correctly.

There's also #596 which deprecates GlobalFromLocalVelocity etc. and in its current version, it replaces it with GlobalVelocityFromLocal which does the local frame computations correctly.

So if we took PRs 596, 597 and 616 together, the result could be:

• keep SPHERICAL constant and do not deprecate it
• do not add SPHERICAL_DEG and SPHERICAL_RAD constants (as is currently done in this PR)
• deprecate LOCAL and LOCAL2 constants and add LOCAL_TANGENT (as done in 596)
  • alternatively, the LOCAL constant could be kept (and LOCAL_TANGENT not added and LOCAL2 deprecated) with a dual meaning for one tick-tock cycle - in (Position|Velocity)Transform(Vector3d), it would lead to the old wrong behavior, while in (Position|Velocity)Transform(CoordinateVector3) it would yield the correct results. There would still be a chance that when migrating the code, users would miss the note that the computation differs, but they would anyways need to edit the line of code using the function - and I guess if they're already editing it, they should have an idea how to do the migration...
• deprecate GlobalFromLocalVelocity(Vector3d) and friends (as done in 596)
• add GlobalFromLocalVelocity(CoordinateVector3) etc. (as done in 616)
• do not add GlobalVelocityFromLocal(Vector3) from 596
• deprecate PositionTransform(Vector3d) and VelocityTransform(Vector3d) and add their CoordinateVector3 counterparts (as done in 616)

This would solve all the problems 596 and 597 tried to solve.

It would also have quite some effect on downstream code, effectively requiring almost all users of SphericalCoordinates.hh to do some changes. But the result would be that the downstream code will be working correctly and it will be unambiguous regarding units. There might probably be a little performance penalty (CoordinateVector3 uses PIMPL with unique_ptr and std::variant). But I don't think this class would ever be used in a performance-critical path...

[arjo129]

I think we are on the same page.

If we go with #616 then I don't see any value in SPHERICAL_DEG and SPHERICAL_RAD. The end users will have to rewrite their code twice. Yes, this change is more drastic but it will provide a much better UX. It probably makes sense to include #616 with ionic.

[peci1]

Great! I'll give it a go later today and add some tests.

[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.

[peci1]

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

[peci1]

I took a little bit different approach in the end. I've kept SPHERICAL to represent spherical coordinates in radians, but deprecated it in favor of the more verbose SPHERICAL_RAD. And I've added SPHERICAL_DEG to allow passing in/out spherical coords in degrees.

Now I think every piece of code that uses PositionTransform should be verbose enough to make it clear what angular units are in the in/out vectors.

[peci1]

Continued as a part of #616