Specification change for ortho() and frustum(), and implementation of scaling in orbitControl() of ortho() #6111

inaridarkfox4231 · 2023-04-20T15:40:32Z

Resolves #6106

Changes:

First, Causes the camera properties cameraNear and cameraFar to be set to the near and far values when calling ortho() and frustum(). These values are used as limits when changing the scale in orbitControl().

Second, I fixed frustum() default value like ortho(). The previous default value ignored how to set the value of frustum(), so if you applied it as it was, the object would disappear.

Also changed the diagonal y to -y in the frustum() projection matrix. This is not so much a fix as perspective() and ortho() are like that, so I wanted to match them.

Finally, in the case of ortho() in orbitControl(), it was not possible to change the scale by scrolling the mouse, but by changing the projection matrix, it became possible.

Screenshots of the change:

sample code: fix_ortho_frustum Demo

This is a video showing how the scale can be changed even when using orbitControl() with ortho().

2023-04-21.00-37-25.mp4

PR Checklist

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

Make sure cameraFar and cameraNear are properly changed when calling ortho() and frustum(). Also change frustum() defaults. In addition, change y in frustum's projection matrix to -y. Match other projection matrices.

Even if you call orbitControl() with ortho(), you can still zoom in and out with the mouse wheel. Note that although it is no longer just "radius", we will not rename it to avoid confusion.

Due to the change in the specification of frustum(), the values of the projection matrix are different, so I also modified the unit test.

This is probably due to the fact that 0 is not negative.

inaridarkfox4231 · 2023-04-21T01:20:45Z

I have one question, the default value of frustum() has changed, so I think I have to rewrite the reference. Where should I fix it?
If I modify this part of src/webgl/p5.Camera, will it be reflected as it is?

davepagurek · 2023-04-21T11:35:07Z

Right, if you edit the doc comment there, when we release a new p5 version, it'll be reflected in the docs on the website. If you want to test a docs change locally, you can run grunt yui:dev to run a local server with the documentation 🙂

davepagurek

Nice work! A few minor comments so far that might make the code easier for future maintainers and contributors to understand.

Also, maybe we should add a unit test for the zooming behaviour in orbitControl? So that we can assert that the projection matrix only changes for ortho cameras

davepagurek · 2023-04-21T11:37:27Z

src/webgl/interaction.js

+    const mouseWheelSign = (this._mouseWheelDeltaY > 0 ? 1 : -1);
+    const deltaRadius = mouseWheelSign * sensitivityZ * zoomScaleFactor;
+    this._renderer._curCamera._orbit(0, 0, deltaRadius);
+    // if ortho, scale change.


For future contributors, it's probably worth elaborating a little bit here about why this is necessary (ortho cameras don't make far things smaller, so just moving the camera wouldn't result in any visual change)

Certainly, the projection matrix is only manipulated when performing orthogonal projection, so an explanation is necessary...
For example, ”For orthogonal projection, the elements of the projection matrix must be changed so that the scaling is done.”?

I think that's a good start, although it would also be good to explain why scaling is necessary at all for ortho cameras (so, why just moving the camera isn't sufficient)

I see, I'm sure it would be easier to explain the reason for the change... how about this?: "In orthogonal projection, the scale does not change even if the distance to the gaze point is changed, so the projection matrix needs to be modified."

sounds great, thanks!

davepagurek · 2023-04-21T11:38:38Z

src/webgl/interaction.js

+    const deltaRadius = mouseWheelSign * sensitivityZ * zoomScaleFactor;
+    this._renderer._curCamera._orbit(0, 0, deltaRadius);
+    // if ortho, scale change.
+    if (uP[15] !== 0) {


Nice find, I didn't realize that ortho was the only one that has a 1 in that corner!

I had to think about that. It would be nice if it could be determined by a flag or something else that it is an orthogonal projection, but "cameraType" cannot do so (ortho and frustum are the same, "custom"), so I decided to rely on the components.

davepagurek · 2023-04-21T11:39:34Z

src/webgl/interaction.js

@@ -93,14 +93,18 @@ p5.prototype.orbitControl = function(sensitivityX, sensitivityY, sensitivityZ) {

  const zoomScaleFactor = 0.02;
  const scaleFactor = this.height < this.width ? this.height : this.width;
+  const uP = this._renderer.uPMatrix.mat4;


Do you think maybe we should edit this._renderer.uPMatrix.mat4 directly instead of making a new variable referencing the same object? The result is the same either way, but it might be clearer to readers that we're modifying the matrix directly.

If it's only used here, that's fine, but I think it might be better to prepare it as a variable in advance because it will be used later when creating a method for moving with the right mouse button.

ah, were you thinking of implementing that via the projection matrix? I was thinking movement would still be done by changing uModelViewMatrix (at least for perspective cameras), would that also work for ortho cameras?

That idea is to drop the gaze point into the normalized device coordinate system using the camera matrix and the projection matrix, move the mouse by the inverse of the movement distance, and focus on the position where the normalized device coordinates come to the destination.
It shifts the viewpoint by parallel movement.
This allows the movement of the camera along with the movement of the mouse. I don't use the modelview matrix because it doesn't involve model transformations. I've already confirmed that it works fine with perspective, ortho, and frustum.
demo:improve panning

Ahh I see, I thought you meant you would be updating the projection matrix to implement movement, I see what you mean now -- this is what I was suggesting too.

I think it might still be more readable if we declare uP just in that branch for that case, as we're just reading it. Then, here, where we're actually modifying it, it's clear to readers that the uPMatrix property will be modified.

I got it. So, let's write this directly and declare it again in the method I will implement later.

The default value of frustum() has changed, so I rewrote the reference.

Added the reason why the projection matrix is changed if it is an orthogonal projection when scrolling the mouse in orbitControl().

In order to clearly indicate that the elements of the matrix have changed, I decided to write them directly instead of using variables.

davepagurek

Thanks for making this change, I think this makes these camera modes a lot nicer to work with!

inaridarkfox4231 · 2023-04-21T17:47:53Z

Thanks for advices!

inaridarkfox4231 added 4 commits April 20, 2023 11:26

Allow ortho and frustum to set cameraNear/Far, etc

031f4fd

Make sure cameraFar and cameraNear are properly changed when calling ortho() and frustum(). Also change frustum() defaults. In addition, change y in frustum's projection matrix to -y. Match other projection matrices.

Allow scaling even in ortho() mode with orbitCtl

f3f27b2

Even if you call orbitControl() with ortho(), you can still zoom in and out with the mouse wheel. Note that although it is no longer just "radius", we will not rename it to avoid confusion.

Fix values of the projection matrix by frustum()

bb046e2

Due to the change in the specification of frustum(), the values of the projection matrix are different, so I also modified the unit test.

add sign to 0

088be2b

This is probably due to the fact that 0 is not negative.

davepagurek reviewed Apr 21, 2023

View reviewed changes

inaridarkfox4231 added 3 commits April 22, 2023 01:48

Change default values for frustum()

ea0887b

The default value of frustum() has changed, so I rewrote the reference.

Added reason for changing projection matrix

1d2e65c

Added the reason why the projection matrix is changed if it is an orthogonal projection when scrolling the mouse in orbitControl().

Write the projection matrix directly.

71de03d

In order to clearly indicate that the elements of the matrix have changed, I decided to write them directly instead of using variables.

davepagurek approved these changes Apr 21, 2023

View reviewed changes

davepagurek merged commit c27002c into processing:main Apr 21, 2023

inaridarkfox4231 deleted the fix-ortho-and-frustum branch April 21, 2023 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specification change for ortho() and frustum(), and implementation of scaling in orbitControl() of ortho() #6111

Specification change for ortho() and frustum(), and implementation of scaling in orbitControl() of ortho() #6111

inaridarkfox4231 commented Apr 20, 2023

inaridarkfox4231 commented Apr 21, 2023

davepagurek commented Apr 21, 2023

davepagurek left a comment

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek Apr 21, 2023

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek Apr 21, 2023

davepagurek Apr 21, 2023

inaridarkfox4231 Apr 21, 2023

davepagurek left a comment

inaridarkfox4231 commented Apr 21, 2023

Specification change for ortho() and frustum(), and implementation of scaling in orbitControl() of ortho() #6111

Specification change for ortho() and frustum(), and implementation of scaling in orbitControl() of ortho() #6111

Conversation

inaridarkfox4231 commented Apr 20, 2023

Changes:

Screenshots of the change:

PR Checklist

inaridarkfox4231 commented Apr 21, 2023

davepagurek commented Apr 21, 2023

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

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

davepagurek left a comment

Choose a reason for hiding this comment

inaridarkfox4231 commented Apr 21, 2023