Add ShapePrimitive support for arcs and ellipses#8617
Add ShapePrimitive support for arcs and ellipses#8617VANSH3104 wants to merge 1 commit intoprocessing:dev-2.0from
Conversation
|
@ksen0, @davepagurek could you please review this and suggest any changes? |
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for taking this on! I left a few comments about how to make these primitives work more like the others. We should also add some unit visual in both 2D and WebGL mode of these methods if we don't already have some to make sure things keep working.
|
|
||
| const primitive = new EllipsePrimitive(x, y, w, h); | ||
| const shape = { accept(visitor) { primitive.accept(visitor); } }; | ||
| this.drawShape(shape); |
There was a problem hiding this comment.
It looks like drawShape already handles clipping so we can remove the clipping code above:
p5.js/src/core/p5.Renderer2D.js
Lines 269 to 279 in 01aa360
| } | ||
|
|
||
| const primitive = new EllipsePrimitive(x, y, w, h); | ||
| const shape = { accept(visitor) { primitive.accept(visitor); } }; |
There was a problem hiding this comment.
Rather than making an object that isn't a full p5.Shape, could we instead make a new real p5.Shape and create a method on it that adds an ellipse primitive? similar to the relation between these methods and their corresponding primitives:
p5.js/src/shape/custom_shapes.js
Lines 891 to 906 in 618e4c8
| const centerY = arc.y + arc.h / 2; | ||
| const radiusX = arc.w / 2; | ||
| const radiusY = arc.h / 2; | ||
| const numPoints = Math.max(3, this.curveDetail); |
There was a problem hiding this comment.
curveDetail is a multiplier on the length, so numPoints should be something like ceil(this.curveDetail * arcLength)
| #stop; | ||
| #mode; | ||
| // vertexCapacity 0 means this primitive should not accumulate normal path vertices | ||
| #vertexCapacity = 0; |
There was a problem hiding this comment.
Rather than a capacity of zero, possibly we can contain a vertex representing the start and the end so that we can include all the other properties on a general vertex, which might include texture coordinates, fill, and stroke colors in webgl? Ideally it should go through #generalVertex in Shape to get all of that functionality.
|
|
||
| for (let i = 0; i <= numPoints; i++) { | ||
| const angle = arc.start + (arc.stop - arc.start) * (i / numPoints); | ||
| verts.push(new Vertex({ |
There was a problem hiding this comment.
Similar to what I mentioned above in ArcPrimitive, ideally we should have some vertices in the primitive already so we can copy their data all around the ellipse but update the position part (the first 2 elements) for arcs and ellipses. For arcs, ideally we would also interpolate the properties of the vertex from the start to the end.
Resolves #8277
Changes:
PR Checklist
npm run lintpasses