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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Nov 17, 2023
Merged

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

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d299497
update computeNormals to do smooth shading for buildGeometry outputs
capGoblin Nov 12, 2023
465ef05
update computeNormals inline docs
capGoblin Nov 12, 2023
2db604d
update computeNormals inline docs
capGoblin Nov 12, 2023
d1529df
addressed all the suggestions
capGoblin Nov 15, 2023
e087f94
fix edge undefined
capGoblin Nov 16, 2023
edb3db6
refactor comments
capGoblin Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 37 additions & 7 deletions src/webgl/p5.Geometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,17 +185,47 @@ p5.Geometry = class Geometry {
return n.mult(Math.asin(sinAlpha) / ln);
}
/**
* computes smooth normals per vertex as an average of each
* face.
* @method computeNormals
* @chainable
*/
computeNormals() {
* computes smooth normals per vertex as an average of each
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* face.
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

* @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'.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

* @chainable
davepagurek marked this conversation as resolved.
Show resolved Hide resolved
*/
computeNormals(shadingType = 'FLAT', { roundToPrecision = 3 } = {}) {
const vertexNormals = this.vertexNormals;
const vertices = this.vertices;
let vertices = this.vertices;
const faces = this.faces;
let iv;

if (shadingType === 'SMOOTH') {
const vertexIndices = {};
const uniqueVertices = [];

// 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)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

if (vertexIndices[key] === undefined) {
vertexIndices[key] = uniqueVertices.length;
uniqueVertices.push(vertex);
}
}

// update face indices to use the new deduplicated vertex indices
faces.forEach(face => {
for (let fv = 0; fv < 3; ++fv) {
const originalVertexIndex = face[fv];
const originalVertex = vertices[originalVertexIndex];
const key = `${originalVertex.x.toFixed(roundToPrecision)},${originalVertex.y.toFixed(roundToPrecision)},${originalVertex.z.toFixed(roundToPrecision)}`;
face[fv] = vertexIndices[key];
}
});

// update the deduplicated vertices
vertices = uniqueVertices;
}

// initialize the vertexNormals array with empty vectors
vertexNormals.length = 0;
for (iv = 0; iv < vertices.length; ++iv) {
Expand Down