Updating all the broken refrence examples. #7739
Conversation
|
Hi @perminder-17 , thanks for working on this! A couple more things - some of these may not be in documentation scope, so please comment here if you end up working on this and find these can't be fixed in documentation itself:
|
|
Hi kit,
Yes, sure. I have been looking for couple of things which can't be fixed through just changing the docs. Some additional works may be involved. I'll write it up here and open up an issue for that as well.
Yes, I think there could be more, I'll have a look.
Yep, I just noticed that now. Really thanks for catching this. I'll have a look and will update this PR if its not pointing to a bigger issue. Thanks for the updates :) |
Hi @ksen0 , Actually we get the documentation of the sound reference by cloning this repo : https://github.com./processing/p5.sound.js into the website and then it builds the jsDocs for this. And, the older sound library of p5 has been closed/deprecated. https://github.com./processing/p5.js-sound.git But now also, we are using the older build in our p5.js addons library in actual dev-2.0 branch. https://github.com./processing/p5.js/blob/dev-2.0/lib/addons/p5.sound.js One solution might be is, removing the older library from lib/addons and updating the new version of p5.js sound new sound library and include all the functions which were present on the older sound library and not present on the new one's. This will update the website with the docs. . Also, if we have any other approach for showing up the docs which currently uses the older-version lib/addons of sound library, I am happy to implement :")
For context, these are the docs which aren't shown when we build dev-2.0 branch, and these actually comes from the older version of sound library. |
|
Also tagging @davepagurek if this approach doesn't makes sense and needs to update the build of older-version of sound library? |
|
Hi @perminder-17 ! Just a quick note about the intent with sound: the 1.x reference on p5js.org should contain only the old sound library; and the 2.0 reference on beta.p5js.org should contain only the new one. Here's where that split was introduced: processing/p5.js-website#737 so things like |
src/core/main.js
Outdated
| * <a href="#/p5/draw">draw()</a> begins looping. If the | ||
| * <a href="#/p5/preload">preload()</a> is declared, then `setup()` will | ||
| * run immediately after <a href="#/p5/preload">preload()</a> finishes | ||
| * <a href="#/p5/draw">draw()</a> begins looping. When `setup()` is declared async, |
There was a problem hiding this comment.
How about a slightly more precise description:
"When setup() uses the async keyword (like this: async function setup() instead of function setup())...."
src/core/main.js
Outdated
| * <a href="#/p5/preload">preload()</a> is declared, then `setup()` will | ||
| * run immediately after <a href="#/p5/preload">preload()</a> finishes | ||
| * <a href="#/p5/draw">draw()</a> begins looping. When `setup()` is declared async, | ||
| * execution pauses at each `await` until the promise resolves, ensuring all assets |
There was a problem hiding this comment.
Similarly, providing a bit of explanation on promise:
"execution pauses at each await: for example, font = await loadFont(...) will wait for the font asset to load. This is because the loadFont(..) function returns a promise, and the await keyword means the program will wait for the promise to resolve."
Feel free to rephrase of course.
src/dom/dom.js
Outdated
| @@ -596,7 +596,7 @@ function dom(p5, fn){ | |||
| * 'https://p5js.org/assets/img/asterisk-01.png', | |||
There was a problem hiding this comment.
Broken URL; maybe grab an image from another part of the reference that works? eg from https://beta.p5js.org/reference/p5/image/?
| * box(200, 200, 200); | ||
| * describe(`red horizontal line right, green vertical line bottom. | ||
| * black background.`); | ||
| * background(220); |
There was a problem hiding this comment.
Nice, thank you for the expanded example 👍
src/webgl/p5.Geometry.js
Outdated
| * let v1 = createVector(0, -40, 0); | ||
| * let v2 = createVector(0, 40, 0); | ||
| * let v3 = createVector(40, 0, 0); | ||
| * myGeometry = buildGeometry(createShape); |
There was a problem hiding this comment.
I think this usage of buildGeometry is a tad unusual currently -- typically the callback to buildGeoemtry draws a shape, but this one just assigns to some global variables, so this might end up confusing contributors.
I think the original pattern here using p5.Geometry is ok if we're doing fully custom vertices/faces. If we do buildGeometry, it could look something like this, without touching the vertices/faces afterwards:
myGeometry = buildGeometry(() => {
beginShape(QUADS);
vertex(-40, -40);
vertex(40, -40);
vertex(-40, 40);
vertex(40, 40);
endShape();
});There was a problem hiding this comment.
When I was initially exploring the documentation, I noticed that using the pattern new p5.Geometry() didn’t seem to work, nothing appeared on the canvas. Because of this, I assumed that we had moved entirely to using buildGeometry(). However, I recently came across examples on the latest beta site (https://beta.p5js.org/reference/p5.geometry/computefaces/), and to my surprise, they now work correctly. It seems that there may have been a recent fix or update that restored proper functionality to p5.Geometry, which is great to see. So, now I have reverted the code to old p5.Geoemtry. Hope that works?
|
Hi @ksen0 , Thanks for all the suggestions. These were really great suggestions :). I have tried to fix them all? Do you think we are good to go, let me know if you have any other suggestions. I'll fix them asap. Also, @davepagurek , do you have any suggestion for this one : #7739 (comment) |
|
Hi @perminder-17 these are looking good! I might have found some new ones :)
|
| @@ -81,6 +81,11 @@ function outputs(p5, fn){ | |||
| * | |||
| * <div> | |||
| * <code> | |||
| * | |||
There was a problem hiding this comment.
Hi @ksen0 , are these functions still broken on my branch as well i.e. After this PR?. I think I tried fixing textOutput() and gridOutput here.
There was a problem hiding this comment.
I suspect there's bigger problems here because they're broken on 1.x and 2.0. I'll make a separate 1.x/2.0 issue tomorrow about it, please feel free to ignore for this PR. Thank you!
There was a problem hiding this comment.
I suspect there's bigger problems here because they're broken on 1.x and 2.0. I'll make a separate 1.x/2.0 issue tomorrow about it, please feel free to ignore for this PR. Thank you!
Hmm, So sorry for the oversight, I think the sketch works correctly but it didn't shows up correctly in the website. I'll have a look today late night or tomorrow morning to figure out what's going on. Thanks for the patience @ksen0 :)
| @@ -215,6 +225,11 @@ function outputs(p5, fn){ | |||
| * | |||
| * <div> | |||
| * <code> | |||
| * | |||
Hmmm, not sure but I think sampleFactor is not clamped to the 0‒1 range. So, I don't know the how to explain it exactly but i think if we passes it in the options object i.e. Line 1066 in 55ed78e |
|
That's correct @perminder-17 , it just needs to be >0, and higher densities than 1 will produce more and more vertices. The main use for this would be if you have called scale(), or anticipate drawing the shape in the future at a higher scale, or intend to apply a vertex shader distortion that will spread out the vertices. |
No description provided.