processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.4k stars 3.28k forks source link

Separate Model and View Matrices. #6761

Closed deveshidwivedi closed 3 months ago

deveshidwivedi commented 7 months ago

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

davepagurek commented 7 months ago

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: https://github.com/processing/p5.js/blob/2ceca4ec656f4d137390c3554b3bd085d698ed60/src/webgl/p5.Camera.js#L1393-L1395

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 commented 7 months ago

@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 commented 7 months ago

Looks like tests are still failing because it still tries to set uMVMatrix here too: https://github.com/processing/p5.js/blob/f5baf367c2fb413ed2f1923912b185971c52e717/src/webgl/p5.Camera.js#L1394

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 commented 7 months ago

There are some different errors now @davepagurek, how could these be resolved?

deveshidwivedi commented 7 months ago

Made these changes, please have a look..

deveshidwivedi commented 6 months ago

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

deveshidwivedi commented 6 months ago

Should we update the matrix here also?

davepagurek commented 6 months ago

Should we update the matrix here also?

Ah yes good catch, I think so!

deveshidwivedi commented 5 months ago

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

davepagurek commented 4 months ago

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 commented 4 months ago

Sure @davepagurek! Looking forward!🎉

Qianqianye commented 3 months ago

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

davepagurek commented 3 months ago

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

Qianqianye commented 3 months ago

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

davepagurek commented 3 months ago

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!

davepagurek commented 3 months ago

@all-contributors please add @deveshidwivedi for code

allcontributors[bot] commented 3 months ago

@davepagurek

@deveshidwivedi already contributed before to code