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

Solves issue #4214 #6734

Merged
merged 19 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion src/webgl/p5.Camera.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,78 @@ p5.prototype.perspective = function (...args) {
return this;
};


/**
*
* Enable or disable perspective for lines in the WebGL renderer.
* The behavior of `linePerspective()` is associated with the type of camera projection being used.
*
* - When using `perspective()`, which simulates realistic perspective, linePerspective
* is set to `true` by default. This means that lines will be affected by the current
* camera's perspective, resulting in a more natural appearance.
* - When using `ortho()` or `frustum()`, which do not simulate realistic perspective,
* linePerspective is set to `false` by default. In this case, lines will have a uniform
* scale regardless of the camera's perspective, providing a more predictable and
* consistent appearance.
* - You can override the default behavior by explicitly calling `linePerspective()` after
* using `perspective()`, `ortho()`, or `frustum()`. This allows you to customize the line
* perspective based on your specific requirements.
*
* @method linePerspective
* @memberof p5.prototype
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think elsewhere we tend to use he following syntax to say that it's a global p5 method:

Suggested change
* @memberof p5.prototype
* @for p5

* @param {boolean} enable - Set to `true` to enable line perspective, `false` to disable.
* @return {boolean} The boolean value representing the current state of linePerspective().
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For other p5 methods that could either be a setter or a getter, we just do one of them in the doc comment (e.g. here, having a @param and no @return) and then adding a second doc comment right after the first with the alternate overload (e.g. a @return with no @param.) An example of that is here:

/**
* @method frameRate
* @return {Number} current frame rate.
*/

Can we do the same thing here too?

*<br>
* @example
* <div>
* <code>
* function setup() {
* createCanvas(100, 100, WEBGL);
* strokeWeight(3);
* describe(
* 'rotated 3D boxes have their stroke weights affected after mouse is clicked.'
* );
* }
*
* function draw() {
* background(220);
* rotateY(PI/8);
* rotateZ(PI/8);
* translate(0, 0, 350);
* for (let i = 0; i < 12; i++) {
* translate(0, 0, -70);
* box(30);
* }
* }
*
* function mousePressed() {
* perspective(PI/12, width/height, 1, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't the default camera perspective any more, when you click, both the perspective in the lines and for the overall image changes. Maybe it would show the difference more clearly if we just call linePerspective(false) without also changing the perspective of the scene?

* linePerspective(false);
* }
* </code>
* </div>
*
* @alt
* Demonstrates the dynamic control of line perspective in a 3D environment with rotating boxes.
*/

p5.prototype.linePerspective = function (enable) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a second overload to this function that has no parameters and returns the current value? Similar to frameRate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davepagurek do you want me to validate its parameters too or this would work fine?

p5.prototype.linePerspective = function (enable) {
  if (enable !== undefined) {
    // Set the line perspective if enable is provided
    this._renderer._curCamera.useLinePerspective = enable;
  } else {
    // If no argument is provided, return the current value
    return this._renderer._curCamera.useLinePerspective;
  }
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validating parameters would be great too! Looks good so far.

p5._validateParameters('linePerspective', arguments);

if (!this._renderer._curCamera) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you encounter something in testing where where this wasn't defined out of curiosity?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, actually I got this on the console initially when i didn't write this if block :
image
For the sketch: sketch1.js
Sketch link: https://editor.p5js.org/Garima3110/sketches/p8jRwB5dw

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's just happening because your setup is:

function setup() {
  createCanvas(400, 400);
  stroke(255);
  strokeWeight(2);
}

Doing createCanvas(400,400,WEBGL) causes the error to go away. Maybe rather than creating a camera in this if statement, we can throw an error saying that the method needs to be called in WebGL mode?

this._renderer._curCamera = new p5.Camera(this._renderer);
}

if (enable !== undefined) {
// Set the line perspective if enable is provided
this._renderer._curCamera.useLinePerspective = enable;
} else {
// If no argument is provided, return the current value
return this._renderer._curCamera.useLinePerspective;
}
};


/**
* Sets an orthographic projection for the current camera in a 3D sketch
* and defines a box-shaped viewing frustum within which objects are seen.
Expand Down Expand Up @@ -487,7 +559,7 @@ p5.Camera = class Camera {
this._renderer = renderer;

this.cameraType = 'default';

this.useLinePerspective = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think calling perspective() should set this to true and calling ortho() or frustum() should set this to false? A user could then override it with linePerspective(), but still match the defaults we have now.

Copy link
Member Author

@Garima3110 Garima3110 Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes maybe, I think it makes sense to associate the behavior of linePerspective() with the type of camera projection being used. If you're using perspective(), which simulates realistic perspective, having linePerspective() set to true by default aligns well with that mode. Similarly, when using ortho() or frustum(), which do not simulate realistic perspective, setting linePerspective() to false by default is appropriate.
This way, the default behavior aligns with the common expectations of how lines should behave based on the chosen camera projection.
let me know what's your take on this.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense! we can mention in the documentation for linePerspective that perspective(), ortho(), and frustum() all change the value of linePerspective, so you can call it after those calls to override it.

this.cameraMatrix = new p5.Matrix();
this.projMatrix = new p5.Matrix();
this.yScale = 1;
Expand Down
1 change: 1 addition & 0 deletions src/webgl/p5.RendererGL.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const defineStrokeJoinEnum = function (key, val) {
STROKE_JOIN_ENUM[constants[key]] = val;
};


// Define constants in line shaders for each type of cap/join, and also record
// the values in JS objects
defineStrokeCapEnum('ROUND', 0);
Expand Down
2 changes: 1 addition & 1 deletion src/webgl/p5.Shader.js
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ p5.Shader = class {
this.setUniform('uPerspective', 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing out the example and realized that we still force perspective when using a default camera. Maybe we can take out this if statement and just use the else branch?

} else {
// strokes have uniform scale regardless of distance from camera
this.setUniform('uPerspective', 0);
this.setUniform('uPerspective', this._renderer._curCamera.useLinePerspective ? 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}
}
this.setUniform('uViewMatrix', viewMatrix.mat4);
Expand Down
Loading