three.js icon indicating copy to clipboard operation
three.js copied to clipboard

LDrawLoader: Fix incorrect normals on double sided faces

Open graywhale opened this issue 3 years ago • 2 comments

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.

iShot_2022-09-13_12 02 48 iShot_2022-09-13_12 03 07

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.

  1. 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
  ...
};
  1. 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 ) );

}
iShot_2022-09-13_15 27 02 iShot_2022-09-13_15 28 00

graywhale avatar Sep 15 '22 02:09 graywhale

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.

gkjohnson avatar Sep 15 '22 03:09 gkjohnson

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 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.

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.

graywhale avatar Sep 15 '22 03:09 graywhale