Solves issue #6519 #6549

Garima3110 · 2023-11-11T08:33:49Z

Solves issue #6519
Changes:
I have tried to incorporate all the changes suggested by @davepagurek . Please let me know if something else needs to be changed. Thank You..!
Link to the changes made: src/webgl/p5.Camera.js

Screenshots of the change:

Change in translate():

Documentation update:

PR Checklist

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

davepagurek · 2023-11-14T23:13:54Z

src/webgl/p5.Camera.js

+ *
+ * If you prefer a fixed field of view, follow these steps:
+ *
+ * 1.Choose your desired field of view angle (`fovy`). This is how wide the camera can see.


Right now these are getting rendered as paragraphs. I think we want to make these appear as a proper list:

To do so, I believe we need a space after the . after each number, and I think we have to take out the blank lines between each list item.

davepagurek · 2023-11-14T23:16:44Z

src/webgl/p5.Camera.js

+ *
+ * 1.Choose your desired field of view angle (`fovy`). This is how wide the camera can see.
+ *
+ * 2.To position the camera correctly, use the formula:


Rather than saying "position the camera correctly", maybe we could phrase this like:

To ensure that you can see the entire width across horizontally and height across vertically, place the camera a distance of `(height / 2) / tan(fovy / 2)` back from its subject.

Note that I also put backticks around the math bit, since that refers to code.

davepagurek · 2023-11-14T23:18:29Z

src/webgl/p5.Camera.js

+ *   cameraDistance = (height / 2) / tan(fovy / 2);
+ *   This ensures that you can see the entire width across horizontally and height across vertically at the fixed field of view.
+ *
+ * 3.Set the near value to  cameraDistance / 10 and the far value to  cameraDistance * 10 .


I think we can probably omit this step, since there actually isn't a variable to set here, we'll just pass it right into perspective.

davepagurek · 2023-11-14T23:19:19Z

src/webgl/p5.Camera.js

+ *
+ * 3.Set the near value to  cameraDistance / 10 and the far value to  cameraDistance * 10 .
+ *
+ * 4.Simply, call perspective with the chosen field of view, canvas aspect ratio, and near/far values:


Super tiny tweak, but can we take out "simply" at the start? We generally try to keep things instructional without passing judgment on how easy or hard things are.

davepagurek · 2023-11-14T23:19:42Z

src/webgl/p5.Camera.js

+ * 3.Set the near value to  cameraDistance / 10 and the far value to  cameraDistance * 10 .
+ *
+ * 4.Simply, call perspective with the chosen field of view, canvas aspect ratio, and near/far values:
+ *    perspective(fovy, width / height, cameraDistance / 10, cameraDistance * 10);


We can put backticks around this too to format it like code:

Suggested change

* perspective(fovy, width / height, cameraDistance / 10, cameraDistance * 10);

* `perspective(fovy, width / height, cameraDistance / 10, cameraDistance * 10);`

davepagurek · 2023-11-14T23:25:44Z

src/webgl/p5.Camera.js

- * where eyeZ is equal to ((height/2) / tan(PI/6)).
+ * If no parameters are given, the default values are used as:
+ *
+ * fov- The default field of view for the camera is such that the full height of renderer is visible when it is positioned at a default distance of 800 units from the camera.


I think we can maybe improve the readability of this list, perhaps by turning it into a bullet list?

To do so, we need to put a - before each item, and take out the empty lines between them. I also think it might flow better once in a bullet list if we use a colon afterwards instead of a dash. Lastly, since each of these is a variable name, we can put them in backticks:

davepagurek · 2023-11-14T23:27:06Z

src/webgl/p5.Camera.js

+ *
+ * aspect- The default aspect ratio is the ratio of renderer's width to renderer's height.
+ *
+ * near - The default value for the near clipping plane is 0.1 times the default distance from the camera to the point it is looking at i.e 800.


Suggested change

* near - The default value for the near clipping plane is 0.1 times the default distance from the camera to the point it is looking at i.e 800.

* near - The default value for the near clipping plane is 80, which is 0.1 times the default distance from the camera to its subject.

davepagurek · 2023-11-14T23:27:24Z

src/webgl/p5.Camera.js

+ *
+ * near - The default value for the near clipping plane is 0.1 times the default distance from the camera to the point it is looking at i.e 800.
+ *
+ * far - The default value for the far clipping plane is 10 times the default distance from the camera to the point it is looking at i.e 800.


Suggested change

* far - The default value for the far clipping plane is 10 times the default distance from the camera to the point it is looking at i.e 800.

* far - The default value for the far clipping plane is 8000, which is 10 times the default distance from the camera to its subject.

davepagurek · 2023-11-14T23:29:01Z

src/webgl/p5.Camera.js

@@ -156,7 +176,7 @@ p5.prototype.camera = function (...args) {
 *
 *   rotateX(-0.3);
 *   rotateY(-0.2);
- *   translate(0, 0, -50);
+ *   translate(0, 0, -100);


We might need to play around with this some more. It looks like it's getting clipped the other way now:

Screen.Recording.2023-11-14.at.6.27.53.PM.mov

Garima3110 · 2023-11-15T05:45:23Z

Thanks @davepagurek for these suggestions ..! I'll just look into these and get back to u soon.

Garima3110 · 2023-11-15T15:07:44Z

@davepagurek I have tried to implement what you have suggested , But I am not sure about the translate distance setting to 85 in the negative z direction.

Since when I run grunt yui:dev to see the local docs change, the canvas shows up nothing even with a prior distance of -50 . Is this something which is local to my device only?

whereas on the original P5 reference, the cubes are visible :

Just not getting the reason of this difference.!

davepagurek · 2023-11-17T15:38:10Z

I think the difference is that now, the camera is a fixed distance back (800), it was likely closer to the objects before. How do you these settings look to you:

Translating closer, and translating before rotating to keep things in frame:

  translate(0, 0, 500);
  rotateX(-0.3);
  rotateY(-0.2);

Garima3110 · 2023-11-17T17:53:27Z

How do you these settings look to you:

Translating closer, and translating before rotating to keep things in frame:

Great! Thanks @davepagurek ..!! This absolutely works fine. Also, I would like to add that similar is the case for ortho also , should I replicate the same thing to that also and then commit the changes.

Without adding translate to 500:

After adding translate to 500

davepagurek · 2023-11-17T18:10:32Z

That would be great if you could fix those too, thanks!

Garima3110 · 2023-11-18T11:36:46Z

Well after going through the camera methods written in camera.js file, I observed this issue with so many methods .

Rather than changing the existing code for the examples for individual methods, I think we should change the default camera settings which are causing this issue in in the local development environment.

What's your call on this @davepagurek ??

davepagurek · 2023-11-21T13:53:33Z

I think the change that did this was the exact change that this PR is improving the documentation for, changing the camera behaviour to have a position that is independent of the field of view. Previously, the camera would move itself forward or backward when changing the window size in order to keep a fixed field of view, but that caused issues when using a custom camera, as it is unclear what should happen to cameras that have been manually positioned. Now that the camera positioning works a bit differently, it makes sense that these examples need to update their camera code a bit.

The alternate approach, where the behaviour would be completely consistent for the examples, we'd likely have to undo #6216, and rather than use a fixed field of view, move the camera closer to/farther from its subject as the window resizes in order to still fix the bugs related to the camera resetting. Those changes are also nontrivial, so I think it's ok in this case to just update the camera examples.

davepagurek · 2023-11-23T18:07:36Z

Looks great! One last thing, it looks like the docs for frustum in both p5 and p5.Camera could also use that same translate call, mind adding that there too? then I'll go ahead and merge everything in!

Garima3110 · 2023-11-23T20:51:30Z

Looks great! One last thing, it looks like the docs for frustum in both p5 and p5.Camera could also use that same translate call, mind adding that there too? then I'll go ahead and merge everything in!

Thanks a lot @davepagurek for your suggestions.! Learnt a lot solving this issue. Please let me know if some other changes are to be made to this one.

davepagurek

Thanks so much for your work on this!

inaridarkfox4231 · 2023-12-09T01:29:17Z

Many questions remain. It's one thing to not execute orbitControl(), but why didn't you check other examples as well?
There is no reason to leave the reference example incorrect. I can't understand why they tried to solve the problem with a simple band-aid solution, even though it could have been done by restoring the camera's specifications by camera function.

Solves issue processing#6519

3d8876c

davepagurek reviewed Nov 14, 2023

View reviewed changes

Garima3110 added 2 commits November 15, 2023 11:26

Merge remote-tracking branch 'upstream/main' into perspective

ea17d7a

Updated reference for perspective()

b482b2d

Garima3110 added 2 commits November 23, 2023 12:19

Merge remote-tracking branch 'upstream/main' into perspective

78b85c6

Updated perspective and ortho methods example code

32d83f7

Garima3110 added 2 commits November 24, 2023 02:06

Updated docs for frustum method in both p5 and p5.Camera

fe87595

Merge remote-tracking branch 'upstream/main' into perspective

f4948c4

davepagurek approved these changes Nov 23, 2023

View reviewed changes

davepagurek merged commit ecaea3a into processing:main Nov 23, 2023
2 checks passed

Garima3110 deleted the perspective branch November 24, 2023 10:28

inaridarkfox4231 mentioned this pull request Dec 7, 2023

Due to #6216, some of the reference examples related to cameras are broken. #6611

Closed

17 tasks

inaridarkfox4231 mentioned this pull request Jan 20, 2024

Default camera view changed in v1.9? #6694

Open

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solves issue #6519 #6549

Solves issue #6519 #6549

Garima3110 commented Nov 11, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

davepagurek Nov 14, 2023

Garima3110 commented Nov 15, 2023

Garima3110 commented Nov 15, 2023 •

edited

Loading

davepagurek commented Nov 17, 2023

Garima3110 commented Nov 17, 2023 •

edited

Loading

davepagurek commented Nov 17, 2023

Garima3110 commented Nov 18, 2023

davepagurek commented Nov 21, 2023

davepagurek commented Nov 23, 2023

Garima3110 commented Nov 23, 2023

davepagurek left a comment

inaridarkfox4231 commented Dec 9, 2023

	* perspective(fovy, width / height, cameraDistance / 10, cameraDistance * 10);
	* `perspective(fovy, width / height, cameraDistance / 10, cameraDistance * 10);`

	* near - The default value for the near clipping plane is 0.1 times the default distance from the camera to the point it is looking at i.e 800.
	* near - The default value for the near clipping plane is 80, which is 0.1 times the default distance from the camera to its subject.

	* far - The default value for the far clipping plane is 10 times the default distance from the camera to the point it is looking at i.e 800.
	* far - The default value for the far clipping plane is 8000, which is 10 times the default distance from the camera to its subject.

Solves issue #6519 #6549

Solves issue #6519 #6549

Conversation

Garima3110 commented Nov 11, 2023

PR Checklist

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

Choose a reason for hiding this comment

Garima3110 commented Nov 15, 2023

Garima3110 commented Nov 15, 2023 • edited Loading

davepagurek commented Nov 17, 2023

Garima3110 commented Nov 17, 2023 • edited Loading

davepagurek commented Nov 17, 2023

Garima3110 commented Nov 18, 2023

davepagurek commented Nov 21, 2023

davepagurek commented Nov 23, 2023

Garima3110 commented Nov 23, 2023

davepagurek left a comment

Choose a reason for hiding this comment

inaridarkfox4231 commented Dec 9, 2023

Garima3110 commented Nov 15, 2023 •

edited

Loading

Garima3110 commented Nov 17, 2023 •

edited

Loading