Update docs for custom shapes and related features

[GregStanton]

trafficstars

Issues

Addresses #6560 Addresses #6766

Pull requests

• #7276
• #7373
• #7471
• #7528
• #7583
• #7600
• #7601
• #7656
• #7703

Changes

This PR updates docs for user-facing changes to custom shapes and related features.

Custom shapes:

• [x] Move vertex.js into custom_shapes.js
• [ ] Update vertex() docs
  • New signature x, y, u, v
  • There's an example of this in ref page for texture(), but not yet documented on ref page for vertex()
  • relevant PR: #7373
• [ ] Document bezierOrder()
  • Might update examples in quadraticVertex() and move them here
  • relevant PR: #7373
  • relevant issue: #6766
• [ ] Update bezierVertex() docs
  • located in custom_shapes.js (after removal of vertex.js)
  • new x, y, u, v and x, y, z, u, v signatures
  • maybe just show default usage, link to docs for bezierOrder()?
  • relevant PRs: #7373, #7600
  • relevant Issues: #6560, #6766
• [ ] Remove quadraticVertex() docs - relevant PRs: #7373, 7600 - relevant Issue: #6766
• [ ] Document splineVertex()
  • start from curveVertex() docs
  • new x, y, u, v and x, y, z, u, v signatures.
  • spline ends are now interpolated by default (show non-default behavior in splineProperty() and link to it?)
  • splines now close smoothly
  • relevant issue: #7373
  • relevant issues: #6560, #6766
• [x] Remove curveVertex() (it's already supported by the 1.x compatibility add-on for shapes)
  • relevant PR: #7373
  • relevant Issue: #6766
• [ ] Review vertexProperty() docs
  • located in custom_shapes.js (after removal of vertex.js)
  • relevant PR: #7276
• [ ] Document splineProperty()
  • might start from existing docs for vertexProperty(), curveTightness()
  • relevant PR: #7471
  • relevant Issue #6766
• [ ] Document splineProperties()
  • relevant PR: #7528
  • relevant Issue: #6766
• [ ] Update beginShape() docs
  • remove reference to quadraticVertex()
  • PATH is the new name for the default shape kind
  • composite paths can now be made from mixed vertex types
  • relevant PR: #7373
• [ ] Update beginContour() docs
  • contours are now general subshapes, essentially
  • they can now be created with a kind parameter, just like shapes.
  • PATH is the default kind.
  • composite shapes can be made from mixed contour types
  • Mixing contour kinds might not work in WebGL yet
  • relevant PR: #7373
• [ ] Document new endShape/Contour(CLOSE) behavior for splines
  • splines now close smoothly (may just want to mention this and link to splineVertex()?)
  • relevant PRs: #7373, #7583, #7601
  • relevant issues: #6560, #6766
• [ ] Update endContour(CLOSE) docs
  • see custom_shapes.js
  • Contours are now open by default (default is OPEN)
  • relevant PRs: #7373
  • relevant issues: #6560, #6766 Curves:
• [ ] Update curve() docs to reflect name change to spline()
  • relevant PR: #7656
• [ ] Remove curveTightness() docs
  • relevant PR: #7471
• [ ] Update curvePoint() docs to reflect name change to splinePoint()
  • relevant PR: #7703
• [ ] Update curveTangent() docs to reflect name change to splineTangent()
  • relevant PR: #7703
• [ ] Update curveDetail() docs, possibly incorporating adapted examples from bezierDetail() docs
  • Replaces count with density, so that it works more generally (including for custom shapes, I think?)
  • See visitors for Béziers and splines in custom_shapes.js
  • Note that the user-facing function is currently located in 3d_primitives.js (it should be moved to curves.js)
  • relevant PR: #7373
• [ ] Remove bezierDetail() docs
  • relevant PR: #7656

3D:

• [ ] remove beginGeometry()/endGeometry() docs
  • Removed from 3d_primitives.js, in favor of buildGeometry().
  • Summary from the PR: "Removing beginGeometry()/endGeometry() eliminates an inconsistency with beginShape()/endShape(), reduces the size of the user-facing API, and prevents confusion (endGeometry() is effectively used as a constructor, in contrast with how objects are usually created in p5)."
  • relevant PR: #7373

Notes

• Except where noted above, reference dependencies[^1] can be updated in a separate PR.
• A simple change to behavior of endShape(CLOSE) for Bézier curves was discussed; decided we can implement this for 2.0

Screenshots

[Reminder] Might be helpful to provide screenshots of top-level listings from main reference page.

PR Checklist

• [ ] npm run lint passes
• [ ] Inline reference is included / updated
• [ ] Unit tests are included / updated

[^1]: Here, reference dependency refers to a reference page that depends on one of the new or changed features mentioned under the Changes section, either because it mentions it or because it uses it in a code example.

[ksen0]

Hi @GregStanton ! Quick update here: I've added minimal documentation and fixes to example code here: https://github.com/processing/p5.js/pull/7749 Please feel free to revise the text in any way that makes sense - though I believe everything is in there, some points could be much more clear. Thank so much for your work on this!

[GregStanton]

Sounds good @ksen0! Thank you so much for putting those docs together. Since all tests are passing in this code reorg, would it make sense for me to copy over your docs, and then make any revisions to them here? I'll hold off on that for now, in case it's easier for you or @davepagurek to review the reorg by itself, without extra commits.

Future beginner-friendly issue to improve test coverage and structure

Maybe after the docs are updated, we could make an issue with the "Good First Issue" and "Help Wanted" labels, so that we can give beginners an opportunity to improve the coverage and structure of tests for custom shapes? (Maybe it could work as a good first issue if we provide some support in case reorganization breaks things.) I'll record a few notes for that potential issue below, before I forget what the problems are. I wouldn't mind if you skip them for now 🙂

Coverage

One thing I noticed when updating imports and tests is that we seem to be lacking full test coverage in test/unit/core/vertex.js.

The 1.x version only has tests for beginShape()/endShape(), vertex(), quadraticVertex(), bezierVertex(), and curveVertex(). But there are several other functions in the 1.x Vertex module: beginContour()/endContour() and normal(). Based on a search of 1.x, it looks like normal() is the only one of these with a unit test, but for some reason it's in its own file in test/unit/webgl/normal.js. The 2.0 version is also missing tests for some functions (at least in test/unit/core/vertex.js), and it doesn't do parameter checks like 1.x does (although perhaps there was a reason for that?).

Structure

There's also some unnecessary friction for contributors, since the file structure of the unit-test directory doesn't reflect the new file structure for 2.0. For example, under src, shape was a subdirectory of core in 1.x but isn't in 2.0; however, the unit test folder in 2.0 still has shape under core. (The 2.0 file structure makes the organization of src more consistent with the organization of the main reference page on the website, which is helpful for new contributors who already use the website reference. So I think we'd want to change the file structure of test/unit rather than src).

[davepagurek]

@GregStanton copying the updated docs to your branch sounds like the right next step! if you have the time, feel free to start that process, and I'm also happy to help after the Easter weekend too! I think I should be OK reviewing regardless of the extra commits.

[ksen0]

Hi both! @GregStanton yes that seems like a great way forward. Please don't feel like you need to keep any of the text I added, you're welcome to organize it as best makes sense. Merging the dev-2.0 branch into this might be a challenge, I see there's some merge conflicts, so please feel free ping if you need help with that, either on Discord or via public office hours!

Test coverage

Yes that would be great! Your notes on coverage make total sense. I support reorganization to improve how navigable the codebase it, but for bulk changes (including on test suite), it would be better to keep reorg separate from substantive changes.

Additionally, I think it might be good to include recent bugs that were patched that touch the Shape API (e.g., https://github.com/processing/p5.js/pull/7750) as potential inspiration of what a test might cover.

Finally, a rule of thumb is that if a function was moved from one file or another, just make sure there's 1 more meaningful test added. Really important these aren't AI-generated, because a good test suite covered both very typical and sort of odd cases, and AI might generate OK typical cases, but is unlikely to generate good strange cases.

If I understand all this right, the test coverage issue can already be open and available for work - it's totally independent of this documentation work?

[GregStanton]

Thanks @davepagurek! I'm going to get to work on this as soon as I can.

@ksen0: Thanks! I'll let you know if I need help with conflicts. Also, great points about using #7750 as a model, and about the rule of thumb for moving functions. I can mention that in the test issue if I'm the one to open it, along with the advice regarding AI.

Makes sense about doing the reorg and the new tests separately. Seems like a good case for GitHub's new sub-issue feature. There could be one issue for the shape tests, with a sub-issue for the reorg and another sub-issue for the new tests. That way, the work can be made separate but can still be coordinated easily.

If I understand all this right, the test coverage issue can already be open and available for work - it's totally independent of this documentation work?

It's independent technically, but I think there are blockers. Basically, some of the shape docs don't fully describe the new behaviors, and it looks like some of the descriptions are incorrect. I'm thinking that anyone writing tests may want access to complete, correct docs. So I figure the current PR should be completed and merged first.

[ksen0]

Hi @GregStanton, are there updates on this / is any support needed? I saw it linked on one of the recent comments so wanted to bump it!

[GregStanton]

Hi @ksen0!

I'd just need a bit of funding support if we want to fast-track this, so that I can spend more time on it. Otherwise, I see a couple of options.

1. I can get to this as soon as I reasonably can, outside of my regular work hours. I'm not sure how soon that will be.
2. I can share a short draft explanation of remaining problems, and allow edits to this PR by maintainers, so that others could pitch in.

It's probably good to avoid having someone else start from scratch, since I already did the file reorg in this PR.

Draft: A few documentation issues to fix (starting with splines, for now)

I'll focus on splines for now, since that seems to be the most pressing issue.

Problem: With the old curveVertex() feature in 1.x, it used to be that when a user included four vertices, only the middle two were connected. While this is still possible with splineVertex(), it works much more simply by default. This simplification isn't accounted for in the new description or code examples, so the description of splineVertex() is incorrect, and the examples don't work as intended. This problem extends to the docs of other spline-related features.

Solution: The new design in 2.0 eliminates the complexity of the old design. It's much more intuitive. Here's how it works: splineVertex() draws a smooth curve through the points you give it. That's the whole explanation. We can talk about customizations on the splineProperty()/splineProperties() pages, including the ends property that makes it possible to achieve the old behavior. With this background in mind, here are some of the specific updates to make:

1. Defaults: Document the default value of the ends property (the default is INCLUDE), at least on the splineProperty() page. The splineProperties() page can link to that.
2. Links: Replace the broken link to curve() with a link to spline(), on the splineVertex() page.
3. Examples: Fix spline() examples.
4. Examples: Fix splinePoint() examples.
5. Examples: Fix splineTangent() examples.
6. Examples: Add code example for the ends property to the splineProperty() page.
7. Language: Typos, language cleanup [^1] (since language complexity and typos affect accessibility)
8. Language: Revise top-line descriptions that appear on the main reference page? For example, for splineVertex(), we might replace "Adds a spline curve segment to a custom shape" with "Connects points with a smooth curve (a spline)." This is less pressing, but a more beginner-friendly explanation focused on the main use case may improve discoverability significantly.

There are other issues that I could write up when I get more time, but that's a start! I'll try to make time for a more systematic review when I can.

[^1]: There are various typos that affect readability, e.g. on the splineProperty() page. There are redundant lines on that same page where some defaults are described more than once. The top-line description of splineProperty() says it "sets the property of a curve" (old terminology) instead of "sets the property of a spline" (splineProperty() doesn't apply to all curves in the Curves section of the reference). Etc. That's just one page, to give the idea.

[ksen0]

Thanks for the update @GregStanton !

The list of examples and language errors you provided already seems really helpful actionable, thank you for putting that together, would you mind if we (the maintainers) took that on in a separate PR?

From what I understand, this PR involves some reorganization internally independent of the above documentation problems; and already has merge conflicts that will need to be resolved, so unless I'm missing something, a separate PR targeting only the list above would not create more work here, for when you are able to get to it?

Let me know if I missed something. Thanks again.

[GregStanton]

Hi @ksen! Yeah, in hindsight, I'm not totally sure how the file reorg affects things. Whatever you think would move this forward most quickly is cool with me. If there's a little rework or something for me, it's not a huge deal. My main concern is that users visiting the beta site don't get the wrong idea based on the current docs. Thanks so much for your attention to this!

[ksen0]

Thanks for the quick follow-up! I'll prioritize it for 2.1, because you're absolutely right about beta site docs.