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

ang_vel.to_radians() is a footgun #13

Closed
johanhelsing opened this issue Mar 16, 2023 · 3 comments · Fixed by #46
Closed

ang_vel.to_radians() is a footgun #13

johanhelsing opened this issue Mar 16, 2023 · 3 comments · Fixed by #46

Comments

@johanhelsing
Copy link
Contributor

Because AngVel implements Deref<f32>, we will get std::f32::to_radians, which converts from degrees to radians, but AngVel is already in radians...

Not sure how we'd fix this besides removing the derive for Deref from AngVel and the other components... It's a trade-off and we'd lose some nice ergonomics. Just putting this here in case anyone else has a good idea about how to go about it.

@Jondolf
Copy link
Owner

Jondolf commented Mar 17, 2023

I guess one workaround would be to just add documentation comments that specify that AngVel (and other angular components) use radians, and hope that users understand not to use to_radians. If we want to be more explicit, we can specifically mention that to_radians probably won't do what you expect, since it's already in radians.

Something like this:

/// The angular velocity of a body, expressed in radians.
///
/// **Note**: Because the component's value is already in radians, calling the derived method `to_radians` will probably not produce the expected result.
#[cfg(feature = "2d")]
#[derive(Reflect, Clone, Copy, Component, Debug, Default, Deref, DerefMut, PartialEq, From)]
#[reflect(Component)]
pub struct AngVel(pub f32);

Like you said, we could also just remove Deref, at least from the components that use radians, but I wouldn't remove it from every component because of ergonomics. Removing it from just a few components would reduce consistency though...

@johanhelsing
Copy link
Contributor Author

Yeah, the reason it's particularly tempting is that we have Rot::from_radians, so it's particularly tempting to try to call through your IDE's auto-completion, and I don't think warnings in the reference would help much then.

e.g. this fails:

let rot = Rot::from_radians(PI);
assert_relative_eq!(PI, rot.to_radians());

I think removing the deref impl would probably be best here... but let's maybe wait and see a bit.

@Jondolf
Copy link
Owner

Jondolf commented Jun 17, 2023

For now I'll just remove the Deref impl from the 2D AngularVelocity (see #46). I agree that it's probably the best way to go about this.

If someone has a better approach, feel free to reopen this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants