Skip to content
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

[p5.js 2.0] Vector nD getter and setter, added values. #7277

Open
wants to merge 23 commits into
base: dev-2.0
Choose a base branch
from

Conversation

holomorfo
Copy link

@holomorfo holomorfo commented Sep 20, 2024

Resolves ##6765

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

  • npm run lint passes
  • Inline documentation is included / updated
  • 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

@holomorfo holomorfo changed the title [p5.js 2.0](IN_PROGRESS) Vector getter and setter, added values. [p5.js 2.0](IN_PROGRESS) Vector nD getter and setter, added values. Sep 20, 2024
@holomorfo holomorfo self-assigned this Sep 20, 2024
this._values = newValues.slice()
}

get x() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this allows us to still consume the vector x, y and z without having an explicit class attributes with that name, this making it more flexible
@limzykenneth

get y() {
return this._values[1] || 0
}
get z() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a w value for 4D? or for nD do users will just search values by number?
@limzykenneth

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok adding a w personally, since math for 3D stuff uses 4D vectors/matrices quite often. it seems fairly common in other apis too, e.g. native js DOMPoint. And then anything above 4D would use indices

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, adding a w is fine for me as well. When working on the documentation we need to balance this convention of using w and it potentially being unintuitive for those very new to linear algebra, possibly by de-emphaiszing w in some way or other solution.

return this._values[0] || 0
}

setValue(index, value) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably I need to create also a get value(index) so users can get entries of the vector by ID
@limzykenneth

@@ -158,7 +207,7 @@ function vector(p5, fn) {
* </div>
*/
toString() {
return `p5.Vector Object : [${this.x}, ${this.y}, ${this.z}]`;
return `p5.Vector Object : [${this.values.join(', ')}]`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the things I'm considering is to have this be serialized into something more standard/machine readable, so maybe just the JSON part at the end.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean something like this?

    toString() {
      return `${this.values.join(', ')}]`;
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah perhaps, might be worth considering how other libraries serialize their vector objects if they do at all

@holomorfo
Copy link
Author

holomorfo commented Sep 24, 2024

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

  • constructor
  • § (get) values - DONE
  • § (set) values -> Check dimentions? YES
  • (get) x
  • getValue
  • setValue
  • § (get) y
  • (get) z
  • (get) w
  • § (set) x
  • (set) y
  • § (set) z
  • § (set) w
  • toString
    Comment from Ken regarding serializtion DONE
  • set
    Need to set dimension also? Yes, DONE
  • сору
  • add
  • rem
    Only 3D for now
  • sub
  • mult
  • 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
  • mag
  • magSq
  • dot
  • cross Needs determinant Only 2D for now
  • dist
  • normalize
  • limit
  • setMag
    why is it call setMag?
    maybe because it does it in place
  • heading
    Check _fromRadians
  • setHeading
    Only for 2D
  • rotate
    Only for 2D
  • angleBetween needs cross to be nD only 3D for now
  • lerp Problem with inference of last value being amount for ND Only 3d for Now
  • slerp Problem with inference of last value being amount for ND Only 3d for Now
  • reflect
  • array
    In theory this should return only this.values however, 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
  • equals
  • clampToZero
  • _clampToZero

Static methods

  • fromAngle - only 2D
  • fromAngles - only 3D
  • random2D
  • random3D
  • сору
  • add
  • rem
    Needs module ND Only 3D for now
  • sub
  • mult
  • rotate
    needs rotate Nd Only 3D for now
  • div
    See comments in the non static method.
  • dot
  • cross - Only 3D for now
  • dist
  • lerp - Only 3D for now
  • slerp - Only 3D for now
  • mag
  • magSq
  • normalize
  • limit
  • setMag
  • heading
  • angleBetween - Only 3D for now
  • reflect
  • array - Only 3D for now, see above
  • equals
    to
    @davepagurek @limzykenneth @Qianqianye

@limzykenneth
Copy link
Member

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.

@holomorfo
Copy link
Author

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() 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.

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

@holomorfo
Copy link
Author

I have added the required test for my changes, I increased the cover for the Vector file from:
Screenshot 2024-09-26 at 10 16 01 PM
To
Screenshot 2024-09-26 at 10 17 48 PM

More tests should and will be added to make sure the nD versions of the functions are correct.

CC: @limzykenneth @davepagurek @Qianqianye

@holomorfo holomorfo changed the title [p5.js 2.0](IN_PROGRESS) Vector nD getter and setter, added values. [p5.js 2.0](READY) Vector nD getter and setter, added values. Sep 27, 2024
@holomorfo holomorfo marked this pull request as ready for review September 27, 2024 05:36
@holomorfo
Copy link
Author

This PR is ready for review.
@limzykenneth @Qianqianye @davepagurek

@limzykenneth
Copy link
Member

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.

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! I just flagged some bits that might be inconsistent in case it's unintentional.

}
if (dimensions === 0) {
this.dimensions = 2;
this._values = [0, 0, 0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the dimensions here match the number of items in _values?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm just now thinking about, should the default vector when no argument is passed be 2D instead of 3D? Seeing as most people start learning to create sketches with 2D before moving into 3D.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense. Even in webgl mode, you may very well still be using 2D vectors for a number of things, so its not a bad idea to have to be explicit to make 3D vectors

src/math/p5.Vector.js Outdated Show resolved Hide resolved
@holomorfo holomorfo changed the title [p5.js 2.0](READY) Vector nD getter and setter, added values. [p5.js 2.0] Vector nD getter and setter, added values. Oct 30, 2024
@holomorfo
Copy link
Author

holomorfo commented Nov 5, 2024

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

@holomorfo
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants