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

Add warning about broken rotations to Transform::look_at() docs #6136

Closed
wants to merge 1 commit into from

Conversation

tim-blackbird
Copy link
Contributor

Add the following note to Transform::look_at() and Transform::looking_at(): "Produces a broken rotation if the resulting forward direction is parralel with up."

@Nilirad Nilirad added C-Docs An addition or correction to our documentation A-Transform Translations, rotations and scales labels Oct 2, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right fix. IMO we should fix the underlying math instead; this is a nasty bug.

Nit: "parallel" is the spelling :)

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Oct 17, 2022

No math to fix I'm afraid. There' is just no way to create a rotation from two parallel vectors.
I rewrote the doc to make that more obvious. Maybe this is a good spot for a debug assertion?

Nit: "parallel" is the spelling :)

Gah! I've made this mistake way to many times the last few weeks. Heck, I wrote it that way again typing the above, haha

Copy link
Contributor

@Carter0 Carter0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alice-i-cecile
Copy link
Member

Ugh, right. Can we return an Option or a Result then?

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Oct 17, 2022

Is this because of the Gimbal Locking problem?

It's similar. We need two direction vectors and them being parallel effectively removes one, like the axes in gimbal lock being parallel effectively removing one axis.

Ugh, right. Can we return an Option or a Result then?

If you don't mind I add 62 instances of .unwrap() to the examples, haha

@alice-i-cecile
Copy link
Member

Hmm, right lol. Can we make a fallible variant which returns a Result then, and then point to it in these docs?

@atlv24
Copy link
Contributor

atlv24 commented Jan 19, 2023

Could use Vec3A::any_orthonormal_vector to generate an up direction if up==forward. Might be preferable to panicking, otherwise its totally possible for a game to crash just because you stand above an entity that looks at you for example. I'd rather a random up be picked than get a crash.

@rparrett
Copy link
Contributor

Linking a related issue: #3736

@MatrixDev
Copy link

MatrixDev commented Mar 15, 2023

No math to fix I'm afraid. There' is just no way to create a rotation from two parallel vectors.

Unity somehow can calculate rotation so there must be a solution. Here is an example:

transform.position = new Vector3(0.0f, 20.0f, 0.0f);
transform.LookAt(Vector3.zero, Vector3.up);

And result:

position = (0.00, 20.00, 0.00)
rotation before = (0.00000, 0.00000, 0.00000, 1.00000)
rotation after = (0.70711, 0.00000, 0.00000, 0.70711)

@tim-blackbird
Copy link
Contributor Author

Unity returns a partially randomized rotation instead.
See #7817 for an alternative PR that does that.

@alice-i-cecile
Copy link
Member

I prefer #7817: reducing mysterious footguns is better than documenting them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants