Add compatibility addon for shape drawing

[davepagurek]

• Restore quadraticVertex
• Restore bezierVertex (already done -- we were able to make the old syntax available in 2.0.)
• Restore curveVertex (alias to splineVertex)
• Change default spline ends mode to not interpolate ends
• Restore curveTightness
• Default endContour() with no args to CLOSE
• Disable loop closing behaviour with endShape(CLOSE) when curveVertex() is used

[GregStanton]

Hi @davepagurek! I'm not sure if we discussed keeping vs. removing the old bezierVertex() syntax. I did find an old comment of mine laying out some pros and cons. I'll add just a few things to the pros and cons I already listed.

Since the purpose of the new API is to make the vertex functions easier to understand, I'm thinking that a reference page documenting both the old and new syntax would be problematic. Were you thinking we'd link to the 1.x reference page for people who want to continue to use the old syntax? Using documentation for an old version to document a feature that's still in the current version feels like it could cause problems down the line.

Also, we're already removing quadraticVertex(), which means that the new bezierVertex() supports quadratic and cubic cases, but it'd only support the old syntax in the cubic case. It'd be fairly confusing.

I do understand the desire to reduce breaking changes as much as we can, but I'm not sure if it makes sense here. I'm open to discussing it more. What do you think? I've been going through updates that'll need to be made to the reference, so I thought it'd be good to ask now rather than later.

[davepagurek]

I think that's a pretty good reason to move the old bezierVertex(x1, y1, z1, x2, y2, z2, x3, y3, z3) overload out of 2.0 and have it live in the compatibility addon, especially now that we have a clearer idea of how easy it will be to add in the shape compatibility addon. Just to confirm, @limzykenneth @ksen0 does this sound OK to you too?

[davepagurek]

We actually already handle the old syntax in the shapes addon here, so all we'd need to do is remove it from dev-2.0.

p5.js-compatibility/src/shapes.js

Lines 25 to 42 in 4b6108c

	fn.bezierVertex = function(...args) {
	this.bezierOrder(3);
	if (args.length === 6) {
	const [x1, y1, x2, y2, x3, y3] = args;
	oldBezierVertex.call(this, x1, y1);
	oldBezierVertex.call(this, x2, y2);
	oldBezierVertex.call(this, x3, y3);
	} else if (args.length === 9) {
	const [x1, y1, z1, x2, y2, z2, x3, y3, z3] = args;
	oldBezierVertex.call(this, x1, y1, z1);
	oldBezierVertex.call(this, x2, y2, z2);
	oldBezierVertex.call(this, x3, y3, z3);
	} else {
	throw new Error(
	`bezierVertex() was expecting either 6 or 9 arguments, but it was called with ${args.length}.`
	);
	}
	}

[limzykenneth]

I've not been following the vertex implementation very closely and I'm a bit confused about a few things. From what is currently in dev-2.0 branch, quadraticVertex() is still around, is it planned to be removed? Although its functionality has been superceded by bezierVertex() with bezierOrder(), I would suggest in this case keeping it around if it does not break the rest of the implementation. For quadraticVertex() I see value in keeping its documentation around for 2.0 as well with maybe a deprecation note to use the newer syntax.

For older bezierVertex() arguments list, if it has already been removed in dev-2.0, we will restore the functionality with the compatibility addon, we don't need to document features restored with compatibility addon in the 2.0 reference as the point of the compatibility addon is to restore 1.x functionality so the user should be referencing 1.x reference if they are using this addon.

Also I'd like to prevent any additional breaking changes if at all possible at the beta stage since I will be opening issues with other addon libraries about updating their library, if the API is still unstable it would be difficult to accurately gauge breakage with them. If there are things that still need to be removed from dev-2.0 do complete them sooner rather than later so that I can do another beta release.

[davepagurek]

I see value in keeping its documentation around for 2.0 as well with maybe a deprecation note to use the newer syntax.

This approach also makes sense, although currently it's a little difficult on a technical level to put a deprecation note next to a specific overload in the list of overloads at the bottom -- it would currently have to be mentioned in the body of the description. quadraticVertex we can keep + add @deprecated to since the whole thing is deprecated, so it's just bezierVertex where this applies to a specific overload.

I think it's possible within JSDoc, since each overload is like its own doc comment, but we don't handle it yet in the schema on the website repo, so I could update our doc generating script in this repo + edit the website source code to handle per-overload deprecations if we want to go this route.

[limzykenneth]

Yes, description for any deprecation note is fine for now.

[GregStanton]

I've not been following the vertex implementation very closely and I'm a bit confused about a few things. From what is currently in dev-2.0 branch, quadraticVertex() is still around, is it planned to be removed?

Thanks @limzykenneth for pointing this out! I'll share some context and updates.

Deprecation vs. compatibility add-on

Community feedback

We offered two options in the discussion for the new vertex API:

1. Keep the old API in 2.0 and deprecate it
2. Move the old API into the compatibility add-on

We never made a final decision. However, as Dave noted in the linked comment, we did some research into the impact of removing the old API, and we found it would be minimal. When we looked at OpenProcessing and The Nature of Code, for example, "We found very little use of the changed methods compared to single standalone curves like bezier()" (quoting Dave here). In fact, not a single example in Nature of Code would be affected. This may be partly because these are relatively advanced features, partly because of the complexity of the legacy API, and partly because the vertex versions did not allow Bézier curves to be mixed with other curve types.

We also reached out to multiple library authors regarding the new API, as well as the advisory committee. We received responses in all cases (1, 2, 3). None included a request for the old API to be kept in 2.0 itself. On the other hand, there was a comment indicating support for using a compatibility add-on.

Based on all this, I think it would be okay to go with the compatibility add-on rather than deprecation (unless feedback from the Google Form survey provides new information).

Analysis

For what it's worth, my own thoughts on the matter are as follows. The purpose of the new API is to make the vertex functions easier to understand, and keeping the old API in 2.0 would partly defeat that purpose.

• For anyone not using the old API, it will be a distraction.
• For anyone who wants to continue using the old API, it may also be more confusing to keep it in 2.0. Skimming the main reference page would suggest to them that quadraticVertex() is deprecated, but bezierVertex() is not. When they dig deeper, they'd find a full reference page for the old usage of quadraticVertex(), but they won't find any details on the old usage of bezierVertex() (unless we want to mix old code examples with the new ones, which would be especially confusing).
• For contributors, keeping the old API would also increase confusion, since it'd increase code complexity.

On the other hand, if we use the compatibility add-on, the change will likely impact a very small number of users. All public feedback so far indicates this group is okay with removal, and they'll be able to use a compatibility add-on if they want to keep using the old API. This may be less confusing, since the compatibility add-on could direct them to the website for 1.x, where the legacy API is documented in full, without being mixed with 2.0 features.

Vertex implementation cleanup

A cleanup phase is still planned for the custom-shape refactor, including the removal of the Vertex submodule (vertex.js), in favor of the Custom Shapes submodule (custom_shapes.js). Right now, the vertex functions are split across these two files, which I think would result in two separate sections appearing in the website's reference pages. There are functions in Vertex that we're keeping, so as part of the documentation updates, I can move those over to Custom Shapes before removing Vertex.

[limzykenneth]

I'm fine with removing the legacy API in 2.0 and reimplementing it in the compatibility addon. The main thing is that the legacy API is still in the 2.0 betas and people are testing against it (both sketches and addons) so if they are to be removed, I would like it to be done as soon as possible so we don't create false impression on what to expect in this part of 2.0.

It would also be removing the reference of the removed legacy API so that it is clearer on your documentation project that this part don't need updates.

[davepagurek]

if they are to be removed, I would like it to be done as soon as possible so we don't create false impression on what to expect in this part of 2.0.

If we're ok taking it out then I can remove that bezierVertex overload and the old quadraticVertex implementation this afternoon. I guess that means we should also do another beta release shortly after?

[GregStanton]

By the way, did you consider endShape(CLOSE) and endContour()? I think there are two other breaking changes to these:

1. When using endShape(CLOSE) to close a shape consisting of spline vertices, the spline will close smoothly in 2.0 but not in 1.x.
2. When ending a contour with endContour(), it'll be open by default in 2.0 but closed by default in 1.x.

[davepagurek]

Good catch, I added those to the to do list in the issue! I'll make an update soon.

[davepagurek]

Ok added those in #7. I'm going to close this issue for now and we can make new ones if we find new things to handle

[GregStanton]

Hi Dave! Not sure if you'd want to start a new issue, but I though maybe we could reopen this one, to address some other breaking changes we made. It's possible that, like the removal of the old syntax for bezierVertex(), we originally planned some of these changes but never completed them. I can check later but wanted to put this on your radar ASAP.

Removal of curve()

We planned to remove this in favor of a new spline() function, to be consistent with how we removed curveVertex() in favor of splineVertex().

Removal of beginGeometry()/endGeometry()

I suppose this could go in the shape-drawing add-on, since these features are under the Shape section of the reference.

Generalization of curveDetail() and removal of bezierDetail().

After removing curve(), curveDetail() needs to be handled. Calling it curveDetail() and having it affect only splines no longer makes sense. If it were to continue to affect only splines, it could be named splineDetail(). But, this might confuse users. It's really a rendering property, not a property that's intrinsic to the spline, but users would likely wonder why it's not set with splineProperty(). At the same time, the fact that the detail level isn't intrinsically a spline property is a reason not to set it with splineProperty().

I think these problems are solved by an idea we discussed: generalize curveDetail() so that it replaces a count with a density. In addition to solving the immediate problems, I think this would simultaneously reduce the size of the API, make it more powerful, and (possibly) make it easier to understand. Now that the generic "curve" term is no longer used to refer specifically to splines, it will be understood that curveDetail() applies to all curves, including curves made with bezier(), spline(), bezierVertex(), and splineVertex(). This name also makes more sense since detail is really a property of how any kind of curve is rendered, rather than being a property that's specific to Bézier curves or splines. Getting all these benefits with one change seems like the sign of a good design.

We could definitely discuss it more, but if that all makes sense, we could modify curveDetail(), remove bezierDetail(), and then add the old versions back in using the compatibility add-on. I guess. And I could document the updates.

Unresolved issues with detail more generally, and with 2D primitives

There's a whole can of worms that was in the shape-overhaul to-do list. I was reminded of it when I started looking at the detail features. My first thought was that 2D and 3D primitives currently take details as counts, so specifying detail as a density for curves introduces an inconsistency.

But then I realized that the current detail features in both 2D and 3D are pretty inconsistent and very buggy. Contributors moved p5 forward by adding those features, but they need some loving care. Fixing them might require breaking changes to parts that are currently working, so it'd be nice to address these issues before those changes get harder to make.

We previously made a table that systematically documented a lot of these problems, and we planned to address them as part of 2.0, but I think we got busy with other things. Here are examples of the issues, including some problems with 2D primitives more generally:

1. Inconsistency: Some 2D primitives support detail in WebGL mode, but not all. For example, ellipse() does, but circle() doesn't. Similarly, rectangles do according to the reference, but square doesn't.
2. Inconsistency: Triangle doesn't work in WebGL mode at all.
3. Inconsistency: Point allows vector arguments, but the other primitives don't.
4. Complexity: Although it's not a bug, the 2D primitives can take up to 14 parameters, which makes some of them extremely hard to read.
5. Bug: None of the examples in the reference demonstrate detailX or detailY for rect(), but we can modify them to see what happens. For rectangles, we get rounded corners instead of detail (I don't think it's possible to distinguish detail arguments from rounding arguments, so this bug is inherent to the API).
6. Bug: For quadrilaterals, you either get nothing (e.g. if the detail is set to 1), or you seem to get only part of the stroke.
7. Bug: The problems with detail aren't actually limited to 2D, it seems. The same problem with quads happens with box(), and I think box() should really have detailX, detailY, and detailZ, similar to widthSegments, heightSegments, and depthSegments for BoxGeometry in three.js.

This is a lot to digest, but we might be able to find a simple solution to most of these issues. There are a lot of potential solutions, I think. To get the ball rolling, here's one idea that might be worth looking into: What if we remove the detail parameters from all 2D primitives and instead use curveDetail() to set their levels of detail? I think this would fix a lot of issues, and most or all of the other issues could be solved after the release of 2.0, since they might not involve breaking changes. Also, I don't think it'd take long to figure out which changes could be made after the release.

(This would break some existing code, e.g. the detail on ellipses, which I think does work. But maybe the features that do work might be added back with the compatibility add-on.)

[davepagurek]

Thanks @GregStanton!

curve() --> spline()

This should be a quick change, so I can update that.

Merging curveDetail()/bezierDetail()

The curveDetail method is now used for beziers too, so we can remove the old bezierDetail function from the codebase, as it doesn't do anything any more. I'm ok with keeping the term "curve" as a generic name for everything now that it's no longer used for splines.

For the compatibility addon, though, I think it's not actually feasible to map these back to the old functions without a large amount of work changing the internals of the new Shape class, so I'm thinking this might be one of those instances where we just accept imperfect backwards compatibility? This could mean trying to do a very approximate mapping from points to density based on a default expected length of each curve, which is not going to be very accurate, but at least would mean your sketch doesn't run super slow because curveDetail(10) to use 10 points in 1.x doesn't start doing 10 points per unit length in 2.0 with this addon included.

Using curveDetail for 3D geometry

I think that's a good idea, although the implementation is unfortunately not a quick change due to most of these 3D primitives being created right now by manually putting vertices in a p5.Geometry. So while this is likely the direction that resolves the most issues in one go, it's probably not going to be feasible to make that change in time for 2.0. Maybe this is the kind of thing where we can deprecate but still support the old detail parameters while moving towards curveDetail in a future (2.1?) release.

[GregStanton]

Hi @davepagurek! Thanks for your quick reply! Before I forget, here's another issue that's left over from our previous discussions:

New Bézier closing behavior

This is an issue we discussed, but I don't think we ever totally settled on it. Here's a summary of our discussion:

Proposed change: it may be good to close Bézier curves with Bézier curves.

Motivation: To draw a full ellipse with arcVertex() (once we implement that), we could CLOSE the curve; then polylines, splines, and arcs would be closed with line segments, spline segments, and arc segments, respectively. So it may be good if we could close Bézier curves with Bézier segments. Composite curves could all be closed with line segments.

A possible implementation: Suppose we have a Bézier segment formed by vertices V0, … , Vn. We could maybe reflect the intermediate control points V1, … , V(n-1) over the line joining V0 and Vn, to get CV1, … , CV(n-1). In other words, the line joining V0 and Vn would be the perpendicular bisector of the line joining Vi and CVi for all intermediate values of i. Then maybe we could close the original Bézier with the Bezier specified by Vn, CV(n-1), … , CV1, V0, in that order. Here's a small example sketch to illustrate what such a closed curve could look like.

This would be pretty easy to implement, I think. We also discussed trying to get a smoothly closed curve, but there were some subtleties with that, and I'm not certain if it's necessarily better? I'm not sure if I have a record of that conversation. We could potentially flesh that out again, though.

[GregStanton]

Now I'll reply to your comments :)

Quick points

1. Sounds great about curve() -> spline()!
2. Did you see the point about the removal of beginGeometry()/endGeometry(), and whether to add it back in with the compatibility add-on?
3. Just to clarify, you want to go forward with having curveDetail() set the detail for bezier(), spline(), bezierVertex(), and splineVertex()?
4. About supporting the old curveDetail() with the add-on... I guess if we at least maintain backwards compatibility in an approximate way, like you mentioned, then that may be enough since we're also going to be providing the community with plenty of notice.

curveDetail() for 2D primitives, not 3D primitives

I actually wasn't proposing we use curveDetail() for 3D surface primitives, or for geometries in general. I'll explain the problem I was trying to solve, and my proposed solution.

Problem: Some of the 2D primitives (ellipses, quads, ...) have detail parameters when they're used in WebGL mode, but in multiple cases, those parameters don't actually work. In at least one case, the problem isn't the implementation; the API actually makes it impossible for the feature to work. Due to the nature of the problems, we might either have to fix them now, or wait until a 3.0 release. I need to look at it a little more closely to understand the different possibilities.

Solution: I'm not sure yet, but we might be able to solve the issues now, by replacing the detail parameters in the 2D primitives (currently, these are counts) with curveDetail() (specified as a density). In other words, curveDetail() could apply to all curves, whether they're 2D curve primitives or custom curves made with beginShape()/endShape(). This approach has three possible benefits:

1. It'd fix bugs in 1.x, some of which aren't fixable without breaking changes.
2. It'd provide a kind of consistency: curve details are always specified as densities, whereas surface details are always specified
   as counts.
3. Leaving 3D surfaces alone reduces the scope of the change, so it's more likely we could squeeze it into 2.0.

If you think this is promising, I can flesh out the details... no pun intended! I suppose the main hurdle would be computing arc lengths, but I'd love to do the mathematics for that, and I think I could probably do it fairly quickly. (There are also other possible solutions I might briefly explore.)

Extras

A few disclaimers about using curveDetail() for 2D primitives:

• There are still other problems with the 2D primitives that wouldn't be fixed by this, but those might be fixable without breaking changes, so they could maybe be fixed later.
• I haven't thought about whether we could maintain backwards compatibility, but if no one ever complained about the fact that several of the 2D detail features don't work at all, I suspect a breaking change to the functioning features wouldn't cause too much disruption.

Also, I did discover at least one bug in the detail features for the 3D primitives. But I think it might be possible to fix such issues after 2.0, as they don't seem to involve breaking changes. If we want to switch 3D over to densities for consistency, e.g. in a possible p5.js 3.0, then that could also happen later.

[davepagurek]

Just made two PRs (links shown above this comment on Github):

• Renames curve() to spline() in p5
• Removes the old unused bezierDetail() in p5 (the new curveDetail() implementation already had it used for all bezier and spline methods)
• Updated the compatibility addon to restore begin/endGeometry
• Updated the compatibility addon to restore curve()
• Updated the compatibility addon with an approximate restoration of curveDetail/bezierDetail

For shapes/detail: I definitely think this is really promising! I'm a little hesitant only because we'll have less time for community feedback and testing for bugs/performance, given also that there are other things that have to get done too.

There are also a few details to iron out, e.g.: curveDetail makes a lot of sense for arcs/circles/rounded rectangles, as those changes are immediately visible. Detail for an already straight line like in a non-rounded rect() or plane() is done normally because one wants to move those vertices in a shader. But if you aren't using a shader, you probably don't want the performance hit of subdivision of straight lines. So it might mean that curveDetail shouldn't apply to those by default, and maybe there's a separate lineDetail?

But also I believe this is true:

if no one ever complained about the fact that several of the 2D detail features don't work at all, I suspect a breaking change to the functioning features wouldn't cause too much disruption.

That might justify allowing a breaking change in a minor version, if the benefit is high, the current implementation is not fully functional everywhere, and the change doesn't affect much? Another way to reduce the affected sketches even further is to only use curveDetail for now once a user has called it in their sketch, and otherwise we fall back to the current defaults. @ksen0, do you have any thoughts on how we might slot in a change like that?