Update computeNormals() to support smooth shading for buildGeometry() outputs #6553

capGoblin · 2023-11-12T17:11:19Z

Resolves #6494

Update computeNormals to support smooth shading feature for buildGeometry outputs by incorporating vertex deduplication. Additionally, a new setting object, roundToPrecision, to enable rounding of vertex coordinates, addressing potential slight differences in calculations.

Changes:

Added support to smooth shading for buildGeometry() outputs by vertex deduplication.
Introduced the roundToPrecision setting for precise vertex coordinate rounding.
Updated inline function docs for the both shadingType and settings object params

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

capGoblin · 2023-11-12T17:22:12Z

I've revised the function documentation to accommodate the optional parameters. Definitely need feedback or suggestions for improvement on that.
And any other changes needed for the code?

davepagurek

Great work so far, this is looking good! Just a few comments to clean up the code a bit and explain to users how to use the feature.

davepagurek · 2023-11-13T12:51:04Z

src/webgl/p5.Geometry.js

+   * face.
+   * @method computeNormals
+   * @param {String} [shadingType] (optional) Shading type ('FLAT' for flat shading or 'SMOOTH' for smooth shading) for buildGeometry() outputs.
+   * @param {Object} [settings={ roundToPrecision: 3 }] (optional) Additional settings object with rounding precision for vertex coordinates when shadingType is 'SMOOTH'.


Maybe we can try to match the style used for the options object in textToPoints?

p5.js/src/typography/p5.Font.js

Line 278 in bd42ed1

* @param {Object} [options] object with sampleFactor and simplifyThreshold

THe type is just Object and the name is just the name, and above the parameters, they list all the options it can include and the default values.

davepagurek · 2023-11-13T12:51:43Z

src/webgl/p5.Geometry.js

- * @chainable
- */
-  computeNormals() {
+   * computes smooth normals per vertex as an average of each


While we're augmenting this method's docs, a quick capitalization fix:

Suggested change

* computes smooth normals per vertex as an average of each

* Computes smooth normals per vertex as an average of each

src/webgl/p5.Geometry.js

davepagurek · 2023-11-13T12:57:44Z

src/webgl/p5.Geometry.js

+      // loop through each vertex and add uniqueVertices
+      for (let i = 0; i < vertices.length; i++) {
+        const vertex = vertices[i];
+        const key = `${vertex.x.toFixed(roundToPrecision)},${vertex.y.toFixed(roundToPrecision)},${vertex.z.toFixed(roundToPrecision)}`;


We have this big string here and then again on line 220. Maybe to make sure they're always in sync, we can put something like this above (maybe around line 200):

const getKey = ({ x, y, z }) => `${x.toFixed(roundToPrecision)},${y.toFixed(roundToPrecision)},${z.toFixed(roundToPrecision)}`;

...so then here we could do const key = getKey(vertex) (and something similar on line 220)

davepagurek · 2023-11-13T13:00:36Z

src/webgl/p5.Geometry.js

- */
-  computeNormals() {
+   * computes smooth normals per vertex as an average of each
+   * face.


Maybe we can add a sentence or two explaining how this works, so that users will know what to expect from it? I think the information would be:

Normals are calculated for each face, and each vertex's normal is the average of its connected face normals

Flat shading leaves duplicated vertices disconnected, while smooth shading reconnects duplicated vertices so their face normals average together (or some other way of explaining this difference that makes sense to you)

davepagurek · 2023-11-17T12:51:09Z

src/webgl/p5.Geometry.js

-  computeNormals() {
+   * This function calculates normals for each face, where each vertex's normal is the average of the normals of all faces it's connected to.
+   * i.e computes smooth normals per vertex as an average of each face.<br>
+   * When using 'FLAT' shading, vertices are disconnected/duplicated i.e each face has its own copy of vertices.<br>


Right now for other methods like createFramebuffer (https://p5js.org/reference/#/p5/createFramebuffer), rather than passing in the string 'FLAT', we create a variable in constants.js like const FLAT = 'flat' and then in the code, check if the parameter is equal to constants.FLAT. That way users can pass it in as just FLAT (without quotes). Do you think we can do that for flat/smooth too for consistency?

davepagurek · 2023-11-17T12:52:49Z

src/webgl/p5.Geometry.js

- */
-  computeNormals() {
+   * This function calculates normals for each face, where each vertex's normal is the average of the normals of all faces it's connected to.
+   * i.e computes smooth normals per vertex as an average of each face.<br>


Generally in other docs, rather than explicitly adding a <br>, we just add a blank line between paragraphs (well, blank except for the * from the doc comment.) Maybe we can do this here too?

davepagurek · 2023-11-17T12:54:13Z

src/webgl/p5.Geometry.js

+   *   helix = buildGeometry(() => {
+   *     beginShape();
+   *
+   *     for (let i = 0; i < TWO_PI * 3; i += 0.1) {


Right now, this example kind of looks smooth just because there are a lot of triangles and the canvas is small. Maybe we can use a larger step size (0.6?) to make the flat shading more apparent?

davepagurek · 2023-11-17T12:56:30Z

src/webgl/p5.Geometry.js

+   *       let radius = 20;
+   *       let x = cos(i) * radius;
+   *       let y = sin(i) * radius;
+   *       let z = i * 10;


orbitControl feels a bit off on this one right now because the z values aren't centered around 0, maybe we could use something like let z = map(i, 0, TWO_PI * 3, -30, 30);?

davepagurek · 2023-11-17T12:57:25Z

src/webgl/p5.Geometry.js

+   *   fill(150, 200, 250);
+   *   lights();
+   *   orbitControl();
+   *   model(helix);


It would also be good to add an initial rotation (maybe rotateX(PI*0.2)) so you can see immediately that this is 3D. Maybe same for the other example too

davepagurek · 2023-11-17T12:59:44Z

src/webgl/p5.Geometry.js

+   *
+   * @method computeNormals
+   * @param {String} [shadingType] shading type ('FLAT' for flat shading or 'SMOOTH' for smooth shading) for buildGeometry() outputs.
+   * @param {Object} [options] object with roundToPrecision property.


Rather than describing roundToPrecision here, can we describe the options in the descriotions above similar to how this doc does it? https://p5js.org/reference/#/p5/createFramebuffer

davepagurek

Great work, thanks for all your updates!

capGoblin added 3 commits November 12, 2023 21:03

update computeNormals to do smooth shading for buildGeometry outputs

d299497

update computeNormals inline docs

465ef05

update computeNormals inline docs

2db604d

davepagurek reviewed Nov 13, 2023

View reviewed changes

capGoblin added 2 commits November 15, 2023 22:10

addressed all the suggestions

d1529df

fix edge undefined

e087f94

davepagurek reviewed Nov 17, 2023

View reviewed changes

refactor comments

edb3db6

davepagurek approved these changes Nov 17, 2023

View reviewed changes

davepagurek merged commit 7246a60 into processing:main Nov 17, 2023
2 checks passed

capGoblin deleted the enhancement/computeNormals-deduplication branch November 17, 2023 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update computeNormals() to support smooth shading for buildGeometry() outputs #6553

Update computeNormals() to support smooth shading for buildGeometry() outputs #6553

capGoblin commented Nov 12, 2023

capGoblin commented Nov 12, 2023

davepagurek left a comment

davepagurek Nov 13, 2023

davepagurek Nov 13, 2023

davepagurek Nov 13, 2023

davepagurek Nov 13, 2023

davepagurek Nov 17, 2023

davepagurek Nov 17, 2023

davepagurek Nov 17, 2023

davepagurek Nov 17, 2023

davepagurek Nov 17, 2023

davepagurek Nov 17, 2023

davepagurek left a comment

	* computes smooth normals per vertex as an average of each
	* Computes smooth normals per vertex as an average of each

Update computeNormals() to support smooth shading for buildGeometry() outputs #6553

Update computeNormals() to support smooth shading for buildGeometry() outputs #6553

Conversation

capGoblin commented Nov 12, 2023

PR Checklist

capGoblin commented Nov 12, 2023

davepagurek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davepagurek left a comment

Choose a reason for hiding this comment