-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Fix look_to resulting in NaN rotations #7817
Fix look_to resulting in NaN rotations #7817
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like Elm deciding that 1 / 0 = 0
, it might not comply with the purely theoretical definition, but I personally think that functional beats pure. Especially if the selected fallback direction is clearly defined.
@@ -120,6 +120,11 @@ impl Transform { | |||
|
|||
/// Returns this [`Transform`] with a new rotation so that [`Transform::forward`] | |||
/// points towards the `target` position and [`Transform::up`] points towards `up`. | |||
/// | |||
/// In some cases it's not possible to construct a rotation. Another axis will be picked in those cases: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this means we should be returning Result<Self, SomeError>
instead, seems odd to silently fail here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we're not silently failing: we're performing a "best-attempt". I'd be open to try
variants too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to work by default, instead of having to add unwrap / error management everywhere this pops up
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Liam Gallagher <ljpgallagher@xtra.co.nz>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Liam Gallagher <ljpgallagher@xtra.co.nz>
Objective
looking_at
is called withTransform
's origin as target #3736Transform::look_at()
docs #6136Solution