LDrawLoader: Incorrect normals in function smoothNormals

[graywhale]

Describe the bug

I loaded part 6156.dat in example webgl_loader_ldraw, LDrawLoader renders black normals while enabled 'Smooth Normals' option, actually the calculated normals are incorrect. And if disabled the option, LDrawLoader renders correctly.

[6156.dat_Packed.mpd.txt](https://github.com/mrdoob/three.js/files/9553431/6156.dat_Packed.mpd.txt)

[graywhale]

Here is an example of the most simple part file, the first rectangle is incorrect, but the second one is correct.

0 Panel  1 x  4 x  3 with Glass
0 Name: 6156.dat
0 Author: Thomas Burger [grapeape]
0 !LDRAW_ORG Part UPDATE 1998-07
0 !LICENSE Redistributable under CCAL version 2.0 : see CAreadme.txt

0 !HISTORY 1998-07-15 [PTadmin] Official Update 1998-07
0 !HISTORY 2007-07-16 [PTadmin] Header formatted for Contributor Agreement
0 !HISTORY 2008-07-01 [PTadmin] Official Update 2008-01

4 16 40 72 10 31 48 10 -31 48 10 -40 72 10 
4 16 -40 72 10 -33 46 10 -33 6 10 -40 0 10 

[gkjohnson]

Thanks for isolating the issue to a small file! I will not have the bandwidth to investigate this but if you'd like to take a deeper look you can check out the "smoothNormals" function here. I'd take a look at what the final normal values are being set to, first, to see if that provides a bit more insight into what's going on.

cc @yomboprime if you're interested.

[graywhale]

@gkjohnson Thanks for your reply so quickly. I dived deeper in LDrawLoader, and found the issue occurs 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 ];

If don't push the reversed vertices, the normal values are null, computed by function smoothNormals.

My temporary solution: Disable the double side feature, in other words, remove or comment lines 1139-1150 / 1189-1200, and set material.side to THREE.DoubleSide instead.

I am not sure this is the best way. Are there new issues by this way, or low performance? And except the official solution.

[graywhale]

I found my temporary solution may cause low performance, I think it made by double sided material. So I use another variable to store the 'back side' vertices.

const backSideFaces = [];
...
if ( doubleSided === true ) {
  backSideFaces.push( {
	material: material,
	colorCode: colorCode,
	faceNormal: null,
	vertices: [ v3, v2, v1, v0 ],
	normals: [ null, null, null, null ],
  } );
  totalFaces += 2;
}

And merge the tow arrays faces and backSideFaces in the first line of the function createObject, which invoked after the function smoothNormals

[yomboprime]

I've tried to follow the process and didn't find an easy solution. It seems that the smoothNormals function is not compatible with the duplication of tris/quads. The best idea that came to me is to construct two separate triangle sets (original and duplicated triangles), compute smoothNormals on both separately, and then joining them (but not merging them).

Anyway this only happens if the model is not bfcCertified.

[yomboprime]

And merge the tow arrays faces and backSideFaces in the first line of the function createObject, which invoked after the function smoothNormals

Oh, sorry for not spotting that. That is exactly what I was meaning in my previous comment. I find it is the right solution. Could you post the entire modified code, or make a PR?

[graywhale]

@yomboprime Actually I completely rewrote the loader in my project, so I post the sample code here.

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

2. 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 ) );

}

NO NEED TO MODIFY the function smoothNormals .

It works correctly in my project so far. And I think it still needs more tests.

[LeviPesin]

Can you please file a PR with these changes?

[gkjohnson]

I dived deeper in LDrawLoader, and found the issue occurs with the Triangles or Quadrilateral which need double sides.

Gotcha - yes I don't believe there have been many meshes with double sided faces tested. What happens in the smoothNormals function is sibling edges are searched for so they can be considered connected and face normals averages for those vertices. When an equivalent backface is added then the backface edge is found as a sibling causing this issue.

Disable the double side feature, in other words, remove or comment lines 1139-1150 / 1189-1200, and set material.side to THREE.DoubleSide instead. ... I am not sure this is the best way. Are there new issues by this way, or low performance? And except the official solution.

Enabling double sided rendering always can have big performance implications and cause undesirable artifacts in some rendering scenarios. Either way we should be loading the model as the format specifies, which is a double sided face for only the geometry that it's specified for.

I found my temporary solution may cause low performance, I think it made by double sided material. So I use another variable to store the 'back side' vertices.

I think a good solution here would be to mark faces as "doubleSided", smooth face normals before having added the double sided face into the pool of candidates, and then when generating the final triangle geometry we add two faces for the double sided geometry - one for the front and one for the back.

[yomboprime]

I think a good solution here would be to mark faces as "doubleSided", smooth face normals before having added the double sided face into the pool of candidates, and then when generating the final triangle geometry we add two faces for the double sided geometry - one for the front and one for the back.

I can try to make a PR with that in a few days.

[gkjohnson]

@graywhale are you planning to make a PR to fix the issue and contribute it back to the project?

[graywhale]

@gkjohnson I have already made a PR, please check it via https://github.com/graywhale/three.js/commit/a26f1a677848dc8503567355c035deb938a4354c. Actually, this is my first PR, if there is missing something, please let me know, thanks.

[gkjohnson]

That is a git commit - if you would like to make a contribution to three.js you need to make a PR into the mrdoob/three.js repo. There is some documentation here on how to make a Github PR if this is your first time:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request

[graywhale]

OK, I'll make it in a few hours, thanks.

[yomboprime]

OK, I'll make it in a few hours, thanks.

Did you have any progress? If not, I could give it a try tomorrow.

[graywhale]

OK, I'll make it in a few hours, thanks.

Did you have any progress? If not, I could give it a try tomorrow.

I've done it, please refer https://github.com/mrdoob/three.js/pull/24643. And I will make another PR, and follow @gkjohnson 's suggestion in a few days.