-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[WIP] Docs: Add initial draft of Strands documentation (feedback wanted) #7940
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
Hi @perminder-17 Could you please review this new PR? It replaces the previous one #7920 due to merge conflicts and now targets Thank you! |
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.
Hi @Abhayaj247, thank you for your work! I noticed you added a new p5.strands.js file under src/webgl just for documentation, but it isn’t referenced anywhere else in the repo. Could you instead place your docs to the end of shaderGenerator.js? That way we avoid adding an unused file and keep things cleaner.
Hi @perminder-17, I’ve moved the p5.strands JSDoc documentation to the end of shaderGenerator.js as requested, starting from line 1644. The separate file has been removed. Could you please confirm if the style, detail, and placement look good? Once you approve, I’ll proceed to document the remaining hooks in the same format. Please let me know if you’d like any further adjustments. Thank you! |
src/webgl/ShaderGenerator.js
Outdated
* Registers a callback to modify the world-space properties of each vertex in a shader. This hook can be used inside {@link p5.baseMaterialShader}.modify() and similar shader modify calls to customize vertex positions, normals, texture coordinates, and colors before rendering. "World space" refers to the coordinate system of the 3D scene, before any camera or projection transformations are applied. | ||
* | ||
* This hook is available in: | ||
* - {@link p5.baseMaterialShader} |
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 although this is valid JSDoc, we don't yet support it in p5's jsdoc-to-reference pipeline. Currently we use <a href="...">
, e.g.:
Line 1207 in 0da0430
* <a href="#/p5/setup">setup()</a> before using the result. |
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.
Hi @davepagurek ,
I've updated all references to use the <a href="...">
syntax that p5.js supports, following the pattern from p5.Font.js. All links now use the format <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>
.
src/webgl/ShaderGenerator.js
Outdated
|
||
/* ------------------------------------------------------------- */ | ||
/** | ||
* @typedef {Object} Vertex |
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'm also not sure that typedef docs will show up in the reference, even though this does simplify the writing of the function parameter. If you test your branch on the p5.js-website repo, what does this look like? If it doesn't render, we may need to either document this object inline, or update the website to be able to support this syntax.
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.
@davepagurek Thank you for the guidance on the @typedef issue. I've removed it and documented the vertex object properties inline in the parameter description to ensure clarity and compatibility with the documentation pipeline.
src/webgl/ShaderGenerator.js
Outdated
* @param {function(ColorComponents): { color: {r: number, g: number, b: number}, opacity: number }} callback | ||
* A callback function which receives a ColorComponents struct and returns the final color and opacity. | ||
* | ||
* @see {@link p5.baseMaterialShader} |
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.
Similar to above, we probably don't support @see
yet, but feel free to use regular links in a paragraph explaining why you might want to check out these related references!
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.
@davepagurek , I've removed all @see
tags since they're not supported in the current documentation pipeline. The documentation now relies on clear descriptions and contextual examples to guide users to related references.
src/webgl/ShaderGenerator.js
Outdated
* myShader = baseMaterialShader().modify(() => { | ||
* combineColors(components => { | ||
* // Custom color combination: add a red tint | ||
* let color = { |
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.
Although you can access .r
, .g
, .b
, and .a
properties, the return value would need to be either [r, g, b, a]
as an array, or more explicitly, vec4(r, g, b, a)
-- we don't yet support an object notation for this. (Also it will need to be just the one object, and not an object with separate color and opacity properties, for it to compile correctly I think.)
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.
This also makes me wonder what the best way to document the inputs
object is, since it's not exactly any normal type. @lukeplowden do you have any thoughts on this? We could call it a vec4
and then explain in the description of the function what this means, or even not give it a type at all, and just have an explanation in the description.
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.
Although you can access
.r
,.g
,.b
, and.a
properties, the return value would need to be either[r, g, b, a]
as an array, or more explicitly,vec4(r, g, b, a)
-- we don't yet support an object notation for this. (Also it will need to be just the one object, and not an object with separate color and opacity properties, for it to compile correctly I think.)
@davepagurek , I've corrected the combineColors example to return vec4(r, g, b, a) instead of object notation. The example now properly demonstrates the correct return format that will compile successfully.
This also makes me wonder what the best way to document the
inputs
object is, since it's not exactly any normal type. @lukeplowden do you have any thoughts on this? We could call it avec4
and then explain in the description of the function what this means, or even not give it a type at all, and just have an explanation in the description.
Excellent point about the input object documentation. I've updated the parameter descriptions to clearly specify the available properties without using complex type annotations. For getWorldInputs, I've documented the vertex object properties inline, and for combineColors, I've listed all available color component properties in the description.
Thank you for the comprehensive feedback @davepagurek . I've addressed all the documentation compatibility issues:
The documentation should now be fully compatible with the current p5.js documentation pipeline. If there are any other modifications or improvements you’d like to see, please let me know—I'm happy to make further adjustments as needed. |
src/webgl/ShaderGenerator.js
Outdated
@@ -1700,19 +1689,16 @@ if (typeof p5 !== 'undefined') { | |||
* @function combineColors | |||
* @experimental | |||
* @description | |||
* Registers a callback to customize how color components are combined in the fragment shader. This hook can be used inside {@link p5.baseMaterialShader}.modify() and similar shader modify calls to control the final color output of a material. The callback receives a ColorComponents struct and should return an object with a `color` property ({ r, g, b }) and an `opacity` property (number). | |||
* Registers a callback to customize how color components are combined in the fragment shader. This hook can be used inside <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>.modify() and similar shader modify calls to control the final color output of a material. The callback receives color components (baseColor, diffuse, ambientColor, ambient, specularColor, specular, emissive, opacity) and returns a vec4 for the final color. |
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.
Can we describe the input as being an object with the properties baseColor
, diffuse
, ambientColor
, etc? I think it would be important to describe the size of each item (e.g. position/normal are vec3s, texture coordinates are vec2s, color is a vec4) -- although maybe rather than using terms like vec3
we could describe it as "a vector with three components" to avoid referencing the vec3
type, since there aren't user-facing docs for that yet. A bullet list might work if that's too much info to put in one sentence.
One other note is that when we refer to a name of a property/variable/etc in code, such as when listing properties here, we should put the names in backticks so they render as code (baseColor
instead of baseColor.)
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.
/**
* @function combineColors
* @experimental
* @description
* Registers a callback to customize how color components are combined in the fragment shader. This hook can be used inside <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>.modify() and similar shader modify calls to control the final color output of a material. The callback receives an object with the following properties:
*
* - `baseColor`: a vector with three components representing the base color (red, green, blue)
* - `diffuse`: a single number representing the diffuse reflection
* - `ambientColor`: a vector with three components representing the ambient color
* - `ambient`: a single number representing the ambient reflection
* - `specularColor`: a vector with three components representing the specular color
* - `specular`: a single number representing the specular reflection
* - `emissive`: a vector with three components representing the emissive color
* - `opacity`: a single number representing the opacity
*
* The callback should return a vector with four components (red, green, blue, alpha) for the final color.
*
* This hook is available in:
* - <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>
* - <a href="#/p5/baseNormalShader">baseNormalShader()</a>
* - <a href="#/p5/baseColorShader">baseColorShader()</a>
* - <a href="#/p5/baseStrokeShader">baseStrokeShader()</a>
* - <a href="#/p5/baseFilterShader">baseFilterShader()</a>
*
* @param {function} callback
* A callback function which receives the object described above and returns a vector with four components for the final color.
*
* @example
* <div modernizr='webgl'>
* <code>
* let myShader;
* function setup() {
* createCanvas(200, 200, WEBGL);
* myShader = baseMaterialShader().modify(() => {
* combineColors(components => {
* // Custom color combination: add a red tint
* let r = components.baseColor[0] * components.diffuse +
* components.ambientColor[0] * components.ambient +
* components.specularColor[0] * components.specular +
* components.emissive[0] + 0.2;
* let g = components.baseColor[1] * components.diffuse +
* components.ambientColor[1] * components.ambient +
* components.specularColor[1] * components.specular +
* components.emissive[1];
* let b = components.baseColor[2] * components.diffuse +
* components.ambientColor[2] * components.ambient +
* components.specularColor[2] * components.specular +
* components.emissive[2];
* return [r, g, b, components.opacity];
* });
* });
* }
* function draw() {
* background(255);
* shader(myShader);
* lights();
* noStroke();
* fill('red');
* sphere(50);
* }
* </code>
* </div>
*/
@davepagurek Thank you for your feedback! I’ve updated the documentation for combineColors
to describe the input as an object with each property listed in a bullet list, using backticks for property names and describing the size of each in plain language. The example now uses array notation for vector properties. Please let me know if you’d like any further adjustments.
src/webgl/ShaderGenerator.js
Outdated
*/ | ||
|
||
/** | ||
* @function getPointSize |
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.
Just a heads up -- currently the only public base shaders are:
The point shader, while it has some hooks, is currently still private, as we may end up using the stroke shader to render points in the future.
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.
/**
* @function getPointSize
* @experimental
* @description
* Registers a callback to modify the size of points when rendering with a shader.
*
* This hook can be used inside the following shader modify functions:
* - <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>.modify()
* - <a href="#/p5/baseNormalShader">baseNormalShader()</a>.modify()
* - <a href="#/p5/baseColorShader">baseColorShader()</a>.modify()
* - <a href="#/p5/baseStrokeShader">baseStrokeShader()</a>.modify()
* - <a href="#/p5/baseFilterShader">baseFilterShader()</a>.modify()
*
* Use this hook when drawing points (for example, with the point() function in WEBGL mode).
* The callback receives the current point size (number) and should return the new size (number).
*
* This hook is available in:
* - <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>
* - <a href="#/p5/baseNormalShader">baseNormalShader()</a>
* - <a href="#/p5/baseColorShader">baseColorShader()</a>
* - <a href="#/p5/baseStrokeShader">baseStrokeShader()</a>
* - <a href="#/p5/baseFilterShader">baseFilterShader()</a>
*
* @param {function} callback
* A callback function which receives and returns the point size.
*
* @example
* <div modernizr='webgl'>
* <code>
* let myShader;
* function setup() {
* createCanvas(200, 200, WEBGL);
* myShader = baseMaterialShader().modify(() => {
* getPointSize(size => {
* // Make points pulse in size over time
* return size * (1.0 + 0.5 * sin(millis() * 0.002));
* });
* });
* }
* function draw() {
* background(255);
* shader(myShader);
* strokeWeight(20);
* stroke('blue');
* point(0, 0);
* }
* </code>
* </div>
*/
@davepagurek Thank you for the clarification regarding public base shaders. I’ve updated the documentation for getPointSize
to reference only the public shaders and removed any mention of private or internal shaders. Please let me know if you have any further suggestions or preferences.
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 getPointSize
itself is only present in the base point shader, so I think we don't need this one.
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
getPointSize
itself is only present in the base point shader, so I think we don't need this one.
@davepagurek Thank you for clarifying! I have removed the documentation for getPointSize
since it is only present in the base point shader and not part of the public shaders.
Please let me know if there’s anything else I should update.
@davepagurek Thank you for your thoughtful feedback and guidance throughout this process. I have addressed all the points you raised:
Please see the updated code blocks above for reference. If you have any further suggestions or would like additional adjustments, I’m happy to make them! |
Thanks @Abhayaj247, the docs are looking good! I think maybe we're ready to give the other hooks the same treatment now. |
@davepagurek Thank you so much for your guidance and support throughout this process! I’m glad the documentation updates meet your expectations. I’ll proceed to document the remaining hooks in the same style. |
Hi @davepagurek , As I continue updating the shader documentation, I’ve reviewed the codebase and found these hooks as the remaining ones that appear to be public or user-facing:
Additionally, I noticed hooks like Could you please confirm that this list covers all the hooks intended for user documentation? Once confirmed, I’ll proceed to document them in the new style. Thank you for your guidance! |
Hi @Abhayaj247! I think some of those, e.g. |
Hi @davepagurek Thank you for clarifying! I’ll reference the tables on the official 2.0 beta documentation pages and ensure that only those hooks are documented. I’ll update the docs accordingly and let you know once it’s ready for review. |
4e5489f
to
1d01ca8
Compare
3b3437c
to
36c4c2d
Compare
f4872c4
to
6e1311d
Compare
Hi @davepagurek Quick check on shouldDiscard: I changed the example to the string-based form to avoid the earlier runtime error.
It renders correctly (3D → Stroke) with no console errors. Is this usage/signature what you’d like to show, or would you prefer a version that produces a more visible effect (e.g., always keep: |
Thanks for the detailed review! @davepagurek , I’ve addressed each point:
Key example updates:
I've made all the requested changes. If you have a moment, could you please review and let me know if any further adjustments are needed? Thank you for your time! |
@davepagurek I’ve now gone through all hooks and their examples in the local reference build. Everything appears and renders correctly. When you have a moment, could you please review on your end as well and let me know if you spot anything else? Thank you! |
src/webgl/ShaderGenerator.js
Outdated
* - `texCoord`: a vector with two components representing the texture coordinates (u, v) | ||
* - `canvasSize`: a vector with two components representing the canvas size in pixels (width, height) | ||
* - `texelSize`: a vector with two components representing the size of a single texel in texture space | ||
* - `color`: a vector with four components representing the current pixel color (red, green, blue, alpha) |
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.
This isn't one of the input properties, we can take this line out.
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.
Removed the color
property from the getColor
parameters list since it’s not an actual input property.
src/webgl/ShaderGenerator.js
Outdated
* let t = uniformFloat(() => millis()); | ||
* getPixelInputs(inputs => { | ||
* // Animate alpha (transparency) based on x position | ||
* if (inputs.color && inputs.texCoord) { |
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.
These properties will always exist, we don't need an if statement here.
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.
Removed the if
check for inputs.color
and inputs.texCoord
as these properties are always present.
src/webgl/ShaderGenerator.js
Outdated
* lights(); | ||
* noStroke(); | ||
* fill('purple'); | ||
* sphere(50); |
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 noticed that this example has some weird artifacts when animating. I'm pretty sure this is just because of the fact that drawing semitransparent stuff in 3D is currently difficult and is order-dependent. But, if we switch sphere(50)
to circle(0, 0, 100)
, it looks great in 2D. Maybe let's do that to avoid the issues?
Screen.Recording.2025-08-08.at.3.51.34.PM.mov
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.
Updated the example to use circle(0, 0, 100)
instead of sphere(50)
to avoid 3D transparency artifacts.
src/webgl/ShaderGenerator.js
Outdated
* getWorldInputs(inputs => { | ||
* // Move the vertex up and down in a wave in world space | ||
* // In world space, moving the object (e.g., with translate()) will affect these coordinates | ||
* inputs.position.y += 0.5 * sin(t * 0.001 + inputs.position.x * 0.05); |
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 maybe the wrong one got updated -- a scale of 20 makes sense here because in world space, the sphere is 50 units tall. The update to change the scale to 0.5 was for object space where the sphere is 2 units tall.
* inputs.position.y += 0.5 * sin(t * 0.001 + inputs.position.x * 0.05); | |
* inputs.position.y += 20 * sin(t * 0.001 + inputs.position.x * 0.05); |
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.
Changed the wave displacement scale from 0.5
to 20
for the world-space example, matching the object’s actual size.
src/webgl/ShaderGenerator.js
Outdated
/** | ||
* @method getWorldInputs | ||
* @description | ||
* Registers a callback to modify the world-space properties of each vertex in a shader. This hook can be used inside <a href="#/p5/baseColorShader">baseColorShader()</a>.modify() and similar shader modify calls to customize vertex positions, normals, texture coordinates, and colors before rendering. "World space" refers to the coordinate system of the 3D scene, before any camera or projection transformations are applied. |
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.
getWorldInputs
doesn't yet have a description of the parameters to inputs
. It has the same inputs as getObjectInputs
and getCameraInputs
, can we copy and paste the properties bullet list from one of those here?
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.
Added a parameters bullet list for getWorldInputs
, matching the format and content from getObjectInputs
/getCameraInputs
.
Thanks for the detailed review! @davepagurek , I’ve addressed each point:
All requested changes have been made. Please let me know if you spot anything else that needs adjusting. Thank you for your guidance! |
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.
Hi @Abhayaj247 thanks for your work on this, it mostly looks good to me, just a few minor suggestions on the examples and docs. Really thanks for your hard work and patience. :)
src/webgl/ShaderGenerator.js
Outdated
/** | ||
* @method getWorldInputs | ||
* @description | ||
* Registers a callback to modify the world-space properties of each vertex in a shader. This hook can be used inside <a href="#/p5/baseColorShader">baseColorShader()</a>.modify() and similar shader modify calls to customize vertex positions, normals, texture coordinates, and colors before rendering. "World space" refers to the coordinate system of the 3D scene, before any camera or projection transformations are applied. |
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.
Probably you could attach a link here for modify where you write:
similar shader .modify() calls to customize.....
It should look something like this.
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.
Thanks for pointing this out! I’ve added a link to .modify()
so it’s consistent with other references.
src/webgl/ShaderGenerator.js
Outdated
* - <a href="#/p5/baseColorShader">baseColorShader()</a> | ||
* - <a href="#/p5/baseStrokeShader">baseStrokeShader()</a> | ||
* | ||
* @param {function} callback |
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.
* @param {function} callback | |
* @param {Function} callback |
Probably you could use Function
wherever you used it in lowercase
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’ve updated @param {function}
to @param {Function}
throughout for consistency.
src/webgl/ShaderGenerator.js
Outdated
* shader(myShader); | ||
* lights(); | ||
* noStroke(); | ||
* fill('red'); |
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.
When you use fill('red')
, the red tint dominates and your custom color mix can’t show clearly. Probably you could remove fill()
or use fill(255)
(white) so your shader’s color shows as intended. Thanks :)
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.
Makes sense - I’ve replaced fill('red')
with fill('white')
so the shader’s colors show clearly.
src/webgl/ShaderGenerator.js
Outdated
/** | ||
* @method combineColors | ||
* @description | ||
* Registers a callback to customize how color components are combined in the fragment shader. This hook can be used inside <a href="#/p5/baseMaterialShader">baseMaterialShader()</a>.modify() and similar shader modify calls to control the final color output of a material. The callback receives an object with the following properties: |
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.
similar shader modify calls to
similarly here as well, use .modify()
with link
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.
Done ✅ I’ve updated this section to use .modify()
with a link for clarity.
src/webgl/ShaderGenerator.js
Outdated
* - `normal`: a vector with three components representing the surface normal | ||
* - `texCoord`: a vector with two components representing the texture coordinates (u, v) | ||
* - `ambientLight`: a vector with three components representing the ambient light color | ||
* - `ambientMaterial`: a vector with three components representing the material's ambient color | ||
* - `specularMaterial`: a vector with three components representing the material's specular color | ||
* - `emissiveMaterial`: a vector with three components representing the material's emissive color | ||
* - `color`: a vector with four components representing the base color (red, green, blue, alpha) | ||
* - `shininess`: a number controlling specular highlights | ||
* - `metalness`: a number controlling the metalness factor |
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.
In these, wherever you write "a vector with three components" would it be nice when you write "a three-component vector" and also, after every sentence you should give a period .
. What you think?
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.
Great suggestion! I’ve rephrased those to “a three-component vector” and added periods at the end of each line.
src/webgl/ShaderGenerator.js
Outdated
* - `color`: a vector with four components representing the stroke color (red, green, blue, alpha) | ||
* - `tangent`: a vector with two components representing the stroke tangent | ||
* - `center`: a vector with two components representing the cap/join center | ||
* - `position`: a vector with two components representing the current fragment position | ||
* - `strokeWeight`: a number representing the stroke weight in pixels |
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.
Here also, rephrasing it and adding a period .
.
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.
Applied the same rephrasing and punctuation improvements here as well 👍.
src/webgl/ShaderGenerator.js
Outdated
* myShader = baseColorShader().modify(() => { | ||
* getFinalColor(color => { | ||
* // Add a blue tint to the output color | ||
* color.b += 0.2; |
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 blue effect is very minimal from the shader when I saw the sketch, could you use 0.4
instead of 0.2
?
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.
Thanks - I’ve increased the blue tint to 0.4
so the effect is more noticeable.
src/webgl/ShaderGenerator.js
Outdated
* let t = uniformFloat(() => millis()); | ||
* getObjectInputs(inputs => { | ||
* // Create a sine wave along the x axis in object space | ||
* inputs.position.y += 20 * sin(t * 0.001 + inputs.position.x * 0.05); |
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.
A very minor suggestion, 20
is too high, the sphere goes off the canvas for long time. Use 2 or 3 instead, so the wave stays visible and the sphere stays in view.
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.
Good point 🙂 I’ve reduced the sine wave amplitude to 3
so the sphere stays visible within the canvas.
src/webgl/ShaderGenerator.js
Outdated
* shader(myShader); | ||
* noStroke(); | ||
* fill('blue'); | ||
* sphere(100); |
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.
Since the canvas is 200×200
, sphere(100)
has a 200-pixel diameter and reaches the edges. Can we switch to sphere(50)
so it stays comfortably within the canvas?
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.
Makes sense - I’ve updated sphere(100)
to sphere(50)
so it fits comfortably within the canvas.
…, wording, and shader examples for clarity and consistency
Hi @perminder-17 , Thanks a lot for the detailed review 🙌 I’ve gone through your comments and made the suggested changes : 1. Added links for All requested changes have been made. Please let me know if you spot anything else that needs adjusting. Thank you for your guidance! |
src/webgl/ShaderGenerator.js
Outdated
* background(200); | ||
* shader(myShader); | ||
* noStroke(); | ||
* fill('blue'); |
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.
Currently the inputs.color.b = 1
has no effect if you comment it out because the input color's blue channel is already maxed out from the fill('blue')
here. Can we use fill('red')
so that we get a visible color change?
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.
Good point 👍 I’ve changed fill('blue')
to fill('red')
so the effect of modifying the blue channel is clearly visible.
src/webgl/ShaderGenerator.js
Outdated
* shader(myShader); | ||
* noStroke(); | ||
* fill('orange'); | ||
* sphere(100); |
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.
@perminder-17 mentioned it on another example, but on here too sphere(100)
fills the canvas entirely, I think sphere(50)
leaves some room for it to go up and down:
Screen.Recording.2025-08-19.at.7.02.27.PM.mov
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.
Thanks for catching that! I’ve updated sphere(100)
to sphere(50)
so it fits better within the canvas.
src/webgl/ShaderGenerator.js
Outdated
* let t = uniformFloat(() => millis()); | ||
* getObjectInputs(inputs => { | ||
* // Create a sine wave along the x axis in object space | ||
* inputs.position.y += 3 * sin(t * 0.001 + inputs.position.x * 0.05); |
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 3 here is also so large it goes off canvas (object space, for a sphere, goes between -1 and 1). Similarly, the small scale on inputs.position.x
causes it to not have a very visible impact. Multipliers of 1 look decent here, meaning we can just take both multipliers out:
* inputs.position.y += 3 * sin(t * 0.001 + inputs.position.x * 0.05); | |
* inputs.position.y += sin(t * 0.001 + inputs.position.x); |
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.
Makes sense - I’ve simplified the sine wave expression by removing the multipliers as suggested. It now uses:
inputs.position.y += sin(t * 0.001 + inputs.position.x);
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.
This is really close to being there! I took a look at the docs in the p5 website locally, and noticed the three small things I commented above. After that we're good to merge!
…size, and simplifying sine wave example
Hi @davepagurek , Thanks a lot for the detailed review 🙌 I’ve gone through your comments and made the suggested changes : - Changed All requested changes have been made. Please let me know if you’d like any further refinements. 🚀 Thank you for your guidance! |
Hi @davepagurek , I’ve addressed the feedback you shared. Once this looks good, could you please let me know if there are any additional steps required before merging?For example, do I need to open a separate pull request following a specific PR format, or will this one be merged directly? Also, if you feel any further adjustments are needed, please let me know and I’ll be happy to update. Thanks again for your guidance! |
Note: This PR replaces #7920 to resolve merge conflicts that occurred while rebasing onto the
dev-2.0
branch.This PR introduces new JSDoc reference entries for several
p5.strands
hooks, as discussed in #7919 and based on recent feedback.Key updates:
getWorldInputs
combineColors
getPointSize
src/webgl/p5.strands.js
for clarity and modularity.Request for feedback:
Once I receive feedback and confirmation on the format and placement, I’ll proceed to document the remaining hooks in the same style.
Thank you for your time and guidance!