Added a couple more vectormath operations #35
Conversation
Incoherent-Code
requested review from
rlandav and
JakeShirley
as code owners
| /** | ||
| * multiply | ||
| * | ||
| * Multiplies two vectors together |
There was a problem hiding this comment.
Can you specify here (and the documentation elsewhere for this) that this is a element-wise multiplication? Just want to make sure it's not confused with dot or cross from the intellisense.
| * rotateX | ||
| * | ||
| * Rotates the vector around the x axis | ||
| * @param a Angle in radians |
There was a problem hiding this comment.
I ran the pipeline for the PR and you have a few lint errors to fix. Specifically, you need params with a dash after the parameter name. If you run npm run lint locally you'll get the full errors to fix (including the need to run prettier which you can do with the --fix option).
There was a problem hiding this comment.
Sorry about that
npm run lint fails on my machine on the prettier stage.
Error: Command failed: C:\Program Files\nodejs\node.exe C:\Users\aiaub\Documents\VSCode\minecraft-scripting-libraries\libraries\math\node_modules\prettier\bin-prettier.js --check src/**/*.ts
I was hoping the vscode extension would catch everything, but I'll try to figure out what's wrong.
There was a problem hiding this comment.
If you run npm run lint -- -- --fix, it will fix it for you automatically. VSCode should have though 🤷 . Also, the number of extra -- may vary depending on your terminal.
| static rotateX(v: Vector3, a: number): Vector3 { | ||
| let cos = Math.cos(a); | ||
| let sin = Math.sin(a); | ||
| return { | ||
| x: v.x, | ||
| y: v.y * cos + v.z * sin, | ||
| z: v.z * cos - v.y * sin | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a specific reason that you chose to do the rotation clockwise (i.e. left hand rule) rather than the standard right hand rule?
Preference would be to make all of these helpers adhere to the standard rotation matrix using right hand rule (counter clockwise rotation), i.e. for a 1/2 radian rotation of the unitY vector, it would become a positive unitZ vector for a X rotation rather than a negative unitZ vector
There was a problem hiding this comment.
I did it clockwise because the minecraft java function under the same name (yarn mappings) that I was familiar with also rotates clockwise. idk why they did it that way. I can make it counterclockwise if that's more standard.
There was a problem hiding this comment.
I believe there is a bit of history there, but since this helpers will operate purely in javascript land, my preference is to have the rotations follow the standard right hand rule rotation matrix.
There was a problem hiding this comment.
Can you group the rotation functions into a describe block since they are related? Also, can you add tests operating off unit vectors too, so its clear at a glance the rotation direction?
|
This last commit should fix all the aforementioned issues |
I added a couple more vector math functions.
multiply([vector], [vector])multiplies 2 vectors together.rotateX([vector], [angle])rotates the vector around the x axis.rotateY([vector], [angle])rotates the vector around the y axis.rotateZ([vector], [angle])rotates the vector around the z axis.I wrote tests and ran beachball already. Let me know if you have any concerns.