Solves issue #4214 #6734

Garima3110 · 2024-01-12T18:40:28Z

Changes:
Solves issue #4214

I have tried implementing the linePerspective() method to solve this issue.
@davepagurek Please let me know is this the right approach for its implementation or not.
I would love to have suggestions to this.
Thanks a lot!

Sample sketch:
https://editor.p5js.org/Garima3110/sketches/p8jRwB5dw

PR Checklist

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

perminder-17 · 2024-01-13T13:10:38Z

Thank you for your efforts, Garima. I apologize if I misunderstood your approach, but I have a concern about the linePerspective() function. It seems that the issue with the misbehavior of strokeWeight occurs after the perspective() function is called, right? In your sketch, it seems to happen by default when we remove the linePerspective(true/false) method. Additionally, when we use linePerspective(true/false), setting it to true seems to work well in your sketch. However, if someone sets linePerspective to false, it should use the default perspective camera. Perhaps we could implement something like this? Currently, when we use false, the stroke misbehaves in both cases, with and without the perspective call.

Actual

when linePerspective(false) with and without perspective

Maybe it should be like

I think opting for this approach could be the most straightforward, as opposed to delving into intricate calculations for a custom perspective. However, I'm not entirely certain if this is the correct solution. I apologize if my approach doesn't prove effective.

Garima3110 · 2024-01-13T16:06:53Z

I think opting for this approach could be the most straightforward, as opposed to delving into intricate calculations for a custom perspective. However, I'm not entirely certain if this is the correct solution. I apologize if my approach doesn't prove effective.

Thanks a lot @perminder-17 for your inputs. I'll just look into what's going wrong in here and get back to you soon!

…nto stroke-weight

davepagurek · 2024-01-28T19:12:50Z

src/webgl/p5.Camera.js

+ * @example
+ * @todo
+ */
+p5.prototype.linePerspective = function (enable) {


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

@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; } };

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

davepagurek · 2024-01-28T19:14:02Z

src/webgl/p5.Shader.js

@@ -329,7 +329,7 @@ p5.Shader = class {
        this.setUniform('uPerspective', 1);
      } else {
        // strokes have uniform scale regardless of distance from camera
-        this.setUniform('uPerspective', 0);
+        this.setUniform('uPerspective', this._renderer._curCamera.useLinePerspective ? 1 : 0);


davepagurek · 2024-01-28T19:15:11Z

src/webgl/p5.Camera.js

@@ -488,7 +505,7 @@ p5.Camera = class Camera {
    this._renderer = renderer;

    this.cameraType = 'default';
-
+    this.useLinePerspective = true;


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.

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!

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.

Garima3110 · 2024-01-29T15:22:47Z

Yeah sure! I’ll just do that and get back to you soon. thanks a lot for the review.

Garima3110 · 2024-02-13T13:47:59Z

@davepagurek Sorry to directly ping you here, but its just to tell you that I had made the changes suggested by you. Please review them whenever you find time . No hurries at all !
Thanks a lot..!

davepagurek

Nice work! Just some minor comments on the docs formatting to be more consistent.

davepagurek · 2024-02-16T00:46:25Z

src/webgl/p5.Camera.js

+ *  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


Maybe we can do these as inline code too for consistency:

Suggested change

* using perspective(), ortho(), or frustum(). This allows you to customize the line

* using `perspective()`, `ortho()`, or `frustum()`. This allows you to customize the line

davepagurek · 2024-02-16T00:51:09Z

src/webgl/p5.Camera.js

 *
 * @method linePerspective
 * @memberof p5.prototype
 * @param {boolean} enable - Set to `true` to enable line perspective, `false` to disable.
+ * @return {boolean} The boolean value representing the current state of linePerspective().


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:

p5.js/src/core/environment.js

Lines 368 to 371 in 37d3324

/**

* @method frameRate

* @return {Number} current frame rate.

*/

Can we do the same thing here too?

davepagurek · 2024-02-16T00:53:34Z

src/webgl/p5.Camera.js

-  this._renderer._curCamera.useLinePerspective = enable;
+  p5._validateParameters('linePerspective', arguments);
+
+  if (!this._renderer._curCamera) {


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

Yeah, actually I got this on the console initially when i didn't write this if block :

For the sketch: sketch1.js
Sketch link: https://editor.p5js.org/Garima3110/sketches/p8jRwB5dw

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?

Garima3110 · 2024-02-20T20:11:39Z

@davepagurek I have made the changes as suggested. Please have a look , and let me know of any other change. Thank you!

davepagurek · 2024-02-23T01:00:41Z

src/webgl/p5.Shader.js

@@ -329,7 +329,7 @@ p5.Shader = class {
        this.setUniform('uPerspective', 1);


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?

davepagurek · 2024-02-23T01:02:31Z

src/webgl/p5.Camera.js

-  this._renderer._curCamera.useLinePerspective = enable;
+  p5._validateParameters('linePerspective', arguments);
+
+  if (!this._renderer._curCamera) {


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?

davepagurek · 2024-02-23T01:03:41Z

src/webgl/p5.Camera.js

+ *    perspective based on your specific requirements.
+ *
+ * @method linePerspective
+ * @memberof p5.prototype


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

davepagurek · 2024-02-23T01:05:37Z

src/webgl/p5.Camera.js

+ * }
+ *
+ * function mousePressed() {
+ *   perspective(PI/12, width/height, 1, 10000);


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?

Garima3110 · 2024-02-23T03:46:39Z

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

Garima3110 · 2024-02-26T20:05:43Z

@davepagurek , Here's the sketch with the latest build including the changes you suggested:
https://editor.p5js.org/Garima3110/sketches/p8jRwB5dw
Please have a look. Thankyou!

Garima3110 · 2024-02-26T20:52:12Z

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?

I am not exactly sure how to go about it , as I think directly writing this won't do the job:

  if (!this._renderer===constants.WEBGL) {
    throw new Error('linePerspective() must be called in WebGL mode.');
  }

It would be great if you could guide me through the process whenever you find time.
Thanks a lot..!

davepagurek · 2024-02-26T23:36:25Z

Something pretty close to that! I think the way you would check would be:

if (!(this._renderer instanceof p5.RendererGL)) {
  throw new Error('linePerspective() must be called in WebGL mode.');
}

Garima3110 · 2024-02-27T09:54:27Z

Great! It works as expected. Thanks @davepagurek
And let me know for any further changes to this.

davepagurek · 2024-02-27T16:53:44Z

Some last thoughts about the examples: right now it's a bit unclear what the difference is. How do you feel about:

Changing the rotation a bit (I tried rotateY(PI/24)) so you can see the cubes getting smaller into the distance more clearly in the small canvas size
Adding setAttributes({ antialias: true }) after creating the canvas to get a bit better line quality
Maybe making mouse click toggle back and forth rather than just turning off perspective? e.g. linePerspective(!linePerspective())

Screen.Recording.2024-02-27.at.11.50.39.AM.mov

Garima3110 · 2024-02-27T19:41:10Z

Changing the rotation a bit (I tried rotateY(PI/24)) so you can see the cubes getting smaller into the distance more clearly in the small canvas size

Adding setAttributes({ antialias: true }) after creating the canvas to get a bit better line quality

Maybe making mouse click toggle back and forth rather than just turning off perspective? e.g. linePerspective(!linePerspective())

Great suggestions @davepagurek I have made the changes please have a look!

davepagurek

Awesome, thanks for all the changes! The examples look great, I think this is good to go.

Garima3110 · 2024-03-12T20:34:33Z

Awesome, thanks for all the changes! The examples look great, I think this is good to go.

Thanks a lot @dave for the help while working on this! Learnt a lot.

Garima3110 added 2 commits January 12, 2024 23:57

Solves issue processing#4214

ba0f02e

Updated inline docs for linePerspective()

656f80c

Garima3110 and others added 5 commits January 16, 2024 18:58

Merge branch 'processing:main' into stroke-weight

c86235e

Some fixes for linePerspective()

2e945b2

Merge remote-tracking branch 'upstream/main' into stroke-weight

e820394

Merge branch 'stroke-weight' of https://github.com/Garima3110/p5.js i…

6b0957f

…nto stroke-weight

Minor changes

ceec746

davepagurek reviewed Jan 28, 2024

View reviewed changes

Merge branch 'processing:main' into stroke-weight

69e7276

Garima3110 and others added 3 commits February 3, 2024 13:34

Minor changes in linePerspective()

e0ed90a

Merge branch 'processing:main' into stroke-weight

f807ca7

Improved docs for linePerspective()

3ea9737

davepagurek reviewed Feb 16, 2024

View reviewed changes

Garima3110 added 3 commits February 20, 2024 00:21

Updated inline docs+code for linePerspective()

6640e17

Merge remote-tracking branch 'upstream/main' into stroke-weight

15ef523

fixed linting errors

fbd12d9

davepagurek reviewed Feb 23, 2024

View reviewed changes

Garima3110 added 2 commits February 27, 2024 01:36

Minor fixes

2e72d2e

Merge remote-tracking branch 'upstream/main' into stroke-weight

517803e

Updated linePerspective

9ebecea

Garima3110 added 2 commits February 28, 2024 01:07

Minor updates on inline doc

bbb4a3f

Merge remote-tracking branch 'upstream/main' into stroke-weight

53a6c4d

davepagurek approved these changes Mar 12, 2024

View reviewed changes

davepagurek merged commit 9fce4ec into processing:main Mar 12, 2024
2 checks passed

Garima3110 deleted the stroke-weight branch May 31, 2024 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solves issue #4214 #6734

Solves issue #4214 #6734

Garima3110 commented Jan 12, 2024 •

edited

Loading

perminder-17 commented Jan 13, 2024

Garima3110 commented Jan 13, 2024

davepagurek Jan 28, 2024

Garima3110 Jan 29, 2024

davepagurek Jan 29, 2024

davepagurek Jan 28, 2024

davepagurek Jan 28, 2024

Garima3110 Jan 29, 2024 •

edited

Loading

davepagurek Jan 29, 2024

Garima3110 commented Jan 29, 2024 •

edited

Loading

Garima3110 commented Feb 13, 2024

davepagurek left a comment

davepagurek Feb 16, 2024

davepagurek Feb 16, 2024

davepagurek Feb 16, 2024

Garima3110 Feb 16, 2024

davepagurek Feb 23, 2024

Garima3110 commented Feb 20, 2024

davepagurek Feb 23, 2024

davepagurek Feb 23, 2024

davepagurek Feb 23, 2024

davepagurek Feb 23, 2024

Garima3110 commented Feb 23, 2024

Garima3110 commented Feb 26, 2024 •

edited

Loading

Garima3110 commented Feb 26, 2024 •

edited

Loading

davepagurek commented Feb 26, 2024

Garima3110 commented Feb 27, 2024 •

edited

Loading

davepagurek commented Feb 27, 2024

Garima3110 commented Feb 27, 2024

davepagurek left a comment

Garima3110 commented Mar 12, 2024

	* using perspective(), ortho(), or frustum(). This allows you to customize the line
	* using `perspective()`, `ortho()`, or `frustum()`. This allows you to customize the line

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

		@@ -329,7 +329,7 @@ p5.Shader = class {
		this.setUniform('uPerspective', 1);

Solves issue #4214 #6734

Solves issue #4214 #6734

Conversation

Garima3110 commented Jan 12, 2024 • edited Loading

PR Checklist

perminder-17 commented Jan 13, 2024

Actual

Maybe it should be like

Garima3110 commented Jan 13, 2024

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 Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Garima3110 commented Jan 29, 2024 • edited Loading

Garima3110 commented Feb 13, 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

Garima3110 commented Feb 20, 2024

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 Feb 23, 2024

Garima3110 commented Feb 26, 2024 • edited Loading

Garima3110 commented Feb 26, 2024 • edited Loading

davepagurek commented Feb 26, 2024

Garima3110 commented Feb 27, 2024 • edited Loading

davepagurek commented Feb 27, 2024

Garima3110 commented Feb 27, 2024

davepagurek left a comment

Choose a reason for hiding this comment

Garima3110 commented Mar 12, 2024

Garima3110 commented Jan 12, 2024 •

edited

Loading

Garima3110 Jan 29, 2024 •

edited

Loading

Garima3110 commented Jan 29, 2024 •

edited

Loading

Garima3110 commented Feb 26, 2024 •

edited

Loading

Garima3110 commented Feb 26, 2024 •

edited

Loading

Garima3110 commented Feb 27, 2024 •

edited

Loading