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

Fix camera rotation around focus point #644

Merged
merged 1 commit into from
May 30, 2022

Conversation

chrisprice
Copy link
Contributor

@chrisprice chrisprice commented May 29, 2022

This fixes the remaining part of #18.

I've noticed a couple of possible further issues -

  • The larger the rotation, the less intuitive the axis of rotation becomes. This is due to it being fixed to x/y axis rather than relative to Camera rotation.
  • Whilst the transform in Camera is broken into rotation and translation matrices, the current implementation of Rotation does not guarantee rotation is an orthogonal matrix (under heading Properties of the Transform Matrix).

edit by @hannobraun:
Close #18 (to make sure the issue is closed when this PR is merged)

@chrisprice chrisprice requested a review from hannobraun as a code owner May 29, 2022 20:35
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you @chrisprice, looks and works great!

The larger the rotation, the less intuitive the axis of rotation becomes. This is due to it being fixed to x/y axis rather than relative to Camera rotation.

Good point! Feel free to open an issue about that. If not, I can do it myself (already made a note, will do it once I get to it).

Whilst the transform in Camera is broken into rotation and translation matrices, the current implementation of Rotation does not guarantee rotation is an orthogonal matrix (under heading Properties of the Transform Matrix).

That looks like an interesting article! Unfortunately I don't have the time to read it completely right now, so I don't quite follow why this lack of orthogonality is a problem. But I'd just like to emphasize that the way that code is written isn't very intentional (beyond trying to get a certain result), so further improvements are certainly welcome.

@@ -43,7 +43,7 @@ impl Rotation {

let inv = trans.inverse();

camera.rotation = trans * rot_y * rot_x * inv * camera.rotation;
camera.rotation = camera.rotation * trans * rot_y * rot_x * inv;
Copy link
Owner

Choose a reason for hiding this comment

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

Oh man, these kinds of fixes are the best. One line changed, but probably took many hours to figure out 😂

@hannobraun hannobraun enabled auto-merge May 30, 2022 13:01
@hannobraun hannobraun merged commit 96c7f2c into hannobraun:main May 30, 2022
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 this pull request may close these issues.

Computation of focus point is not fully correct
2 participants