Adding strokeMode() to access SIMPLE lines. #7390
Conversation
|
The test which are currently failing are
I have observed that these tests are also failing in the current Dev 2.0 branch. Therefore, I believe that this pull request is not related to the issues affecting these tests. |
|
It looks like they're hitting an error in the simple lines code, we might need to double check that it's accessing the properties correctly: |
There was a problem hiding this comment.
Thanks @perminder-17, this is looking good, great explanation of the use of both modes!
src/core/constants.js
Outdated
| * @property {String} FULLY | ||
| * @final | ||
| */ | ||
| export const FULLY = 'fully'; |
There was a problem hiding this comment.
A minor note, could we call this FULL instead of FULLY to use the same adjective form as SIMPLE?
src/webgl/3d_primitives.js
Outdated
| */ | ||
|
|
||
| fn.strokeMode = function(mode){ | ||
| if(mode === constants.SIMPLE){ |
There was a problem hiding this comment.
Could we make this return the current mode if we call it with no arguments?
src/webgl/p5.Geometry.js
Outdated
| @@ -1398,7 +1399,7 @@ class Geometry { | |||
| if (dirOK) { | |||
| this._addSegment(begin, end, fromColor, toColor, dir); | |||
| } | |||
|
|
|||
| if(!this.renderer._simpleLines){ | |||
There was a problem hiding this comment.
Since p5.Geometry created manually won't have a renderer passed in, I think we need to handle the case where there is no renderer, and mayube default to the full line behaviour:
| if(!this.renderer._simpleLines){ | |
| if(!this.renderer?._simpleLines){ |
There was a problem hiding this comment.
Also, do you think we could intent the block inside this to keep the indentation consistent everywhere?
src/webgl/p5.Geometry.js
Outdated
| @@ -1520,6 +1522,7 @@ class Geometry { | |||
| } | |||
| } | |||
| this.lineVertices.push(...a, ...b, ...a, ...b, ...b, ...a); | |||
| if(!this.renderer._simpleLines){ | |||
There was a problem hiding this comment.
Similarly here, we might need to check for the existence of the renderer first:
| if(!this.renderer._simpleLines){ | |
| if(!this.renderer?._simpleLines){ |
src/webgl/shaders/line.vert
Outdated
| @@ -67,6 +68,8 @@ vec2 lineIntersection(vec2 aPoint, vec2 aDir, vec2 bPoint, vec2 bDir) { | |||
|
|
|||
| void main() { | |||
| HOOK_beforeVertex(); | |||
|
|
|||
| if(!uSimpleLines){ | |||
There was a problem hiding this comment.
Could we indent the block below?
src/webgl/3d_primitives.js
Outdated
| * let lineLength = 50; | ||
| * | ||
| * beginShape(LINES); | ||
| * vertex(centerX - lineLength / 2, centerY, 0); |
There was a problem hiding this comment.
How do you feel about having a 3D shape like a default sphere(), and maybe a single Bezier curve in each of these examples? I feel like those are two cases where the difference is clear: the sphere probably looks the same regardless, and the curve (especially with a higher stroke weight) will look very disjoint without full curve decorations.
We can also make the examples more than 100x100 if that makes it easier to see since I think the new site's layout accommodates that decently.
|
Adjusting the indentation caused unexpected spacing issues in the code, making it difficult to review. I apologize for any inconvenience this may have caused. Also, the checks were failing because we need to check the existence of the renderer first. Thanks for the suggestions:) |
fixes : #7278
Created a method
strokeMode()to access SIMPLE lines for faster fps.