Skip to content

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