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

[wpimath] Remove rotation2d value field #7490

Conversation

calcmogul
Copy link
Member

It's not part of SO(2).

@calcmogul calcmogul requested a review from a team as a code owner December 5, 2024 19:26
@calcmogul calcmogul added the 2027 2027 target label Dec 5, 2024
@Kevin-OConnor
Copy link
Contributor

Other than mathematical rigor, what is the motivation for this change?

Given the widespread usage of this class in both team and library (both vendor and otherwise) code, I'm pretty nervous about the idea of introducing a significant behavior change in December.

@calcmogul
Copy link
Member Author

calcmogul commented Dec 5, 2024

Other than mathematical rigor, what is the motivation for this change?

There's a thread about it on Chief Delphi. Here's some links of the relevant context:

https://www.chiefdelphi.com/t/possible-bugs-in-rotation2d-and-rotation3d-java/474140/2
https://www.chiefdelphi.com/t/possible-bugs-in-rotation2d-and-rotation3d-java/474140/11
https://www.chiefdelphi.com/t/possible-bugs-in-rotation2d-and-rotation3d-java/474140/12

Fundamentally, Rotation2d is not an Euler angle; it’s a 2D unit vector. Thus, no assumptions should be made about the Euler angle it returns later. Rotation3d already works like this, but Rotation2d special-cases it because 254’s version did it that way first. I’ve wanted to get rid of that behavior, but was told we couldn’t because it would break too much user code. In my opinion, if a user wants Euler angle behavior, they should just use an Euler angle (double or Angle) instead of Rotation2d.

Jared from 254 replied that the behavior we implemented wasn't actually what theirs did. Rotation2d wrapping the angle was always their intention.

We initially based Rotation2d and all the other manifold geometry methods on Sophus, so the idea is that Rotation2d is a member of SO(2). There are an infinite number of Euler angles that can map to an individual SO(2) rotation, so the convention we adopted was to always* wrap the Euler angle we provide back to the user to remove this ambiguity.

On the roboRIO’s potato-sized processor, we found value in letting Rotation2d act something as a union type, with the ability to store the Euler angle and/or the unit vector containing SO2 elements independently, and lazily computing (and caching) the missing fields upon asking for them: FRC-2023-Public/src/main/java/com/team254/lib/geometry/Rotation2d.java

But conceptually, we want it to be as if getRadians() was always computed anew from the SO(2) rotation and so adopted a canonical wrapping scheme. If you want to do math on Euler angles without adopting SO(2) semantics, then you should use doubles or a first class Angle type.

* as a (very small and likely premature) performance optimization we did allow construction without enforcing wrapping as a deliberate breach of this contract only because in many cases the caller trivially knows they aren’t wrapped, or knows that whatever math they are doing later won’t care.

Unfortunately, we didn’t realize this was the design intent and baked that exception into the API contract of WPILib’s version. 2027 is our chance to fix that.

I'm pretty nervous about the idea of introducing a significant behavior change in December.

This PR is targeting the 2027 branch, not main.

@Kevin-OConnor
Copy link
Contributor

This PR is targeting the 2027 branch, not main.

Thanks, I'll just be over here removing my foot from my mouth. No issues with making this change targeting '27.

@calcmogul calcmogul force-pushed the wpimath-remove-rotation2d-value-field branch from d57f421 to 6b1c11c Compare December 6, 2024 19:45
@PeterJohnson PeterJohnson merged commit 144e79a into wpilibsuite:2027 Dec 7, 2024
42 checks passed
@calcmogul calcmogul deleted the wpimath-remove-rotation2d-value-field branch December 7, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2027 2027 target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants