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 n-dimentional and Matrix Interface #7405

Merged
merged 9 commits into from
Dec 17, 2024

Conversation

holomorfo
Copy link

@holomorfo holomorfo commented Dec 4, 2024

Resolves #6765
Related conversation on closed PR #7277

@davepagurek @limzykenneth @ksen0

Summary of Changes

This pull request introduces significant improvements to matrix handling in p5.js, enabling more generalized and flexible n-dimensional matrix operations.

Generalized Matrix Class

  • Introduced a unified Matrix class to replace mat3 and mat4, supporting matrices of any dimensionality.
  • Abstracted and modularized matrix operations to facilitate higher-dimensional use cases.

New MatrixInterface File

  • Encapsulates common matrix operations such as addition, multiplication, and transformations.
  • Provides a standardized interface for consistent matrix handling across the library.

Enhanced Functionality

  • Added new methods, including add, for manipulating matrices in n-dimensional space.
  • Updated internal math utilities to support more dynamic and flexible matrix calculations.

Integration with p5.js

  • Ensured compatibility with WebGL renderers while abstracting dimension-specific logic.
  • Refactored existing code to remove hardcoded references to mat3 and mat4, enabling scalable matrix support.

These changes establish a robust foundation for advanced matrix operations in p5.js and improve its versatility for mathematical and graphical applications.
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 Class Changes

Support for N-Dimensional Vectors
  • Expanded the vector class to support n-dimensional vectors, enabling more versatile mathematical operations.
  • Updated internal logic to make vector operations dimension-agnostic.
New Features
  • Introduced additional methods for advanced vector manipulation, including support for n-dimensional addition, subtraction, and dot product.
  • Enhanced normalization and magnitude calculations for vectors of arbitrary dimensions.
Code Refactoring
  • Streamlined the vector class for better performance and readability.
  • Improved compatibility with the updated matrix framework to handle transformations for vectors in n-dimensional space.

Vector methods details

Some vector methods remain 4 or 3 dimensional to retain compatibility with the existing api.

  • constructor
  • § (get) values - DONE
  • § (set) values -> Check dimensions? YES
  • (get) x
  • getValue
  • setValue
  • § (get) y
  • (get) z
  • (get) w
  • § (set) x
  • (set) y
  • § (set) z
  • § (set) w
  • toString
    Comment from Ken regarding serialization DONE Pending more general serialization.
  • 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

PR Checklist

@holomorfo
Copy link
Author

@limzykenneth @davepagurek my previous PRs were very outdated and getting lots of conflicts with the recent refactors, so I created a clean one updated with my latest changes, I'm also experimenting with an abstract class implementation for the Matrix so we can define exactly what methods will be public for the user, and what are internal as to not cause confusion, like the references to mat3 and mat4 even more, I'm trying to abstract everything so these mat3 and mat4 are only getters in an abstract data structure and will only return value if the matrix itself is of dimension 3x3 or 4x4.

@holomorfo holomorfo marked this pull request as ready for review December 4, 2024 06:05
@holomorfo
Copy link
Author

I only have one test error that I can't figure out when doing the abstraction:

It might be because the new Matrix abstraction has no access to p5, but I don't see why the 'connect' method would be related.

 FAIL |unit|  test/unit/core/param_errors.js > Validate Params > validateParams: web API objects > p5.MediaElement.connect(): no friendly-err-msg
TypeError: Cannot read properties of undefined (reading 'connect')
 ❯ fn.generateZodSchemasForFunc src/core/friendly_errors/param_validator.js:133:38
    131|
    132|     const { funcName, funcClass } = extractFuncNameAndClass(func);
    133|     let funcInfo = dataDoc[funcClass][funcName];
       |                                      ^
    134|
    135|     let overloads = [];

CC: @davepagurek @limzykenneth

@limzykenneth
Copy link
Member

@holomorfo It cannot read connect because it cannot find p5.MediaElement since there is no p5. I can fix this, will have a look.

@limzykenneth
Copy link
Member

Test seems to pass on CI still so perhaps it's not a problem at this stage.

@holomorfo
Copy link
Author

holomorfo commented Dec 12, 2024

@davepagurek @limzykenneth
I have removed the references to mat3 and mat4 and everything is done now with this.matrix also the multiplication is N dimentional, I retained the special cases o element wise multiplication of mat4 and mat4 only to preserve the gl-matrix optimization of not doing any loops, but now that we have benchmark we can actually test if its faster or not.

I also simplified the API (I believe its simpler, let me know) in the sense that we don't need to have an extra static for identity, I believe is simpler if the user specifies new Matrix(5) and this gives them a 5x5 matrix, or whatever integer. As a personal opinion I don't like that the user can have both ways to enter a matrix new Matrix([1,2,3]) and new Matrix(1,2,3) I think we should favor the array notation if possible, this can be reverted easily if needed, please let me know your thoughts.

Also now we don't need to specify 'mat3' or 'mat4' in the constructor, we can just specifiy the desired dimention, this is great for generalization.

In the consuming components like renderer, camera or geometry I have kept access to mat3 and mat4 only as a getter.

@limzykenneth
Copy link
Member

@holomorfo Is this ready for merge? I can fix the conflicts so don't worry about that, just that all your work has been pushed here. Also do we still need #7277?

@holomorfo
Copy link
Author

Hi Ken, yes, this PR is ready for merging, I have added a tentative inline documentation, we can continue working on that. Also I have closed #7277 since all changes are here now. I have fixed the test and all pass in local also, even the parameters one that was failing before.
@limzykenneth @davepagurek

@limzykenneth limzykenneth merged commit b89ac5b into processing:dev-2.0 Dec 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants