-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Affine transformation helpers functions for Matrix
classes
#10124
Conversation
Thanks for the pull request @IanLilleyT!
Reviewers, don't forget to make sure that:
|
015aec3
to
e0df525
Compare
e0df525
to
674f059
Compare
Thanks @IanLilleyT! @ebogo1 can you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just wanted to double check that the inline math in Matrix2 and Matrix3 was what you intended.
const scaleRatioX = scale.x / existingScale.x; | ||
const scaleRatioY = scale.y / existingScale.y; | ||
|
||
result[0] = matrix[0] * scaleRatioX; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just call multiplyByScale
like Matrix4 does? Unless this is what you meant by inlining more of the math.
|
||
const scale = Matrix2.getScale(matrix, scaleScratch4); | ||
|
||
result[0] = rotation[0] * scale.x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - there are a couple other places in this file so I'll stop commenting on each one. Maybe the uniformScale
function doesn't need it because it's not using a scratch vec2.
Yep the inline math is intended and should give slightly better performance and IMO doesn't hurt readability. There are some areas in this PR where the function was already implemented but I changed it to be inline math for consistency. |
This PR fills in some gaps in
Matrix2
,Matrix3
, andMatrix4
for affine transformations. This includes:setScale
- forMatrix2
,Matrix3
Matrix4
alreadysetUniformScale
- forMatrix2
,Matrix3
,Matrix4
multiplyByUniformScale
- forMatrix2
,Matrix3
Matrix4
alreadysetRotation
- forMatrix2
,Matrix3
,Matrix4
getRotation
- forMatrix2
,Matrix4
Matrix3
alreadyfromRotation
- forMatrix4
Matrix2
andMatrix3
already have their own versions of thisAlso did some cleanup in the specs so that all tests are grouped by:
Misc: