Skip to content
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

WebGL mode documentation improvements #6167

Merged
merged 19 commits into from Jun 20, 2023

Conversation

wong-justin
Copy link
Contributor

@wong-justin wong-justin commented May 29, 2023

(This PR is a working draft that's part of the GSoC 2023 project 'Supporting shader-based filters in p5.js", mentored by @aferriss and @aceslowman)

Initial changes:

  • add basic summary of differences between P2D and WEBGL mode
  • add links to WEBGL mode tutorial content

Screenshots:

  • Before:
    2023-05-29-14-45-01
  • After:
    2023-05-29-14-45-09

PR Checklist

@welcome
Copy link

welcome bot commented May 29, 2023

🎉 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!

Comment on lines 28 to 32
* - The <strong>coordinate system</strong> is centered and uses an optional z-dimension
* - <strong>Textures</strong>, <strong>materials</strong>, and <strong>lighting</strong> can be applied to shapes
* - You can control the <strong>camera</strong> that views the scene
* - <strong>Text</strong> is drawn differently
* - You can use <strong>shaders</strong>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about adding links to each of the bold features, like <a href="https://github.com/processing/p5.js/wiki/Getting-started-with-WebGL-in-p5#text"><strong>Text</strong></a> is drawn differently. Not sure if function docs should be linked, or p5js tutorial sections, or other content, or if they should even have links in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linking sounds good to me, for what it's worth!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is helpful to link out to more info for each of those topics and I appreciate this as a brief list of differences. I think that replacing the bold text with links though might not be very clear, I wonder if things can be reworded in a way that lets you describe where the information can be found. Something like:

The coordinate system is centered and uses an optional z-dimension, which you can read more about in the <a href="">"Getting Started With WebGL"</a> tutorial in the Learn page.

Additionally, it would help to include a little more text for "Text is drawn differently" and "You can use shaders". For text, the reference page for text() describes some of the issues, maybe you can rephrase it into something like:

Text in WebGL requires opentype/truetype fonts loaded in your [preload()](https://p5js.org/reference/#/p5/preload) function using the [loadFont()](https://p5js.org/reference/#/p5/loadFont) method. More information about using text in this mode can be found in the [wiki](https://github.com/processing/p5.js/wiki/Getting-started-with-WebGL-in-p5#text).

And it might be helpful to briefly define shader, as some users might not be familiar:

WebGL mode makes it possible to use shaders, which are programs that can be used for creating a variety of effects and graphics that take advantage of hardware acceleration.

* One of the two render modes in p5.js, most known for 3D rendering.
*
* Some differences and new features to expect compared to the default render mode <a href="/#/p5/P2D">P2D</a>:
* - The <strong>coordinate system</strong> is centered and uses an optional z-dimension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just a heads up, for now bullet lists in our docs render just as newlines in the reference. I don't think we have an issue to track this yet so I made an issue for it here: #6170

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok once this is merged #6177 you'll get bullets next to your list items!

src/core/constants.js Outdated Show resolved Hide resolved
src/core/constants.js Outdated Show resolved Hide resolved
src/core/constants.js Outdated Show resolved Hide resolved
src/core/constants.js Outdated Show resolved Hide resolved
@wong-justin
Copy link
Contributor Author

Thanks for all the feedback!
Updated content:

image

Documentation is generated from p5.prototype.point/line
at src/core/shape/2d_primitives, not from p5.RendererGL.point/line.
Copy link
Contributor Author

@wong-justin wong-justin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit took me a while to realize. Maybe a comment should be left for future contributors to know that documentation will not be generated for p5.RendererGL.prototype definitions?

src/webgl/3d_primitives.js Outdated Show resolved Hide resolved
@wong-justin
Copy link
Contributor Author

The past few commits are changes to drawing function docs, starting with line() and arc(). I'm trying to strike a balance between useful changes and consistency.

I also noticed that the season of docs contributor in #6178 is making a style guide for clear and consistent docs and snippets, and he'll be starting with 2D primitives too. So I feel like I should only make a few priority changes for now.

Comment on lines 378 to 387
* <div>
* <code>
* createCanvas(100, 100, WEBGL);
* strokeWeight(7);
* line(0, 0, 0, 30, 0, 0);
* line(0, 0, 0, 0, 30, 0);
* line(-30, -30, -30, 30, 30, 30);
* describe('two lines mark the flat x and y axes while a third line spans diagonally through the third dimension');
* </code>
* </div>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other code examples only used the x and y parameters for line(), so I added one using the z parameters for WEBGL mode.

I figure it's a good goal to show all the possible parameters in the code examples, which would mean having a 3D example for the 2D primitives.

Having these scaffolded examples might naturally help users transition to WEBGL. Eg. they might know the WEBGL coordinate system is different just from their time browsing 2D primitives examples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, it's nice to see the webGL versions with the updated signatures! If we don't think it's too much code I think it could be nice to color these lines in rgb (xyz) gizmo fashion like so https://editor.p5js.org/aferriss/sketches/qCsZ08lJ8

Copy link
Contributor

@aferriss aferriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes @wong-justin I think things are looking good!

Given that some doc style changes are around the corner, maybe it makes sense to separate out the code examples for webGL shape functions into a separate PR that can be updated to match the new style when / if those changes are made.

On the one hand I don't want to get blocked by that work, but also it would be a shame to have to make these changes once now and then once again when new guidance is available. What do you think @aceslowman ?

Comment on lines 175 to 183
* <div>
* <code>
* createCanvas(100, 100, WEBGL);
* arc(0, 0, 80, 80, 0, PI + QUARTER_PI, PIE, 7);
* describe(
* 'ellipse with a quarter missing, and the outline is segmented instead of curved'
* );
* </code>
* </div>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The round 2D primitives have a detail parameter, so technically for completeness they could use an example like this to show it in action.

But I think it would distract from useful parameters and examples, and detail doesn't seem like a popular feature for users, and detail is already documented well in curveDetail() and bezierDetail() and the 3D primitives.

Maybe this should make it into the list of WEBGL vs P2D differences? Something like:

  • Vertex Count - When drawing curved shapes in WEBGL mode, you can specify how many vertices are drawn with a detail parameter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of Vertex Count I think we can use p5's existing language and call it Shape Detail.

Also I don't think additional examples detracts and to be honest I didn't realize the round 2d shapes even had that parameter. I think it would be a good addition, and as you note, the 3d shapes include this, so why not 2d as well?

https://p5js.org/reference/#/p5/sphere

Copy link
Contributor

@aceslowman aceslowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great improvement, and the formatting works well / doesn't feel like too much text.

@aceslowman
Copy link
Contributor

Thanks for making the changes @wong-justin I think things are looking good!

Given that some doc style changes are around the corner, maybe it makes sense to separate out the code examples for webGL shape functions into a separate PR that can be updated to match the new style when / if those changes are made.

On the one hand I don't want to get blocked by that work, but also it would be a shame to have to make these changes once now and then once again when new guidance is available. What do you think @aceslowman ?

@aferriss @wong-justin I think separating it out into a new PR would be helpful, it could also be a good opportunity to communicate a bit with the style docs contributor. Personally I think continuing with doc changes makes sense, even if there is some revision needed down the line.

Also, I think that the text changes made on the WEBGL reference page are a great improvement, I don't feel like it's excessively wordy and it does a good job at linking out to further reading.

@nickmcintyre
Copy link
Member

@wong-justin @aferriss @aceslowman the changes you've made thus far are great improvements. I won't begin editing WebGL docs until August, so please go ahead with your work. I'll share a draft of the style guide later this week and will ping you for feedback.

Here is the draft Wording section:


Write simple, declarative sentences. Brevity is a plus: get to the point.

Write in present tense: "Returns an object that...", rather than "Returned an object that..." or "Will return an object that...".

Start comments in upper case. Follow regular punctuation rules:

// Draws a fractal from a Julia set.
function drawFractal(c, rad, maxIter) {
  // ...
}

Communicate to the reader the current way of doing things, both explicitly and implicitly. Use the idioms recommended in this guide. Reorder code samples to emphasize favored approaches if needed. The documentation should be a model for best practices and approachable for beginners.

Documentation has to be brief but comprehensive. Explore and document edge cases. What happens for the each combination of arguments? What bugs are most likely to appear in a beginner's code?

Spell names correctly: p5.js, CSS, HTML, JavaScript, WebGL. When in doubt, please have a look at some authoritative source like their official documentation.


In the docs, I propose we stick with WebGL unless you're referencing the WEBGL constant.

How should we incorporate canvas descriptions?

Narrowing scope for the PR.
This reverts commit 30d6fd4.
Narrowing scope for the PR.
This reverts commit 9dbe1c2.
Narrowing scope for the PR.
This reverts commit 021c565.
Narrowing scope for the PR.
This reverts commit f1a00c6.
@wong-justin wong-justin marked this pull request as ready for review June 20, 2023 14:36
@aferriss aferriss merged commit e9a28b9 into processing:main Jun 20, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants