-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
A few documentation issues to fix #8031
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
base: dev-2.0
Are you sure you want to change the base?
Conversation
Update p5.Geometry.js
Update README.txt
Revert "Update README.txt"
Revert "Update p5.Geometry.js"
Hi @GregStanton, thank you for your comment on this issue (link). It helped me make a clear checklist, and I believe I’ve fixed the points you mentioned. I’m also checking the math parts to see what might be missing. If you know any other examples, docs, or tutorials that need updates, please let me know and I’ll fix them quickly. If you have time, your review on this PR would be very helpful for future readers. I’m happy to improve anything else you suggest. Thanks again! For reference, here’s the code I’m using for ends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes are looking good!
We can optionally do this in another PR, but because all the examples are ports from 1.x where it only had EXCLUDE mode, the examples for spline
and splineVertex
currently demonstrate mostly the more complicated EXCLUDE case. I feel like the first thing people should see, and see most of, would be the INCLUDE case. It's a bit of a bigger undertaking so it's ok to wrap up and merge this one first. I think the other improvements we could make would be:
- For
splineVertex
, show some examples of drawing shapes without editing the default end mode - For
splineVertex
, show examples of smooth loops usingendShape(CLOSE)
, like in this example @GregStanton sent me a little while ago: https://editor.p5js.org/highermathnotes/sketches/lyeF29D_v - For
spline
, also show some examples first without changing to EXCLUDE mode - In
splineProperty
, show examples fortightness
right after describing it using the interleaved example syntax, and then put theINCLUDE
/EXCLUDE
description + examples after that - Update
splinePoint
examples to use INCLUDE - Update
splineTangent
examples to use INCLUDE
src/shape/custom_shapes.js
Outdated
* | ||
* `splineVertex()` adds a curved segment to custom shapes. The spline curves | ||
* it creates are defined like those made by the | ||
* <a href="#/p5/curve">curve()</a> function. `splineVertex()` must be called | ||
* <a href="#/p5/spline">spline()</a> function. `splineVertex()` must be called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've got another curve()
reference later in this doc on line 1683 that needs a similar update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also later on in this doc, some things to correct:
-
Splines are defined by two anchor points and two control points.
This explanation only applies to the non-default EXCLUDE mode now, we can maybe replace this with @GregStanton's concise explanation, "splineVertex() draws a smooth curve through the points you give it."
-
splineVertex() must be called at least four times
I think this is no longer true, thanks to the default INCLUDE ends mode. Two points also works, it just draws a straight line. So we can probably take out that restruction entirely.
-
The code examples talk about control points and anchor points. In the default ends mode, all vertices are anchor points, since the cable goes through all of them.
-
The code snippet above would only draw the curve between the anchor points, similar to the curve() function. The segments between the control and anchor points can be drawn by calling
splineVertex()
with the coordinates of the control pointsSimilarly, this is also no longer the case with the INCLUDE mode, and it'll go through allt he points immediately. So this distinction and the snippet that follow can be removed.
src/shape/custom_shapes.js
Outdated
* splineVertex(83, 61); | ||
* splineVertex(25, 65); | ||
* splineVertex(25, 65); | ||
* splineVertex(60, 76); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the default INCLUDE mode, we don't need to double the first and last vertex, it'll go through them automatically, so we can take this and the last splineVertex
line out!
Also some notes for the
I think this control point / anchor point distinction currently also exists in |
Thanks for moving this forward! Based on your work and the detailed review that @davepagurek did, I'll outline an approach to tackling these docs. I'll include draft docs and code examples for some of the key features. I hope this does two things:
Please let me know if any of this is unclear, and if you want to ping me again for a review, I'd be happy to take another look! Also, I'm okay with whatever you two decide regarding whether or not to do a separate PR for the larger changes. splineVertex()As @davepagurek's review shows, there are a lot of updates to be made if we try to adjust the docs line by line. That's because the current documentation is based on the old docs for Below, I'll outline how the new docs could look. Note that the typical flow of these reference pages uses Dave's interleaved-example feature and goes like this:
The idea is to place explanations alongside examples, instead of putting all the explanations in a wall of text at the top, followed by a list of examples. This is based on the spatial contiguity principle: "corresponding information is easier to learn in a multimedia format when presented close together rather than separate or farther apart." DescriptionConnects points with a smooth curve (a spline). That's the whole description!!! We can add a couple links to other features below that:
Example: Basic usageIf you provide three points, the spline will pass through them. It works the same way with any number of points. ![]() Example: Basic closed splinePassing in ![]() Example: Advanced closed splineThanks @davepagurek for sharing my animated blob example. It's reasonably short, and it's kind of cool, so it might inspire some creativity. But it's also not a totally minimal example (it uses
|
Thank you all for your inputs. I’ll update and fix this PR, and also draft a new one with the changes suggested by @davepagurek and @GregStanton. Once I feel this PR is in good shape, I’ll request a review from the maintainers. There’s still a bit of work left, so really thanks everyone for the guidance and patience. |
Here's are some of the specific updates to make:
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.