-
Notifications
You must be signed in to change notification settings - Fork 88
feat: Add translateByVector2 method to Matrix4 #349
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
Conversation
| for (var i = 0; i < inputA.length; i++) { | ||
| relativeTest(output1[i], output2[i]); | ||
| relativeTest(outputA[i], outputB[i]); | ||
| } |
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.
note: I shifted the variable names a bit here to make it match some sort of logic with the new names, but please lmk if there is a preferred naming convention.
tbh this test feels like two tests in one but I don't want to change the structure too much.
|
@kevmoo could we have a review of this one? :) |
|
@luanpotter |
kevmoo
left a comment
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.
Please update the changelog! Also the version should (now) be 2.3.0-wip since we're adding a feature!
|
@kevmoo added - I can update the other PR after this is merged so I can rebase the changelogs 🙏 |
|
actually I think I misunderstood how the changelog works at first - I think it is as intended now |
Before the deprecation of the dynamic
translatemethod, since thezvariable had default value of0, you could translate by a vector two by doing:Here is an example "in the wild" of such usage.
With the new methods, this becomes even less ergonomic, as it would require either calling
translateByDoublewith 2 extra parameters, or converting theVector2to aVector3.So I propose we introduce a
translateByVector2which behaves exactly astranslate(x, y)would before.