Update MeshHelper to fix #8140 #8307
Conversation
fixed CalculateNormal indexing
|
@grasshopperhamburger to ensure the resolution is correcting an issue, can you also please add a Unit test to validate what this PR is resolving please |
|
@SimonDarksideJ |
|
If it helps prove the case better then yes, as it then protects against such changes in the future. Ideally, make that a separate pr, so the test shows as failing, then retest with these changes to prove it now passes. |
| @@ -95,9 +95,9 @@ public static void CalculateNormals(GeometryContent geom, bool overwriteExisting | |||
| // by Shuangshuang Jin, Robert R. Lewis, David West. | |||
| // | |||
|
|
|||
| normals[positionIndices[ia]] += faceNormal; | |||
There was a problem hiding this comment.
@grasshopperhamburger what makes you say that positionIndices is the parent positions?
If that is correct then certainly this code was wrong and this is the right fix.
| @@ -95,9 +95,9 @@ public static void CalculateNormals(GeometryContent geom, bool overwriteExisting | |||
| // by Shuangshuang Jin, Robert R. Lewis, David West. | |||
There was a problem hiding this comment.
@github managed to screw up code review again.
@grasshopperhamburger If you look up at line 65:
var normals = new Vector3[positionIndices.Count];
Is this allocation wrong then? We should have the same amount of normals as we do positions in the current mesh.
There was a problem hiding this comment.
Sorry for the delay.
It is not wrong, every channel has a length equal to the positionIndices (and VertexContent.Positions) collection (for a given geometry), which in turn is less or equal the actual positions (stored in MeshContent.Positions) once the mesh is finalized. It may be less (such as the case in the failing test) when between two geometries (same mesh) there is no full overlap of used vertices
|
So yeah i don't remember this code well enough to say if this is right or not @SimonDarksideJ . A unit test would be best to verify the old code was broken and the new code works. |
…TestMergePositionsMultipleGeometries
Hello,
I've modified MeshHelper to address the issue mentioned in #8140 .
Based on my understanding, the values in each of the VertexContent "Channels" should be indexed to match their corresponding vertex in the "Positions" index.
The previous code incorrectly indexed them using the vertex positions in the parent Mesh collection.