-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix writable mat4/mat3 setters for p5.Matrix to restore Camera.slerp() in ortho mode #8295
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
base: dev-2.0
Are you sure you want to change the base?
Conversation
|
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
a30bb99 to
d1dcd64
Compare
|
Rebasing this PR onto the latest dev-2.0. |
|
Hi @nakednous, sorry for the delay on this! I think this approach works, but it might be for the best to not allow these to be settable for safety? @holomorfo @limzykenneth do you have thoughts? If we don't want to let those properties be settable, rather than cloning + updating the existing matrix here: Lines 1593 to 1594 in a6e0b46
...maybe we just set the state directly to a clone of the camera's matrix? |
…r in ortho mode (fixes processing#7837)
d1dcd64 to
ce3044c
Compare
|
I double-checked this locally and can confirm both variants remove the ortho if (this._isActive()) {
this._renderer.states.setValue('uPMatrix', this._renderer.states.uPMatrix.clone());
}and if (this._isActive()) {
this._renderer.states.setValue('uPMatrix', this._renderer.states.uPMatrix.clone());
const dst = this._renderer.states.uPMatrix.mat4;
const src = this.projMatrix.mat4;
for (let i = 0; i < dst.length; i++) {
dst[i] = src[i];
}
}In the minimal test sketch I used, the ortho projection parameters of Happy to go with whichever direction you prefer. |
|
Thanks for taking a look! Have you tried if something like this works? this._renderer.states.setValue('uPMatrix', this.projMatrix.clone()); This would be directly creating a clone of the intended matrix rather than cloning the renderer's matrix and updating it, since the result in both cases is a new matrix with the intended values. |
This PR fixes issue #7837.
In p5.js v2,
Matrix.mat4is exposed via a getter that returns an internalFloat32Array.Camera.slerp()(and potentially other code) assigns touPMatrix.mat4which throws:TypeError: setting getter-only property "mat4"This change adds a
set mat4(values)onMatrixthat:Float32Arrayinstead of reassigning it.set mat3(values)for 3×3 matrices.This fixes the intended writable behavior (used by
Camera.slerp()), while preserving the internal buffer identity required by WebGL bindings and other code that holds references to the matrix storage.