p5.js
p5.js copied to clipboard
processing
[p5.js 2.0](READY) Vector nD getter and setter, added values.
Resolves ##6765 (Partially)
Changes:
Modify Vector class to include a values list to make it n-dimensional while mantaining the existing API of x, y, z by setting up accessors, getter and setter.
Pending Update documentation and add new Unit tests
Screenshots of the change:
PR Checklist
- [X]
npm run lintpasses - [x] Inline documentation is included / updated
- [x] Unit tests are included / updated (Didn't modify any existing test, added new tests to increase coverage of the Vector file from 94.59% to 97.12%)
CC: @limzykenneth @davepagurek @Qianqianye
I have added new inline documentation for the new methods. Here I share the analysis of the pending methods to update and some questions/challenges I'm facing:
Vector methods
- [x] constructor
- [x] § (get) values - DONE
- [x] § (set) values -> ~~Check dimentions?~~ YES
- [x] (get) x
- [x] getValue
- [x] setValue
- [x] § (get) y
- [x] (get) z
- [x] (get) w
- [x] § (set) x
- [x] (set) y
- [x] § (set) z
- [x] § (set) w
- [x] toString ~~Comment from Ken regarding serializtion~~ DONE
- [x] set ~~Need to set dimension also?~~ Yes, DONE
- [x] сору
- [x] add
- [x] rem Only 3D for now
- [x] sub
- [x] mult
- [x] div
Updated working for scalar, vector and array, question about different dimension vectors, how should it be handled? should we only divide what is possible and leave the rest unchanged? For example, what to do in this case
[1,2,3]/[8,9]should it be:[1/8,2/9,3]or in the other case[8,9]/[1,2,3] =? [8/1, 9/2]? I've not seen this implementation or inference in any other mathematics library or in general. @limzykenneth @davepagurek - [x] mag
- [x] magSq
- [x] dot
- [x] cross ~~Needs determinant~~ Only 2D for now
- [x] dist
- [x] normalize
- [x] limit
- [x] setMag why is it call setMag? maybe because it does it in place
- [x] heading
Check
_fromRadians - [x] setHeading Only for 2D
- [x] rotate Only for 2D
- [x] angleBetween ~~needs
crossto be nD~~ only 3D for now - [x] lerp ~~Problem with inference of last value being amount for ND~~ Only 3d for Now
- [x] slerp ~~Problem with inference of last value being amount for ND~~ Only 3d for Now
- [x] reflect
- [x] array
In theory this should return only
this.valueshowever, this breaks some functionality on the test " p5.RendererGL > color interpolation > geometry with stroke colors use their colors" because of the default always returning a 3 elements array, will retain ad 3D for now but will re-visit - [x] equals
- [x] clampToZero
- [x]
_clampToZero
Static methods
- [x] fromAngle - only 2D
- [x] fromAngles - only 3D
- [x] random2D
- [x] random3D
- [x] сору
- [x] add
- [x] rem ~~Needs module ND~~ Only 3D for now
- [x] sub
- [x] mult
- [x] rotate ~~needs rotate Nd~~ Only 3D for now
- [x] div See comments in the non static method.
- [x] dot
- [x] cross - Only 3D for now
- [x] dist
- [x] lerp - Only 3D for now
- [x] slerp - Only 3D for now
- [x] mag
- [x] magSq
- [x] normalize
- [x] limit
- [x] setMag
- [x] heading
- [x] angleBetween - Only 3D for now
- [x] reflect
- [x] array - Only 3D for now, see above
- [x] equals to @davepagurek @limzykenneth @Qianqianye
For the inline documentation, it is not great at inferring the right name and parent class/module with the code only so we may need to add additional hints around these, you can have a look at the other already documented entries and follow their convention. We can revisit the documentation at a later date though so no need to solve this immediately.
For set() and (set) values, is the idea here that they can be used to change the dimension of the vector or would the dimension of a vector stay the same throughout its lifetime?
For setMag() and others, the name basically all come from Processing and we'll keep them as is for compatibility reasons.
setHeading() and rotate() currently is documented to work only in 2D. For 3D there probably need to be an axis defined or things like xyz/quartenion values, for 4D I'm not sure what it would mean. I think it can be fine to make them 2D only methods but if you have specific ideas or other libraries handle these in some way, do put something together for consideration.
For the inline documentation, it is not great at inferring the right name and parent class/module with the code only so we may need to add additional hints around these, you can have a look at the other already documented entries and follow their convention. We can revisit the documentation at a later date though so no need to solve this immediately.
Sounds good, will do my best now and will revisit the docs later
For
set()and(set) values, is the idea here that they can be used to change the dimension of the vector or would the dimension of a vector stay the same throughout its lifetime?
Actually this is my exact question that I'm facing with operations that have multiple dimensions like div and mult, I have added some comments to the PR, I believe that dimension needs to change also in the setters when the values are changed, increased or decrease in components
For
setMag()and others, the name basically all come from Processing and we'll keep them as is for compatibility reasons.
Understood, thanks
setHeading()androtate()currently is documented to work only in 2D. For 3D there probably need to be an axis defined or things like xyz/quartenion values, for 4D I'm not sure what it would mean. I think it can be fine to make them 2D only methods but if you have specific ideas or other libraries handle these in some way, do put something together for consideration.
Sounds right, I will indicate explicitly what functions will stay in fixed dimentions, I will think also of possible generalisations for nD for the defined axis
I have added the required test for my changes, I increased the cover for the Vector file from:
To
More tests should and will be added to make sure the nD versions of the functions are correct.
CC: @limzykenneth @davepagurek @Qianqianye
I'd like to test the documentation generation first to see how it works, will get to it when I have a bit more time, you can go ahead with next steps branching off of this branch.
I have added my proposal for the adapter for toggling between MatrixLegacy which is what we currently have that is based on the gl-matrix implementation and MatrixNumJs classs.
To toggle between this I currently setup a static constant
export const MATRIX_ENGINE = 'legacy';
export const MATRIX_ENGINE = 'numjs';
All existing tests pass with both versions of the matrix, please let me know your thoughts @limzykenneth @davepagurek. CC: @Qianqianye
Since we might have two bundles of p5.js with different matrix engines, maybe we should do a configuration to export differently for example p5.numjs.js or something similar, and on build time pass a specific flag.
I wanted to ask what do you suggest can be a next step to integrate these changes
@holomorfo I can work on the build a bit later, for now we default to the older glMatrix implementation and keep the numjs class around.