Skip to content

Fix rotation around Y axis #262

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

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Conversation

moritzblume
Copy link
Contributor

Current implementation rotates clockwise which is not the norm and not consistent to the implemented X and Y rotation matrices.

This PR fixes issue #261 .

Current implementation rotates clockwise which is not
the norm and not consistent to the other implemented rotations.
@google-cla
Copy link

google-cla bot commented Apr 19, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@moritzblume
Copy link
Contributor Author

Any updates? When will it be merged?

@johnmccutchan
Copy link
Collaborator

Hi,

If you are still around I'd be interesting in merging this change.

@johnmccutchan
Copy link
Collaborator

Ping @moritzblume

@moritzblume
Copy link
Contributor Author

moritzblume commented Jun 20, 2023 via email

@johnmccutchan johnmccutchan merged commit 048777a into google:master Jul 19, 2023
@kevmoo
Copy link
Collaborator

kevmoo commented Jul 20, 2023

johnmccutchan added a commit that referenced this pull request Jul 26, 2023
johnmccutchan added a commit that referenced this pull request Jul 26, 2023
@johnmccutchan
Copy link
Collaborator

@moritzblume We discovered two issues with the CL and so we had to revert, please address the following:

  1. Broke some tests (run dart test to reproduce)
  2. Made rotateY inconsistent across Matrix3 and Matrix4 classes.

For issue (1) is pretty trivial, just swapping the signs in the test expectation.
For issue (2) both Matrix3 and Matrix4 need to be updated together so they are consistent.

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.

3 participants