-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix textToModel face normals for extruded text #8091
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
base: dev-2.0
Are you sure you want to change the base?
Conversation
@davepagurek Could you please take a look at this PR and provide a review? Thanks! |
Hi! So far this change just reverses the order of the vertices in front faces; I don't think we need to change that. I left a comment here #8074 (comment) explaining the approach I had in mind, which likely involves changing this part of the code that constructs the geometry: Lines 547 to 588 in fc8ca8a
Currently there's an if/else that uses a different approach if extruding vs if creating a flat model. Instead, we'd probably always start with the flat model, but if we need extrusion, you could read the |
@davepagurek can you review this now its working perfectly i add your recommendation of making a new geometry |
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.
This is looking good! I left a few little comments below. I think the big thing left to figure out is the normals. The problem currently is that each side face has its own normal, so the sides will look faceted instead of smooth.
Ideally, I'd like us to be able to just call extruded.computeNormals()
and have it generate all the normals for us. It works per vertex by averaging the normals of the faces sharing that vertex. In order to do that, this means:
- the side faces need to share vertices with adjacent side faces. This means adding all the vertices on each side to
extruded.vertices
once, and then adding faces that reference the vertex indices for each of those rather than creating brand new vertices in each face. - the side faces cannot share any vertices with the front and back faces. This is because if they do, the edge between the front/back and the sides will be smoothed, but we want a crisp edge there.
Let me know if you need any pointers on how to accomplish that!
this._pInst.beginShape(); | ||
this._pInst.normal(0, 0, 1); | ||
for (const contour of contours) { | ||
const outer = glyphContours[0]; |
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 2.0, you can surround every contour with begin/endContour
including the first one! so you dont have to special case the first contour. How the code used to work will suffice here:
for (const contour of contours) {
this._pInst.beginContour();
for (const { x, y } of contour) {
this._pInst.vertex(x, y);
}
this._pInst.endContour(this._pInst.CLOSE);
}
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.
Thanks for pointing this out! I updated the code so every contour uses beginContour/endContour, including the first one.and I originally added stroke(0) with a push/pop to make sure edges are generated consistently.
Following your suggestion, I’ll keep only what’s needed and avoid forcing stroke unless required.
const geom = this._pInst.buildGeometry(() => {
const prevValidateFaces = this._pInst._renderer._validateFaces;
this._pInst._renderer._validateFaces = true;
this._pInst.push();
this._pInst.stroke(0);
contours.forEach(glyphContours => {
this._pInst.beginShape();
for (const contour of glyphContours) {
this._pInst.beginContour();
contour.forEach(({ x, y }) => this._pInst.vertex(x, y, 0));
this._pInst.endContour(this._pInst.CLOSE);
}
this._pInst.endShape(this._pInst.CLOSE);
});
this._pInst.pop();
this._pInst._renderer._validateFaces = prevValidateFaces;
});
i add this adding stroke 0 always and also add push
}); | ||
|
||
if (extrude === 0) { | ||
console.log('No extrusion'); |
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.
We should take this out before merging so we don't clutter users' console.
extruded.faces = []; | ||
|
||
let vertexIndex = 0; | ||
const Vector = this._pInst.constructor.Vector; |
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.
We can probably just import { Vector } from '../math/p5.Vector';
at the top so that we don't need to import it through here
// Side faces from edges | ||
let edges = geom.edges; | ||
if (!edges || !Array.isArray(edges)) { | ||
edges = []; |
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.
Was there a scenario when edges
was empty? We can try temporarily setting stroke(0)
(within a push
/pop
) when generating the initial geometry to ensure that edges are present if we need. I think if we can do that and guarantee the edges exist, we can take out this block of code.
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 had a fallback in case edges was empty, but since you mentioned we can guarantee edges by enabling stroke inside a push/pop, I simplified this block and removed the redundant fallback.
const validEdges = geom.edges.filter(([a, b]) => a !== b);
for (const [a, b] of validEdges) {
const v0 = geom.vertices[a];
const v1 = geom.vertices[b];
const vFront0 = new Vector(v0.x, v0.y, v0.z + half);
const vFront1 = new Vector(v1.x, v1.y, v1.z + half);
const vBack0 = new Vector(v0.x, v0.y, v0.z - half);
const vBack1 = new Vector(v1.x, v1.y, v1.z - half);
extruded.faces.push([vFront0, vBack0, vBack1]);
extruded.faces.push([vFront0, vBack1, vFront1]);
}
@davepagurek The extrusion is still not rendering correctly on the top and bottom faces. In my earlier changes, the top and bottom rendered perfectly, but in this version they only work if I comment out extruded.computeNormals(). It seems that computeNormals() is overriding the normals in a way that breaks the top/bottom faces I think keeping of eeping the front and back faces with their own independent vertices and make the side faces share vertices with each other, so their normals average smoothly.. this is the code
|
@davepagurek I think the problem could be duplication of normals for the top and bottom faces when using separate side vertices. When we create new vertices for the sides, each side face has its own independent vertices, so computeNormals() cannot average normals across adjacent side faces. This causes the sides to look faceted and can even distort the normals for the top and bottom faces.I also noticed that the extrusion renders correctly if we reuse the existing front and back vertices for the side faces instead of creating new ones.
Am I on the right track? Which approach aligns better? |
The first approach you mentioned looks right -- we intentionally don't want the front/back faces to share vertices with the sides so that there is a crisp edge. Can you send a screenshot of what the shading look like with that so I get a clearer idea of the problem? Some thoughts though:
|
@davepagurek it looks same as original issue have top and bottom dont render normals properly its really getting way complex triying to figure out now |
@davepagurek ok so I Initialized vertexNormals array manually and assigned exact normals of [0,0,1] to all front vertices and [0,0,-1] to all back vertices when creating them. This eliminates any numerical issues since we know these faces should be perfectly flat.Reordered the face creation process. Side faces are now added first, then computeNormals() is called which only affects the side vertices since front and back vertices already have their normals set. After that, front and back faces are added to the faces array. This optimization means computeNormals() doesn't waste computation on faces where we already know the exact normals.Reversed the winding order of back faces by swapping face[i] and face[i+1] in the back face loop. This ensures that when viewed from outside the geometry, the back faces have vertices ordered counterclockwise (opposite to the front faces), which is necessary for normals to point in opposite directions.I think that allign to your thoughts still it giving same rendering dont able to figure it now
|
Resolves #8074
Changes:
Screenshots of the change:
Screencast.From.2025-09-15.15-30-14.mp4
PR Checklist
npm run lint
passes