-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[p5.js 2.0 RFC Proposal]: New API for vertex functions #6766
Comments
Thanks for the writeup @GregStanton! I'm in favor of the consistency we'd get from an API like this, as well as the readability (any time a function has a bunch of positional arguments, I find myself having to write comments between arguments to remind myself which is which.) For curveVertex texture coordinates, I think the same pattern we're suggesting for bezier vertices will also apply. When getting a position along a Catmull-Rom curve in between two control points, the interpolated position is a weighted sum of the curve's control points. I think we can do the same weighting for texture coordinates if we have one texture coordinate per control point, similar to position. |
I do really like the overall simplicity and flexibility this can potentially give. One thing I would like to illustrate this proposal a bit more would be some more complete code examples, maybe even a before and after versions to make the use case clearer. I'll probably share this with my colleagues teaching with p5.js to get their thoughts as well. |
@limzykenneth Thanks! Code examples are a great idea. Here's a list of examples that illustrates some key differences: Example 1Current syntax:
Proposed Syntax:
Example 2Current syntax (the Bézier case isn't actually supported, but a user might expect this to work):
Proposed syntax (contours are used to specify separate pieces of different orders):
Example 3Current syntax (the first one will cause an error because it's not supported, due to an inconsistency):
Proposed syntax:
Example 4Current syntax (this will produce an error because higher-order Bézier curves aren't currently supported):
Proposed syntax (this assumes we want to support higher-order Bézier curves):
Example 5Current syntax (this is how a user might try to create a Bézier triangle, but that's not currently supported):
Proposed syntax:
Special casesShape built from multiple béziersOne issue with having a single Composite shapesCurrently, composite shapes aren't supported, but #6560 would address this, and it provides more examples of that type in the current syntax. TODO / Invitation for contributions
|
I want to also invite @peilingjiang who created the p5.bezier addon library to have a look at this. Seeing that your library also deals with Bezier curves and also support higher order Bezier curves, it would be great to hear your thoughts, we can even look at the implementation you have in p5.bezier as a starting point for this too. |
Thanks for the invite! Looking at the example code comparisons, I think this would be a very nice update, syntax-wise (I don't know how differently the updated functions will be implemented, so I'll only share some thoughts on the proposed syntax)! The vertex-related syntax would be unified, and having consistent, separate functions with a manageable number of arguments, instead of a crazy list of 9 or 15 arguments, would greatly improve the code readability. For the implementation, I hope my implementation for the higher-order Bézier curves would be helpful to serve as a starting point for the new native support. Moreover, I wonder if you would consider having an array of arrays of vertices, e.g., beginShape();
bezierVertices([
[x0, y0, z0, u0, v0],
[x1, y1, z1, u1, v1],
[x2, y2, z2, u2, v2],
[x3, y3, z3, u3, v3]
])
endShape(); I did something similar for the addon for the usability of the API, as managing items in an array (e.g., when adding or removing vertices) would be easier than modifying separate function calls. Moreover, the users may already maintain the positions somewhere else using nested arrays. This would be a further departure from the native Canvas API, but that might not be a bad thing - we could keep the old APIs for a few more minor version updates, mark them as deprecated, and introduce new ones like |
Thanks for your feedback @peilingjiang! Your library looks really cool, by the way. I think it'd be great to flesh out the pros and cons of your API idea, for the sake of discussion. I've already written up a partial list, but that gave me a new idea that I want to think about a bit more. I'll share what I have as soon as I'm able to think it over a bit more. |
Okay, @peilingjiang! I have some initial feedback to share regarding the API you proposed. ProsThe
ConsThe
Questions for the community
Edit: Added first point under cons, about features of the |
Thanks so much @davepagurek! This is a great discussion. Disambiguating primitivesI'm glad you raised the issue about using Having
Implicit start verticesYep! The current refactoring plan handles Note: These are initial thoughts. It will be good to confirm the details when time permits. |
I think I think as a matter of opinion, I'm still unsure that the extra code required to surround each segment by
The con for me is really just:
At least in my own experience, I've rarely felt the need to go higher than a cubic Bezier, and SVG/CanvasRenderingContext2D APIs don't natively support them either. I'd still love to support them, but I'm personally OK going with a more complex API for those since I see them as a very special case. Meanwhile, most shapes I'd want to draw (imagine drawing text glyphs as a motivating example) are composed of connected lines and curves, often with many connected cubic segments. Since a single text glyph can have upwards of 20 segments in it, and each segment would now need a duplicate vertex to remain connected, this adds a lot of overhead, which gives me pause. As an alternative, I think if we break each control point into their own call but don't try to support multi-order bezier and don't try to surround segments with other functions, we'd still check the other boxes we wanted. That's just an opinion based on my own experience though! I want to hear if other people's experience is similar to that too. As a final aside, I think if we keep separate methods for the different bezier orders, maybe it's worth aliasing |
Thanks @davepagurek! I'll reply to your points about using Considerations for
|
I think the issue I was thinking of is that, if there isn't an implicit
Here we've got all four in the segment. I guess the point I'm unclear on is whether or not that's a special case for just the first segment? e.g. would the following also be valid? beginShape();
vertex(x0, y0, z0);
beginPrimitive();
bezierVertex(x1, y1, z1);
bezierVertex(x2, y2, z2);
bezierVertex(x3, y3, z3);
endPrimitive();
endShape(); If it is, then does that mean that within the first primitive, bezier segments will always have one fewer point than in the later ones?
For context, what is the special case you're referring to here? |
Thanks for bringing up these points @davepagurek! This is all helpful for filling in the details. Bézier curves are a special case?Yeah, I probably could've worded that more clearly. What I meant was that So, I really meant that Implied
|
Thanks for explaining! I think my confusion about the special casing is because I saw a different pattern in vertex/quadraticVertex/bezierVertex: all can be used to chain multiple segments, but each segment has an increasing number of control points (1, 2, then 3) from the last point. In fact, vertex() can be seen as a linear bezier, which is just a line segment, needing no control point in the middle. In that sense, adding multiple vertices isn't different from cubic beziers, since each straight line is a segment, not the whole polyline. (That's also why curveVertex felt like the odd one out to me, where all the points make up one segment. For what it's worth, the curveVertex workflow feels pretty separate from the rest, which are shared in other vector software. A possibly simplifying mental model would just be to treat curve vertices as a shortcut to bezier segments rather than a "real" primitive?) One of the things that still feels inconsistent in the variable-order proposal is that multiple vertices adds more segments, but multiple of arc or bezier vertices increases the order of the same segment. To me it feels that to be consistent, more vertices in a row would do a line of beat fit through them rather than a polyline. I think the heart of what I'm suggesting is to always add more segments as the standard behavior. The shift I was initially imagining was from grouping all control points of a segment extension into a single function call, to putting each control point into its own call., and the difference between segment extension types would be the interval at which a whole segment is complete. So vertex() calls, needing only one control point, add a new segment each time, quadratic points add a new segment every other point, cubic every three, and one could add quartic and quintic in the future that add a segment every four and five. (One could maybe add a generic version where you pass in the order into the first parameter.) Of course, this is all from a frame of mind where chaining is key. I still think this in my own work this is the case that is most common, where paths are composed of many segments. I ideally want the freedom that vertex() has in your proposal, where you can keep chaining more of them without boilerplate, for bezier curves too, since I think in your proposal right now every cubic segment would need to be bounded by begin/endPrimitive. Do you think there's anything we can do in the API proposal to make that use case a bit easier? |
Another thought: if we do go the route of having chains of vertices be all one segment like you're suggesting, maybe curve vertices don't have to be a separate thing, but just an interpolation strategy for those vertices?
So for an API, that would mean quadratic and cubic vertices continue to chain the same way normal vertices do, but you could surround vertices in Sorry if I'm derailing a bit haha, just wanted to share some thoughts that might combine both models! |
This is getting very interesting @davepagurek! I'm going to think about this some more. As if we didn't already have enough to think about, here are some papers that your recent comments inspired me to discover: I've only barely glanced through them, but the first of these seems to provide an interesting framework for a whole class of splines, which includes Bézier splines and elliptical splines (splines made from elliptical arcs) as special cases. They're distinguished by choosing different interpolation functions. The second article seems to develop an approach that's specific to splines made up of elliptical arcs. I need to think through everything again in light of all this discussion, so I'm not sure if these will be relevant yet, but I figured I'd share them in case they inspire ideas from others. |
Good morning again, just adding a summary comment about some of the discussion so far so it's easier to follow!
|
Thanks so much for the summary @davepagurek! I'll add a few thoughts about the existing options and propose a new one. A chaining-only APIOne option is to keep the original proposal, with a few changes.
In this case, we would sacrifice extensibility (e.g. higher-order Bézier curves); in other words, we wouldn't try to solve Problem 8 of the original proposal.1 We’d also lose the simplicity of being able to call any vertex function an unlimited amount of times. Separate APIs for chaining and aggregatingHere I'll add some issues to consider in relation to the idea you recently proposed.
Combined API for chaining and aggregatingBelow, I'll describe one approach that seems to do everything we want in a simple way. It's a variation of the API proposed by @peilingjiang. I'm not sure about it yet, but it seems really nice. The idea is to have four vertex functions:
As in the current version of p5.js, each function call creates a new segment (when the default shape kind is being used), and multiple calls will chain segments. There are two main differences from the current API:
As an initial example, here's how we could create a cubic Bézier segment:
This would be a slight departure from typical p5.js usage, which has users passing coordinates directly into functions, but it's similar to how we already allow users to pass a If we don't immediately see any major problems with this overall approach, I can work out more of the details (e.g. I can redo this list of examples in the new API). Footnotes
|
Thanks @GregStanton! Here are a few thoughts on the different ideas you mentioned.
|
@davepagurek I have some initial impressions but haven't had the opportunity to teach this part of API yet. I'll share some thoughts soon. @tabreturn you know much more about teaching curves than I do. What do you think about the proposal? |
I'll need to make some time to study this properly -- but it looks interesting! |
Hi @nickmcintyre and @tabreturn! The last time you were pinged on this issue, we were in the midst of a fairly complicated discussion. Since then, we've ironed out the main issues, and I've updated the original proposal to summarize the key points. I'm hoping it's easier to digest now. If you have any feedback, that'd be amazing! Also, @peilingjiang, the proposal now includes a function |
Thanks @GregStanton! @shiffman @stalgiag @outofambit, if any of you are interested and have the time, the issue description at the top has been updated with a new proposal. Since the last time you read it, we've reduced the scope of the changes, and they now mainly affect bezier and curve vertices, described in the "What's the solution?" section. |
Just wanted to add an update about this footnote in the issue description! Greg and I took a look at our shape drawing APIs, and as he mentioned, by looking at the number of arguments, we can distinguish between 1.x's
I'll be implementing it in one of those two spots at least, so we only need to decide whether the old API should remain a core feature or not. We also did some checks to see how much would be affected by our breaking changes. We found:
|
Sounds amazing! @davepagurek thank you for being so thoughtful about checking the Nature of Code examples ❤️ ❤️ ❤️ |
This feels like a big step in the right direction! The examples @GregStanton shared seem like they'd be much easier to teach -- maybe draw the curve, then draw a @davepagurek the 1.x compatibility addon seems perfect for making the old API available for awhile. |
Thanks so much for taking a look @nickmcintyre! That's a good observation about (Eventually, we might even consider making |
Quoting this comment from @golanlevin below, for the record. Here, an add-on library author acknowledges possible breaking changes and writes that the new API "is great" :)
|
Hi all, just chiming in to say that the vertex-functions proposal looks very thoughtfully conceived overall — and I look forward to adjusting my new SVG-exporting addon library, https://github.com/golanlevin/p5.plotSvg, as appropriate, in due time. I'm glad to learn of the 1.x compatibility addon for lots of reasons, this is thoughtful. But relative to p5.plotSvg, my library is quite new and it doesn't yet have a large user base. When 2.x comes out I'll probably just shrink-wrap a 1.x-compatible version of p5.plotSvg and start a new branch. |
A new Also, @davepagurek and I continued the discussion about the explicit-first-vertex issue, and we settled on leaving the proposal as it currently stands. Specifically, Béziers start with |
Before we lock in the names of the modes for ProblemAs @davepagurek pointed out, the names Solution?Dave previously suggested Less appealing alternativesI haven't come up with anything better. I don't think Future
|
I like |
This will be in 2.0 as of #7373! @GregStanton do you think I should keep this issue open for a bit for more discussion + any follow up work? |
Thanks @davepagurek! Yeah, I do think it'd be good to keep this open in case we can get a bit more feedback on the latest changes. I just read over all the discussion again, and I think the key points that remain are below. This issueThere are two points that would be nice to discuss further.
|
+1 for It'd be helpful to see a few quick examples of As for @GregStanton @davepagurek FWIW I appreciate the master class you two led while working on this part of the API 🙏🏽 Can't wait to use it! |
Thanks so much for the thoughtful reply and kind words @nickmcintyre! That's a great point about continuing the discussion of A revised API for spline settingsBelow, I've included some examples of what user code could look like, for a sample of spline types, and two possible APIs. In either case, vertices would be specified with For now, I'm using
Beginner-friendliness:
|
Thanks for the detailed examples @GregStanton! I like the idea of configuration objects as the way to handle all the possible future spline types in a single API, and I also lean towards I guess the main downside of putting everything in e.g. One idea: we could have Another possible piece of inspiration could be // Using default splines but adjusting tightness
splineOptions('tightness', 2);
// Using hobby curves with default params
splineMode(HOBBY);
// Using hobby curves with full config
splineMode(HOBBY);
splineOptions({tension: 1, curlStart: 1, curlEnd: 1});
// Using Yuskel curves and changing just the interpolator from the defaults
splineMode(YUSKEL);
splineOptions('interpolator', HYRID); The downside here is that there are now a lot of different ways you could be using Also, maybe a bit off topic, but I think maybe part of the issue with having many overloads like that is that the p5 reference always shows the full description followed by all the examples, so users get exposed to documentation of advanced cases before seeing a possibly quite simple example. It might be possible with our docs setup in 2.0 to not group all examples at the end like that, and allow interleaving. |
This is brilliant @davepagurek! I think you solved the issue: we could just support Updated proposal for spline-configuration APIExample usage is below (adapted from Dave's example). I've changed "options" (plural) to "property" (singular). // Using default splines but adjusting tightness
splineProperty('tightness', 2);
// Using hobby curves with default params
splineMode(HOBBY);
// Using hobby curves with full config
splineMode(HOBBY);
splineProperty('tension', 1);
splineProperty('curlStart', 1);
splineProperty('curlEnd', 1);
// Using Yuskel curves and changing just the interpolator from the defaults
splineMode(YUSKEL);
splineProperty('interpolator', HYBRID); Benefits of
|
We might want to be consistent with other places across p5 where we have gettable/settable properties. In typography, for example, |
Thanks @dhowe! That's very helpful. For spline properties, I was proposing that we support In general, I think it'd be great to make an inventory of features and check it for consistency. It'd actually be nice to do an audit of p5's full API (not just for getters and setters), although it'd take time to sort out what exactly we want to look for, how we want to go about looking for it, and who can actually do it. It's ambitious, but maybe it's not too late for such an effort. |
Hi all! I have a few updates:
Details are below. If anyone has any feedback on any of this, please share! 1.
|
I'm fine with this, however just to note that |
I don't know about the original intention, but a while ago I looked into the units (extra context in the link), and tightness in p5 ( so there's the potential that calling this "tension" might be a bit misleading. I think the intention of this unit change was that negative Since there's an easy conversion between tension and tightness, another option here is to keep tightness for our current implementation, and add tension in the future as an additional way to set the same underlying property? So people trying to be consistent with literature they look up will still be able to, and users who are just trying to use p5 and aren't looking up papers still have the same normalized range as before? Or, alternatively, we can use tension everywhere, and in the future make a new spline mode that is explicitly called catmull-rom (as opposed to the default, which we can maybe call catmull-rom inspired/based/etc) that uses the standard tension definition?
I think this is a pretty important reason to keep this around, and it's not too bad adding |
Thanks @dhowe and @davepagurek! Tightness vs. tensionThe secondary source I cited previously conflicted with the interpretation @davepagurek provided, so I tracked down the original source. What I found is pretty interesting! The original source is a significant paper by Kochanek and Bartels from 1984. It actually uses both of the terms "tightness" and "tension." They first use "tightness" when describing a parameter
Although
Solving this equation for Having said all that, it does seems that "tension" is sometimes used to refer to Summary:
My preference: Response to Dave's questions: Answer: Should we support separate Catmull-Rom and Catmull-Rom-inspired modes? Answer: If we eventually want to make p5's default spline type more configurable, we could use a different name. For example, Kochanek-Bartels splines add configurable continuity and bias parameters to the existing tension/tightness parameter. (This is actually described in the paper I cited above.) But I don't expect we'd do that, and fewer people will have heard of Kochanek-Bartels splines. So even then, we probably wouldn't need to rename the default type; we could just make a brief comment in the reference with a clarification of which generalization we're actually using.
|
@GregStanton thanks for digging up more sources on that! Cool to see a SIGGRAPH paper out of Canada's National Film Board and also the University of Waterloo, where I did my undergrad 😅 Your conclusions all sound to me like the most reasonable options here. I added a plural version, |
Increasing access
Implementing this proposal would
As a result, p5.js would be more accessible to beginners, while offering others access to a wider feature set.
Which types of changes would be made?
Most appropriate sub-area of p5.js?
What's the problem?
Short version
The current API for vertex functions contains complexities, inconsistencies, and hard-to-extend features. It's included below for easier comparison with the proposed API.
Long version
Eleven distinct problems arise from the current API for vertex functions. Below, each problem is listed and tagged with a problem type. (In case we want to modify the proposed solution, we can check the new version against this list.)
vertex()
but not toquadraticVertex()
,curveVertex()
, orbezierVertex()
, as noted in #5722. Under the current API, specifying texture coordinates would require up to fifteen parameters in a single function call, with the following syntax:bezierVertex(x2, y2, z2, u2, v2, x3, y3, z3, u3, v3, x4, y4, z4, u4, v4)
. [Inconsistency/Inflexibility]quadraticVertex()
andbezierVertex()
really do start withx2, y2
/x2, y2, z2
. That's because the first set of parametersx1, y1
/x1, y1, z1
must be specified separately, and we need a way to distinguish the second, third, and fourth points since they're grouped together in the same function call. [Complexity]x1, y1
/x1, y1, z1
gets passed tovertex()
. But that's just for Bézier curves. If we want to make a Catmull-Rom spline, the first point and all successive points get specified directly tocurveVertex()
. In other words, the current API allows the user to callvertex()
to start and continue a polyline, and to callcurveVertex()
to start and continue a Catmull-Rom spline; however, to start a shape with a quadratic or cubic Bézier curve, the user needs to mix commands. [Inconsistency]bezierVertex()
is named after a single vertex, but it accepts coordinates for three points. We may try to reconcile this by identifying the first two of those points as "control points" and the last point as a "vertex," but this is an uncommon distinction, and it doesn't hold up: we cannot reconcile it with the meaning of "vertex" incurveVertex()
. [Inconsistency]bezierVertex(x2, y2, x3, y3, x4, y4)
andbezierVertex(x2, y2, z2, x3, y3, z3, x4, y4, z4)
. Each of these signatures looks rather complicated by itself. [Complexity]bezierVertex()
andbezier()
are supported, andcurveVertex()
andcurve()
are supported. We also havequadraticVertex()
, butquadratic()
is not supported. (I've proposed separately that we should introduce a new functionarcVertex()
to address a similar inconsistency.) [Inconsistency]quadraticVertex()
is confusing for multiple reasons. BothquadraticVertex()
andbezierVertex()
produce Bézier curves, but only one of them contains the name "Bézier"; in other words, one function is named after the type of curve while the other is named after the order of the curve. Also, many p5 users are students who will have recently learned in algebra that "the vertex of a quadratic" (a parabola) corresponds to its lowest or highest point; that's not generally true of a vertex specified withquadraticVertex()
. [Inconsistency]vertex()
and specify the next four withquadraticVertex()
, we only have five vertices. We could potentially usevertex()
to specify the sixth vertex at the end, but this is inconsistent with howvertex()
is used everywhere else, and it still requires us to mix command types to create a single primitive. [Inflexibility]curveVertex()
uses a general term for a special shape, which leads to inconsistencies. For example, in the p5 reference, “Curves” is used as a general section heading that includes Béziers, whilecurve()
specifically creates Catmull-Rom splines only. This may lead to confusion. For instance, if we introduce a function such ascurveType()
for different spline implementations, it will be hard to guess whether it applies to all curves or only curves made withcurveVertex()
. [Inconsistency]What's the solution?
The main idea is to pass only one vertex into each vertex function.
Proposed API
The proposed API is indicated below.1 A more extensible API for handling multiple spline types, as well as type-specific properties such as tightness, is currently under discussion. The discussion includes a sample of spline types and a new syntax.
Amazingly, this simple API solves all eleven problems listed above 🤯The basic design was inspired by this comment from @davepagurek.
Notes:
bezierOrder()
eliminates the need forquadraticVertex()
arcVertex()
is based on the latest iteration of #6459splineVertex()
replacescurveVertex()
2Code examples
Example 1: Consistency across all types of curves
Note: Conceptually, polylines can be viewed as linear splines or as chained first-order Béziers.
Example 2: Clear function names, readable parameter lists, and flexible design
Note: Chaining quadratic and cubic Bézier curves does not currently work in p5, but issue #6560 aims to address this.
Example 3: Consistency across all chained shapes (curves, triangles, and quads)
Note: Bézier control points are rendered below as a visual aid, but the code for that is not shown.
Pros (updated based on community comments)
arcVertex()
, rather thanvertex()
. Same for other curve types.Cons (updated based on community comments)
Proposal status
Under review
Updates
This proposal has been updated to reflect some small adjustments, based on the discussion in the comments section:
bezierOrder()
was added so that chained segments of different orders can be distinguishedarcVertex()
was added based on the current consensus in #6459curveVertex()
was replaced withsplineVertex()
based on Problem 11, which was added to the problem listsplineProperty()
was added as a more extensible way to set spline properties (this replacescurveTightness()
)Additional code examples have also been included.
Footnotes
We may also want to support texture coordinates when a
z
-coordinate is not passed. This is currently supported byvertex()
(see the last example fortexture()
in the reference); however, it requires a separate signature forvertex()
that is not documented on its reference page (i.e.vertex(x, y, [u], [v])
). We'd need to discuss whether the benefits justify the extra complexity (namely, documenting and supporting an extra signature for every type of vertex function). ↩For consistency, we could also rename
curve()
asspline()
. ↩We can distinguish new and old usage of
bezierVertex()
by detecting the number of arguments, and we can deprecate the old usage. We could deprecatequadraticVertex()
andcurveVertex()
entirely with the @deprecated tag in the inline documentation. ↩The text was updated successfully, but these errors were encountered: