Conversation
| // if we found a sibling edge add it into the vertex array if | ||
| // it meets the angle threshold and delete the edge from the map. | ||
| if (glvec3.dot(normal, edgeData[reverseHash].normal) <= thresholdAngle) { | ||
| vertices.push(v0[0], v0[1], v0[2]); |
There was a problem hiding this comment.
Since you are converting vertices to Float32Array later, make a block-based allocator with typed array could make gc happy.
There was a problem hiding this comment.
I think this is OK as-is for now — it's a new feature and can be optimized later.
donmccurdy
left a comment
donmccurdy
left a comment
There was a problem hiding this comment.
Thank you @WallanceLee, and sorry for the slow review! I'd be excited to get this feature merged, and have left some feedback on the PR in comments below. If any of the suggestions are confusing or feel like more than you can do right now, let me know and we'll figure something out. :)
| // if we found a sibling edge add it into the vertex array if | ||
| // it meets the angle threshold and delete the edge from the map. | ||
| if (glvec3.dot(normal, edgeData[reverseHash].normal) <= thresholdAngle) { | ||
| vertices.push(v0[0], v0[1], v0[2]); |
There was a problem hiding this comment.
I think this is OK as-is for now — it's a new feature and can be optimized later.
|
|
||
| const edgePrim = document | ||
| .createPrimitive() | ||
| .setName(prim.getName() + '_edges') |
There was a problem hiding this comment.
Surprisingly, glTF primitives do not have names, so this line has no effect on export and can be removed. The .setName() method in glTF Transform exists only as inherited from the Property class.
| .createPrimitive() | ||
| .setName(prim.getName() + '_edges') | ||
| .setMode(Primitive.Mode.LINES) | ||
| .setAttribute('POSITION', accesor); |
There was a problem hiding this comment.
Perhaps ideally, all vertex attributes from the source primitive would be preserved in the output, but I recognize that's a fairly tricky change. For example, so that edges of a skinned mesh can be animated along with the rest of the mesh. Feel free to skip it and we can include it as a later improvement!
|
OK, I will try my best to optimize the code. |
|
Is there anything to be done? |
3 participants
No description provided.