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 incorrect rotation value multiplication order #564

Closed
wants to merge 2 commits into from
Closed

Fix incorrect rotation value multiplication order #564

wants to merge 2 commits into from

Conversation

jngbsn
Copy link
Contributor

@jngbsn jngbsn commented Sep 24, 2020

Without this, the rotation is applied before any translation, causing it act as if (0, 0, 0) is the rotation origin, instead of rotating around its own local origin.

@Moxinilian Moxinilian added the C-Bug An unexpected or incorrect behavior label Sep 24, 2020
@cart
Copy link
Member

cart commented Sep 25, 2020

While I think this change is useful, it creates an inconsistency with the "apply_scale" method, which "scales" the current matrix relative to the origin. Rotate() was intended to be the corollary to that method. But rotating the "rotation" property (like you do in this pr) makes a lot more sense to me.

However I'm starting to think that the "apply operation directly to current matrix" convenience methods like apply_scale and rotate cause more trouble/confusion than they're worth.

Can we make the following changes in this pr:

  1. remove apply_scale and apply_uniform_scale from Transform/GlobalTransform
  2. add your "rotate fix" to GlobalTransform

@cart
Copy link
Member

cart commented Sep 26, 2020

Hmm this latest build error isn't your fault. Clippy nightly apparently just got updated and added new rules.

@cart
Copy link
Member

cart commented Sep 26, 2020

I'm honestly getting pretty tired of nightly clippy breakage. I think we should switch back to stable.

@cart
Copy link
Member

cart commented Sep 26, 2020

I'm fixing clippy here: #577

@cart
Copy link
Member

cart commented Oct 2, 2020

We will likely want to close this now that #596 is the direction we're taking the transform in. I'll leave it open for now, but if we merge #596, this will become unnecessary.

@jngbsn jngbsn closed this Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants