LDrawLoader: Fix incorrect normals on double sided faces
Related issue: mrdoob#24635
Description
The function smoothNormals in LDrawLoader.js got incorrect normals while the model is not bfcCertified incorrectly.
The issue happens with the Triangles or Quadrilateral which need double sides.
In LDrawLoader.js:
Lines 1139-1150 / 1189-1200, pushes a reversed vertices to the variable faces array.
if ( doubleSided === true ) {
faces.push( {
material: material,
colorCode: colorCode,
faceNormal: null,
vertices: [ v3, v2, v1, v0 ],
normals: [ null, null, null, null ],
} );
totalFaces += 2;
}
And in the function smoothNormals finds RESERVED vertices, at the lines 421 &422, and it causes the final computed normal values are an array with Zero Vectors.
const reverseHash = hashEdge( v1, v0 );
const otherInfo = halfEdgeList[ reverseHash ];
There are conflicts about RESERVED vertices, so introduce a new array to store the 'back side faces' and merge it to the origin faces array after invoking the function smoothNormals.
- Refactor the function
LDrawParsedCache.parse:
const faces = [];
const backSideFaces = []; // Define the array which stores the 'back side faces' at line 783
...
// Add the back side face of triangle to backSideFaces instead of faces
if ( doubleSided === true ) {
backSideFaces.push( { // Line 1142
material: material,
colorCode: colorCode,
faceNormal: null,
vertices: [ v2, v1, v0 ],
normals: [ null, null, null, null ],
} );
totalFaces += 2;
}
...
// Add the back side face of quadrilateral
if ( doubleSided === true ) {
backSideFaces.push( { // Line 1192
material: material,
colorCode: colorCode,
faceNormal: null,
vertices: [ v3, v2, v1, v0 ],
normals: [ null, null, null, null ],
} );
totalFaces += 2;
}
...
return {
faces,
backSideFaces, // Export the property
...
};
- Refactor the function
LDrawPartsGeometryCache.processIntoMesh:
// Start at line 1504
if ( info.faces.length > 0 ) {
if ( info.backSideFaces && info.backSideFaces.length > 0 ) {
info.faces.push( ...info.backSideFaces ); // Append info.backSideFaces to info.faces
}
group.add( createObject( info.faces, 3, false, info.totalFaces ) );
}
Awesome! Thanks for making a PR. Have you considered the solution I suggested in https://github.com/mrdoob/three.js/issues/24635#issuecomment-1245628352? Ie instead of adding two faces here we can just add a field to the face that marks it as "double sided" and then in createObject we can add the double sided faces twice - with the second have inverted winding order and normals.
The issue with the implementation in this PR is that the backsides of the double sided faces currently will not have smooth normals while the front ones will. With my proposed solution we will make sure the frontsides have the normals smoothed first and then the backsides can be added once the normals have been computed.
Awesome! Thanks for making a PR. Have you considered the solution I suggested in #24635 (comment)? Ie instead of adding two faces here we can just add a field to the face that marks it as "double sided" and then in
createObjectwe can add the double sided faces twice - with the second have inverted winding order and normals.The issue with the implementation in this PR is that the backsides of the double sided faces currently will not have smooth normals while the front ones will. With my proposed solution we will make sure the frontsides have the normals smoothed first and then the backsides can be added once the normals have been computed.
Yes, this solution just make sure the front sides have the normals smoothed, and I will follow your suggestion in a few days, 'cause I got a task need to finish first.