-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[WIP] Docs: Add reference entries for p5.strands hooks (feedback wanted) #7920
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
Conversation
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
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 would be inclined to give a little bit more context for each of the hook functions. For now this is the only presence of p5.strands in the documentation. I would mention where these functions can be used, and explain what world space is, for example. Let me know if you need help with that as it's a newer feature
src/webgl/p5.strands.js
Outdated
* myShader = baseMaterialShader().modify(() => { | ||
* getWorldInputs((inputs) => { | ||
* // Move the vertex up and down in a wave | ||
* inputs.position.y += 20 * Math.sin(millis() * 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.
In this case, you have to use sin()
not Math.sin
!
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 sin() instead of Math.sin() as suggested.
// Before:
inputs.position.y += 20 * Math.sin(millis() * 0.001 + inputs.position.x * 0.05);
// After:
inputs.position.y += 20 * sin(millis() * 0.001 + inputs.position.x * 0.05);
src/webgl/p5.strands.js
Outdated
* @function getWorldInputs | ||
* @experimental | ||
* @description | ||
* Registers a callback to modify world-space vertex inputs for each 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.
Maybe 'vertex inputs for each vertex' is a bit redundant here. I would remove the 'for each vertex' part or reword slightly
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.
Reworded the description to remove redundancy and improve clarity.
// Before:
* Registers a callback to modify world-space vertex inputs for each vertex.
// After:
* Registers a callback to modify the world-space properties of each vertex in a shader.
src/webgl/p5.strands.js
Outdated
* - {@link p5.baseStrokeShader} | ||
* | ||
* @param {function(inputs: { position: {x: number, y: number, z: number}, normal: {x: number, y: number, z: number}, texCoord: {x: number, y: number}, color: {r: number, g: number, b: number, a: number} }): object} callback | ||
* A function that receives the current inputs and returns the modified inputs. |
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.
Maybe something like "a callback function which receives and returns a Vertex struct."
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 callback parameter description to reference a Vertex struct and added a @typedef for clarity.
/**
* @typedef {Object} Vertex
* @property {{x: number, y: number, z: number}} position - The position of the vertex in world space.
* @property {{x: number, y: number, z: number}} normal - The normal vector at the vertex in world space.
* @property {{x: number, y: number}} texCoord - The texture coordinates (x, y) for the vertex.
* @property {{r: number, g: number, b: number, a: number}} color - The color at the vertex.
*/
/**
* @param {function(Vertex): Vertex} callback
* A callback function which receives and returns a Vertex struct.
*/
src/webgl/p5.strands.js
Outdated
* The callback receives the current point size (number) and should return the new size (number). | ||
* | ||
* This hook is available in: | ||
* - {@link p5.pointShader} |
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.
Do we have a point shader?
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 reference to p5.pointShader and only listed the correct shaders for each hook.
// Before:
* This hook is available in:
* - {@link p5.pointShader}
// After:
* This hook is available in:
* - {@link p5.baseMaterialShader}
* - {@link p5.baseNormalShader}
* - {@link p5.baseColorShader}
* - {@link p5.baseStrokeShader}
Thank you for the review and detailed feedback @lukeplowden . I have made the following updates to address all the points :
Please let me know if there’s anything else I should update. Thank you! |
Hi @lukeplowden , I have addressed all the feedback and updated the documentation accordingly. If everything looks good and the changes are correct, please let me know so I can proceed with documenting the remaining hooks in the same style. Or, if any further changes are needed, please let me know and I will update accordingly. Thank you! |
6f732bc
to
ae09118
Compare
9947d92
to
ae09118
Compare
cc727b0
to
ae09118
Compare
Also, just to note, @lukeplowden : my previous commit was mistakenly deleted. All the updates mentioned above are now included in my current commit. Thank you for your understanding! |
Hi @lukeplowden , just following up on this PR. I’ve made all the updates you suggested (details in my previous comment). Please let me know if there’s anything else I should update, or if this is good to go. 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 thankyou for taking this task. I haven’t reviewed it in detail yet, but have you created a separate file for the documentation? Do you think we could generate the reference pages by adding a doc-comment block at the end of the file that describes each method?
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 also noticed that you added the reference documentation to the main branch, but p5.Strands is implemented in the dev-2.0 branch.
Could you please retarget your PR to the dev-2.0 branch and re-base your work on top of it?
Steps:
- In the PR, click Edit ➜ change the base from
main
todev-2.0
➜ Save (Already I did)
Locally:
git fetch origin
git checkout strands-docs-draft
git rebase origin/dev-2.0
git push --force-with-lease
This will make the PR show only your intended changes against dev-2.0
, with conflicts resolved. Thanks!
Hi @perminder-17 , thank you for your response! I haven’t created a separate file for the documentation - instead, I’ve added detailed doc-comment blocks directly above each method within the p5.strands.js file. This approach is consistent with how documentation is handled elsewhere in the codebase and should allow the reference pages to be generated automatically. If you’d prefer, I can also add a summary doc-comment block at the end of the file that describes each method, or create a separate documentation file as needed. Please let me know your preference, and I’ll be happy to make the adjustments! Thank you! |
Hi @perminder-17,
However, While rebasing onto dev-2.0 locally, I’m encountering repeated merge conflicts in files like README.md and .all-contributorsrc. Should I need resolve each conflict manually, or is it acceptable to skip these commits if they’re not relevant to my changes? Please advise on the preferred approach. Thank you! Here’s an example of the error I’m seeing: For
|
Hi @perminder-17, Just following up on my previous question regarding the repeated merge conflicts while rebasing onto dev-2.0 (specifically in files like README.md and .all-contributorsrc). Should I resolve each conflict manually, or is it acceptable to skip these commits if they’re not relevant to my changes? Would appreciate your advice on the preferred approach. Thank you! |
Would it be okay for you to open a new PR targeting |
Thank you for the suggestion @perminder-17 ! |
Thanks for your patience and really sorry for the delay in reviewing. |
No problem at all, and thanks for taking the time to review! |
Closing this PR in favor of #7940, which ports these changes to the dev-2.0 branch as suggested by the maintainers. The new PR avoids the previous merge conflicts and follows the recommended workflow. Thank you to everyone who reviewed and provided feedback here! |
This draft PR adds new JSDoc reference entries for a few p5.strands hooks, following the feedback in #7919.
The new doc comments use JavaScript-only examples and describe the callback signature as discussed.
I’ve included examples for :
getWorldInputs
combineColors
getPointSize
The doc comments are currently placed in a new file :
src/webgl/p5.strands.js
(Open to moving them if you prefer a different location.)
Could you please review the style, level of detail, and file location?
Once confirmed, I’ll update the rest of the hooks in the same format.
Thanks for your guidance!