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

Separate Model and View Matrices. #6761

Merged
merged 35 commits into from
May 25, 2024
Merged

Conversation

deveshidwivedi
Copy link
Contributor

Resolves #5287

Changes:

Introduced a distinct modelMatrix. By separating these matrices (modelViewMatrix), it would become easier to manipulate each aspect of the transformation independently, allowing for more flexibility especially when working with shaders.

PR Checklist

src/webgl/p5.Camera.js Outdated Show resolved Hide resolved
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.

Great work on this so far! I left some comments where it seems like the behaviour we had before wouldn't be completely the same afterwards -- normally when we reset uMVMatrix to just the camera matrix in the past, now that means we have to reset the model matrix and set the view matrix to the camera matrix.

package-lock.json Outdated Show resolved Hide resolved
src/webgl/3d_primitives.js Outdated Show resolved Hide resolved
src/webgl/interaction.js Outdated Show resolved Hide resolved
src/webgl/interaction.js Outdated Show resolved Hide resolved
src/webgl/p5.Camera.js Outdated Show resolved Hide resolved
src/webgl/p5.Matrix.js Show resolved Hide resolved
src/webgl/p5.RendererGL.js Show resolved Hide resolved
@davepagurek
Copy link
Contributor

Looks like tests are failing because:
image

It seems like it's because of this line still setting uMVMatrix rather than the model and view matrices:

p5.js/src/webgl/p5.Camera.js

Lines 1393 to 1395 in 2ceca4e

if (this._isActive()) {
this._renderer.uMVMatrix.set(this.cameraMatrix);
}

Might be worth searching in the repo for this.uMVMatrix or _renderer.uMVMatrix to make sure we aren't leaving any other lingering references to it

@deveshidwivedi
Copy link
Contributor Author

@davepagurek I made a few changes, please let me know if they make sense..
I rechecked files to see where I missed initialization but couldn't really make any improvements..
I had a doubt, do we need to introduce or update any camera properties specifically to get things working?

@davepagurek
Copy link
Contributor

Looks like tests are still failing because it still tries to set uMVMatrix here too:

this._renderer.uMVMatrix.set(this.cameraMatrix);

Maybe try searching this file to make sure it doesn't reference uMVMatrix anywhere any more?

I had a doubt, do we need to introduce or update any camera properties specifically to get things working?

I think the camera matrices are OK as is! I think your code is making the right changes already, so it's just a matter of finding any bits that we missed where we need to apply the same pattern.

@deveshidwivedi
Copy link
Contributor Author

Made these changes, please have a look..

@deveshidwivedi
Copy link
Contributor Author

Hi @davepagurek! I tried updating a few other tests, please take a look

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.

Thanks for your continued work on this! I noticed a few more things that might be throwing the tests off, let's see if those changes get the tests passing.

src/webgl/GeometryBuilder.js Show resolved Hide resolved
test/unit/webgl/p5.Camera.js Outdated Show resolved Hide resolved
test/unit/webgl/p5.Camera.js Outdated Show resolved Hide resolved
test/unit/webgl/p5.RendererGL.js Outdated Show resolved Hide resolved
test/unit/webgl/p5.Shader.js Outdated Show resolved Hide resolved
@deveshidwivedi
Copy link
Contributor Author

Should we update the matrix here also?

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.

I think we're almost done! We only have a few tests still failing, and I think the suggestions I've left will hopefully be enough to resolve them.

src/webgl/p5.RendererGL.js Outdated Show resolved Hide resolved
src/webgl/p5.Camera.js Outdated Show resolved Hide resolved
src/webgl/p5.Framebuffer.js Outdated Show resolved Hide resolved
src/webgl/p5.Shader.js Show resolved Hide resolved
src/webgl/p5.Shader.js Outdated Show resolved Hide resolved
@davepagurek
Copy link
Contributor

Should we update the matrix here also?

Ah yes good catch, I think so!

@deveshidwivedi
Copy link
Contributor Author

Hi @davepagurek! Sorry for the delay. I have made these changes and the tests do no fail anymore. Are any more updates required?

@davepagurek
Copy link
Contributor

Sorry for the delay! I think it's looking good now. We're just finishing up the p5.js website redesign (the source of the delays in my reviews 😅) and will probably be doing another quick release just to push out more documentation updates. After that, we'll be back in a more normal release schedule, including a beta release to do further testing in case we've missed anything from the tests. So I'll merge this in after that little release!

@deveshidwivedi
Copy link
Contributor Author

Sure @davepagurek! Looking forward!🎉

@davepagurek davepagurek mentioned this pull request May 22, 2024
3 tasks
@Qianqianye
Copy link
Contributor

Thank you all. @davepagurek, are we good to merge this?

@davepagurek
Copy link
Contributor

good to go if we've done our last docs deploy!

@Qianqianye
Copy link
Contributor

Just to clarify, do you mean the updating all the reference doc? @davepagurek

@davepagurek
Copy link
Contributor

right, mostly we were just waiting to put this into a release where we would be able to have a standard beta testing period to catch any bugs. if we're going to be back into a regular release cadence again then this is good to merge!

@Qianqianye Qianqianye merged commit 3c7a094 into processing:main May 25, 2024
2 checks passed
@davepagurek
Copy link
Contributor

@all-contributors please add @deveshidwivedi for code

Copy link
Contributor

@davepagurek

@deveshidwivedi already contributed before to code

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.

Separate the Model and View Matrices
4 participants