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

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

Merged
Merged
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 132 additions & 7 deletions src/webgl/p5.Geometry.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,17 +185,142 @@ 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() {
* 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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

* When using 'FLAT' shading, vertices are disconnected/duplicated i.e each face has its own copy of vertices.<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

* When using 'SMOOTH' shading, vertices are connected/deduplicated i.e each face has its vertices shared with other faces.<br>
*
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* @chainable
davepagurek marked this conversation as resolved.
Show resolved Hide resolved
*
* @example
* <div>
* <code>
* let helix;
*
* function setup() {
* createCanvas(100, 100, WEBGL);
*
* helix = buildGeometry(() => {
* beginShape();
*
* for (let i = 0; i < TWO_PI * 3; i += 0.1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

* let radius = 20;
* let x = cos(i) * radius;
* let y = sin(i) * radius;
* let z = i * 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

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);?

* vertex(x, y, z);
* }
* endShape();
* });
* helix.computeNormals();
* }
* function draw() {
* background(255);
* stroke(0);
* fill(150, 200, 250);
* lights();
* orbitControl();
* model(helix);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

* }
* </code>
* </div>
*
* @alt
* A 3D helix using the computeNormals() function by default uses 'FLAT' to create a flat shading effect on the helix.
*
* @example
* <div>
* <code>
* let star;
*
* function setup() {
* createCanvas(100, 100, WEBGL);
*
* star = buildGeometry(() => {
* beginShape();
* for (let i = 0; i < TWO_PI; i += PI / 5) {
* let outerRadius = 60;
* let innerRadius = 30;
* let xOuter = cos(i) * outerRadius;
* let yOuter = sin(i) * outerRadius;
* let zOuter = random(-20, 20);
* vertex(xOuter, yOuter, zOuter);
*
* let nextI = i + PI / 5 / 2;
* let xInner = cos(nextI) * innerRadius;
* let yInner = sin(nextI) * innerRadius;
* let zInner = random(-20, 20);
* vertex(xInner, yInner, zInner);
* }
* endShape(CLOSE);
* });
* star.computeNormals('SMOOTH');
* }
* function draw() {
* background(255);
* stroke(0);
* fill(150, 200, 250);
* lights();
* orbitControl();
* model(star);
* }
* </code>
* </div>
*
* @alt
* A star-like geometry, here the computeNormals('SMOOTH') is applied for a smooth shading effect.
* This helps to avoid the faceted appearance that can occur with flat shading.
*/
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 = [];

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

// loop through each vertex and add uniqueVertices
for (let i = 0; i < vertices.length; i++) {
const vertex = vertices[i];
const key = getKey(vertex);
if (vertexIndices[key] === undefined) {
vertexIndices[key] = uniqueVertices.length;
uniqueVertices.push(vertex);
}
}

// update face indices to use the deduplicated vertex indices
faces.forEach(face => {
for (let fv = 0; fv < 3; ++fv) {
const originalVertexIndex = face[fv];
const originalVertex = vertices[originalVertexIndex];
const key = getKey(originalVertex);
face[fv] = vertexIndices[key];
}
});

// update edge indices to use the deduplicated vertex indices
this.edges.forEach(edge => {
for (let ev = 0; ev < 2; ++ev) {
const originalVertexIndex = edge[ev];
const originalVertex = vertices[originalVertexIndex];
const key = getKey(originalVertex);
edge[ev] = vertexIndices[key];
}
});

// update the deduplicated vertices
this.vertices = vertices = uniqueVertices;
}

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