Add compatibility addon for shape drawing

[davepagurek]

• Restore quadraticVertex
• Restore bezierVertex
• Aliases old curve* methods to the new spline* ones:
  • Restore curveVertex
  • Restores curve()
  • RestorescurvePoint
  • Restores curveTangent
• 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
• Restores begin/endGeometry
• Adds an approximate conversion from the old curveDetail/bezierDetail units of number of points to the new curveDetail units of samples per unit length
  • Note that these now the edit the same state, and this library doesn't split them back into two different ones as we think that's enough of an edge case to not justify the implementation complexity.

[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