Solves issue #7059 #7113

Garima3110 · 2024-07-05T13:09:46Z

Resolves #7059

Changes:
Here's a combined sketch that demonstrates the worldToScreen function in both 2D and WebGL contexts.
https://editor.p5js.org/Garima3110/sketches/ilpG7v9Cw

Screenshots of the change:

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

Garima3110 · 2024-07-05T13:39:55Z

Please review the code , whenever some maintainer finds the time, Also if you have any idea or some sort of a suggestion, for what example code should i add for the inline documentation of worldToScreen() it would be great!
Thankyou :-)

davepagurek · 2024-07-11T19:05:40Z

Thanks for taking this on, the code looks good!

Do you think we can add a unit test for 2D and a unit test for 3D? Maybe both a simple case (e.g. no transforms have been applied) and also another that we can verify manually (like a 90 degree rotation)?
For an example, maybe you could have something like a rotating square (or cube in 3D) where we draw the screen coordinates next to each vertex?

Garima3110 · 2024-07-17T04:58:05Z

Thanks for taking this on, the code looks good!

Do you think we can add a unit test for 2D and a unit test for 3D? Maybe both a simple case (e.g. no transforms have been applied) and also another that we can verify manually (like a 90 degree rotation)?

For an example, maybe you could have something like a rotating square (or cube in 3D) where we draw the screen coordinates next to each vertex?

Yeah sure! @davepagurek
I'll try doing that by the end of this week.
Thankyou.

Garima3110 · 2024-07-20T11:11:42Z

@davepagurek Just wanted to know , I tried npm run build command for building p5 and run some tests, but for dev2.0 branch the command doesn't work. Is there a different way to build p5 for dev2.0 branch, unlike the main branch which builds p5 just with npm run build.

limzykenneth · 2024-07-20T12:25:59Z

@Garima3110 npm run build should work, what error did you get for it?

Garima3110 · 2024-07-20T12:28:56Z

@Garima3110 npm run build should work, what error did you get for it?

limzykenneth · 2024-07-21T08:58:55Z

Right I see, currently FES is still relying on the parameterData.json file for parameter validation but that file is dynamically generated by the 1.x build via grunt. @Garima3110 Can you switch back to the main branch, run the build, then switch to the dev-2.0 branch to try again? This should generate the parameterData.json file that can be packaged by the rollup build.

limzykenneth · 2024-07-26T13:59:06Z

Actually just running npm run docs in the dev-2..0 branch will also generate the parameterData file as @davepagurek have set it up to run the generation script manually.

Garima3110 · 2024-08-05T12:13:31Z

@davepagurek Do the tests seem fine ? Please have a look and let me know if some changes need to be made.
Thankyou!!!

davepagurek · 2024-08-06T14:36:06Z

test/unit/core/environment.js

+    test('worldToScreen for 3D context', function() {
+      let worldPos = myp5.createVector(0, 0, 0);
+      let screenPos = myp5.worldToScreen(worldPos);
+      assert.isTrue(screenPos.x >= 0 && screenPos.x <= 100);


Do you think we can assert exact coordinates for the 3D tests like we do for the 2D tests instead of having a range like this? e.g. 50, 50 in this case, and I think 50, 100 in the second case?

davepagurek · 2024-08-06T14:36:22Z

test/unit/core/environment.js

+    test('worldToScreen for 2D context', function() {
+      let worldPos = myp5.createVector(50, 50);
+      let screenPos = myp5.worldToScreen(worldPos);
+      assert.closeTo(screenPos.x, 50, 0.1);


This one is great! Can we add a test of rotation in 2D too?

Garima3110 · 2024-08-06T17:36:00Z

I have added more tests , and made some changes according to the suggestions please have a look!

davepagurek

Thanks for making changes, and sorry for the delay! Got sick and then recovered in the mean time, but we're back now!

davepagurek · 2024-08-18T18:28:55Z

test/unit/core/environment.js

+    });
+
+    test('worldToScreen for a rotating cube in 3D', function() {
+      myp5.push();


This one doesn't assert anything so it might not work super well as a unit test, but this would be a cool example to have in the reference for this item! Maybe we could move it into the reference comments instead? (Same for the 2D case, having those as two examples would be great!)

@limzykenneth in the 2.0 branch, is there an easy way to get a preview of how the docs will look, similar to grunt yui:dev on the main branch?

Not at the moment. The offline reference (which I also forked out here) can potentially help but it is not setup to work automatically yet.

davepagurek · 2024-08-18T18:33:15Z

test/unit/core/environment.js

+    test('worldToScreen with rotation in 2D', function() {
+      myp5.push();
+      myp5.translate(50, 50);
+      myp5.rotate(myp5.PI / 2);


I think since we translate before we rotate, the rotation doesn't end up actually affecting the position. If we rotate first, we should see an effect (I would expect (x, y) to become (y, -x) I think?)

Wait, actually, ignore this -- it's only true if you're using (0,0) as your local coordinate. I see the offset by 10 does actually switch axes here!

davepagurek · 2024-08-18T18:34:43Z

test/unit/core/environment.js

+
+    test('worldToScreen with rotation in 3D', function() {
+      myp5.push();
+      myp5.rotateY(myp5.PI / 2);


Similar comment here: just rotating won't affect the coordinates, so could we do a translation after this?

actually, ignore that, as mentioned above, it should be ok as long as you're converting a nonzero coordinate. But it feels odd that the resulting coordinate would still be 50,50 since the untransformed example above also ends up at that. should this value be something else?

actually, ignore that, as mentioned above, it should be ok as long as you're converting a nonzero coordinate. But it feels odd that the resulting coordinate would still be 50,50 since the untransformed example above also ends up at that. should this value be something else?

Maybe it should be 200,200
The default camera position in p5.js is at (0, 0, 800), looking towards the origin (0, 0, 0) along the negative Z-axis. The camera's view is centered on the Z-axis, so any point on this axis is projected to the center of the screen.
rotateY(myp5.PI / 2) rotates the point (50, 0, 0) 90 degrees around the Y-axis.
This rotation transforms the point (50, 0, 0) into (0, 0, -50). After rotation, the point is 50 units in front of the origin along the negative Z-axis. After the rotation, the point (0, 0, -50) is at a distance of 850 units from the camera (800 - (-50)). The screen coordinates are calculated based on the projection of this point into the 2D view of the camera.
Since the rotated point lies directly on the Z-axis, its X and Y screen coordinates should map to the center of the canvas, so --> 200,200

What are your thoughts on this @davepagurek please let me know?!

oh, I definitely didn't catch that this was a rotateY -- I read it as just rotate, which would have been around the z axis. That now explains why the local offset doesn't update the resulting screen x and y values. The 50,50 result now makes sense, since I think the canvas is only 100x100:

p5.js/test/unit/core/environment.js

Line 242 in 420067b

myp5.createCanvas(100, 100);

So I think this result does actually make sense, and maybe we should just add one more test of rotation about the Z axis so that we can confirm that an example like the one you use in the 2D tests also works in WebGL.

davepagurek · 2024-08-20T12:55:24Z

src/core/environment.js

+ *     createVector(-50, 50, 50)
+ *   ];
+ *
+ *   push();


How do you feel about moving this to draw() and making the rotation change slowly over time?

A great idea , will do this in a while.

davepagurek · 2024-08-26T11:30:13Z

src/core/environment.js

+ *   let rotationX = millis() / 1000;
+ *   let rotationY = millis() / 1200;
+ *
+ *   rotateX(rotationX);


If we want to make sure the text itself isn't rotated, I think we might need to wrap the rotation + worldToScreen calls in a push/pop like you've got in the 2D mode example.

davepagurek · 2024-08-26T11:31:11Z

src/core/environment.js

+ *     noStroke();
+ *     ellipse(pos.x, pos.y, 3, 3); // Draw points as small ellipses
+ *     fill(0);
+ *     text(`(${pos.x.toFixed(1)}, ${pos.y.toFixed(1)})`, pos.x, pos.y);


Are the screen coordinates relative to the top left? if so we may need to subtract width/2 and height/2 to keep these centered when drawing them in WebGL mode

davepagurek · 2024-08-26T11:31:36Z

src/core/environment.js

+ *   background(200);
+ *
+ *   screenPos.forEach((pos, i) => {
+ *     text(`(${pos.x.toFixed(1)}, ${pos.y.toFixed(1)})`, pos.x, pos.y);


Looks great!

davepagurek · 2024-09-14T13:54:30Z

Sorry for the delay on this! I recently got the tests passing on the dev-2.0 branch, so if you pull that branch in your fork and merge it into this branch again, we can double-check that we get a green checkmark from the test runner and then merge if it looks good.

…rldToScreen

Garima3110 · 2024-09-14T19:23:58Z

While I continue troubleshooting the failed tests, I'm open to any suggestions you might have, @davepagurek!

davepagurek · 2024-09-15T11:30:34Z

src/core/environment.js

+    return new p5.Vector(screenCoordinates.x, screenCoordinates.y);
+  } else {
+    // Handle WebGL context (3D)
+    const cameraCoordinates = renderer.uMVMatrix.multiplyPoint(worldPosition);


oh, maybe it's this -- we split up uMVMatrix into uModelMatrix and uViewMatrix, and only update uMVMatrix right before we draw a shape here:

p5.js/src/webgl/p5.Shader.js

Lines 579 to 584 in 4fbbaf5

_setMatrixUniforms() {

const modelMatrix = this._renderer.uModelMatrix;

const viewMatrix = this._renderer.uViewMatrix;

const projectionMatrix = this._renderer.uPMatrix;

const modelViewMatrix = (modelMatrix.copy()).mult(viewMatrix);

this._renderer.uMVMatrix = modelViewMatrix;

Maybe we can refactor the bit that sets uMVMatrix into a function on the renderer, like calculateCombinedMatrix(), which gets called in _setMatrixUniforms above, and then you can also call it here right before you make cameraCoordinates?

Garima3110 · 2024-10-17T06:29:32Z

Strange, all tests pass as I try this for the main branch but not here in the 2.0 branch.

davepagurek

I noticed a few places where it either wasn't accessing states where it should, or was accessing it where it shouldn't, we can see if it works after this. The states thing is new in the 2.0 branch so that's probably where the discrepancy is coming from, hopefully once that's fixed itll work again!

davepagurek · 2024-10-17T17:12:55Z

src/webgl/p5.Shader.js

@@ -937,7 +937,7 @@ function shader(p5, fn){
      const viewMatrix = this._renderer.states.uViewMatrix;
      const projectionMatrix = this._renderer.states.uPMatrix;
      const modelViewMatrix = (modelMatrix.copy()).mult(viewMatrix);
-      this._renderer.states.uMVMatrix = modelViewMatrix;
+      this._renderer.states.uMVMatrix = this._renderer.states.calculateCombinedMatrix();


I think this method needs to be on the renderer and not the states:

Suggested change

this._renderer.states.uMVMatrix = this._renderer.states.calculateCombinedMatrix();

this._renderer.states.uMVMatrix = this._renderer.calculateCombinedMatrix();

davepagurek · 2024-10-17T17:13:36Z

src/webgl/p5.RendererGL.js

+  // Combines the model and view matrices to get the uMVMatrix
+  // This method will be reusable wherever you need to update the combined matrix.
+  calculateCombinedMatrix() {
+    const modelMatrix = this.uModelMatrix;


I think this and the line below would need to access states now:

Suggested change

const modelMatrix = this.uModelMatrix;

const modelMatrix = this.states.uModelMatrix;

davepagurek · 2024-10-17T17:13:48Z

src/webgl/p5.RendererGL.js

+  // This method will be reusable wherever you need to update the combined matrix.
+  calculateCombinedMatrix() {
+    const modelMatrix = this.uModelMatrix;
+    const viewMatrix = this.uViewMatrix;


Suggested change

const viewMatrix = this.uViewMatrix;

const viewMatrix = this.states.uViewMatrix;

Garima3110 · 2024-10-17T19:46:28Z

Thanks for the clarification @davepagurek
I’ll work on these suggestions and get back to you soon!

davepagurek · 2024-10-20T14:17:57Z

src/core/environment.js

+        const modelViewMatrix = renderer.calculateCombinedMatrix();
+        const cameraCoordinates = modelViewMatrix.multiplyPoint(worldPosition);
+        const normalizedDeviceCoordinates =
+          renderer.uPMatrix.multiplyAndNormalizePoint(cameraCoordinates);


Looks like this one is causing the test failures now:

I think this one should be in states too:

Suggested change

renderer.uPMatrix.multiplyAndNormalizePoint(cameraCoordinates);

renderer.states.uPMatrix.multiplyAndNormalizePoint(cameraCoordinates);

Garima3110 · 2024-10-20T19:10:53Z

@davepagurek I have made some changes in the doc examples too, everything seems to work fine, please let me know if any other change needs to be done.
Thankyou!

davepagurek

Sorry this took a while to review! In the process I finally figured out a way to preview the docs live in the p5.js-website repo 😅

Garima3110 added 2 commits July 5, 2024 18:36

Solves issue processing#7059

260fd59

Fixed linting

4366334

Qianqianye requested review from davepagurek and limzykenneth July 5, 2024 22:54

Added unit tests

eeed9c9

davepagurek reviewed Aug 6, 2024

View reviewed changes

Added more tests+ some changes

420067b

Qianqianye requested a review from davepagurek August 16, 2024 23:09

davepagurek reviewed Aug 18, 2024

View reviewed changes

Minor changes with unit tests + examples added

d7fc17d

davepagurek reviewed Aug 20, 2024

View reviewed changes

Changes in tests and doc examples

c10c423

davepagurek reviewed Aug 26, 2024

View reviewed changes

minor changes in example

09343bf

Garima3110 added 4 commits September 14, 2024 23:09

Merge branch 'dev-2.0' of https://github.com/processing/p5.js into wo…

54547c3

…rldToScreen

Minor fixes to pass tests

c2e87bd

Handled edge cases in worldToScreen to avoid Infinity values

b7436b8

Minor fixes

6e9f46c

davepagurek reviewed Sep 15, 2024

View reviewed changes

Garima3110 and others added 3 commits September 15, 2024 19:55

Added calculateCombinedMatrix()

d8a87a0

Fixes

bd4a5a3

Merge branch 'dev-2.0' into worldToScreen

8163082

davepagurek reviewed Oct 17, 2024

View reviewed changes

Fix

a0ce81d

davepagurek reviewed Oct 20, 2024

View reviewed changes

Garima3110 added 3 commits October 20, 2024 20:59

Minor change

de3c777

Test fix

85244c0

Docs improved

23accfe

davepagurek mentioned this pull request Oct 31, 2024

Automatically keep track of transformations #7342

Open

17 tasks

davepagurek approved these changes Nov 2, 2024

View reviewed changes

davepagurek merged commit 68ae795 into processing:dev-2.0 Nov 2, 2024
2 checks passed

Garima3110 deleted the worldToScreen branch November 2, 2024 07:01

Garima3110 mentioned this pull request Nov 2, 2024

Suggestion of a function to get screen coordinates #7059

Closed

17 tasks

	_setMatrixUniforms() {
	const modelMatrix = this._renderer.uModelMatrix;
	const viewMatrix = this._renderer.uViewMatrix;
	const projectionMatrix = this._renderer.uPMatrix;
	const modelViewMatrix = (modelMatrix.copy()).mult(viewMatrix);
	this._renderer.uMVMatrix = modelViewMatrix;

	this._renderer.states.uMVMatrix = this._renderer.states.calculateCombinedMatrix();
	this._renderer.states.uMVMatrix = this._renderer.calculateCombinedMatrix();

	const modelMatrix = this.uModelMatrix;
	const modelMatrix = this.states.uModelMatrix;

	const viewMatrix = this.uViewMatrix;
	const viewMatrix = this.states.uViewMatrix;

	renderer.uPMatrix.multiplyAndNormalizePoint(cameraCoordinates);
	renderer.states.uPMatrix.multiplyAndNormalizePoint(cameraCoordinates);

Solves issue #7059 #7113

Solves issue #7059 #7113

Conversation

Garima3110 commented Jul 5, 2024 • edited Loading

PR Checklist

Garima3110 commented Jul 5, 2024

davepagurek commented Jul 11, 2024

Garima3110 commented Jul 17, 2024

Garima3110 commented Jul 20, 2024

limzykenneth commented Jul 20, 2024

Garima3110 commented Jul 20, 2024

limzykenneth commented Jul 21, 2024

limzykenneth commented Jul 26, 2024

Garima3110 commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garima3110 commented Aug 6, 2024

davepagurek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garima3110 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garima3110 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek commented Sep 14, 2024

Garima3110 commented Sep 14, 2024

Choose a reason for hiding this comment

Garima3110 commented Oct 17, 2024

davepagurek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garima3110 commented Oct 17, 2024

Choose a reason for hiding this comment

Garima3110 commented Oct 20, 2024

davepagurek left a comment

Choose a reason for hiding this comment

Garima3110 commented Jul 5, 2024 •

edited

Loading

Garima3110 Aug 20, 2024 •

edited

Loading

Garima3110 Aug 20, 2024 •

edited

Loading

davepagurek left a comment •

edited

Loading