From b264931d93cd553cd1f41ad06da45a4256e27988 Mon Sep 17 00:00:00 2001 From: Tom Fili Date: Thu, 23 Feb 2017 17:53:19 -0500 Subject: [PATCH 1/3] Fix issue where we can't generate correct normals. Triangles are either degenerate or part of coplanar triangles with opposite winding order. --- Source/Core/GeometryPipeline.js | 26 ++++++++++++------ Specs/Core/GeometryPipelineSpec.js | 44 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 9 deletions(-) diff --git a/Source/Core/GeometryPipeline.js b/Source/Core/GeometryPipeline.js index ed8427ec4602..de4051e1407b 100644 --- a/Source/Core/GeometryPipeline.js +++ b/Source/Core/GeometryPipeline.js @@ -1173,20 +1173,28 @@ define([ for (i = 0; i < numVertices; i++) { var i3 = i * 3; vertexNormalData = normalsPerVertex[i]; + Cartesian3.clone(Cartesian3.ZERO, normal); if (vertexNormalData.count > 0) { - Cartesian3.clone(Cartesian3.ZERO, normal); for (j = 0; j < vertexNormalData.count; j++) { Cartesian3.add(normal, normalsPerTriangle[normalIndices[vertexNormalData.indexOffset + j]], normal); } - Cartesian3.normalize(normal, normal); - normalValues[i3] = normal.x; - normalValues[i3 + 1] = normal.y; - normalValues[i3 + 2] = normal.z; - } else { - normalValues[i3] = 0.0; - normalValues[i3 + 1] = 0.0; - normalValues[i3 + 2] = 1.0; + + // We can run into an issue where a vertex is used with 2 primitives that have opposite winding order. + if (Cartesian3.equals(Cartesian3.ZERO, normal)) { + Cartesian3.clone(normalsPerTriangle[normalIndices[vertexNormalData.indexOffset]], normal); + } } + + // We end up with a zero vector probably because of a degenerate triangle + if (Cartesian3.equals(Cartesian3.ZERO, normal)) { + // Default to (0,0,1) + normal.z = 1.0; + } + + Cartesian3.normalize(normal, normal); + normalValues[i3] = normal.x; + normalValues[i3 + 1] = normal.y; + normalValues[i3 + 2] = normal.z; } geometry.attributes.normal = new GeometryAttribute({ diff --git a/Specs/Core/GeometryPipelineSpec.js b/Specs/Core/GeometryPipelineSpec.js index 31005ab93ec2..00ce6cfcb001 100644 --- a/Specs/Core/GeometryPipelineSpec.js +++ b/Specs/Core/GeometryPipelineSpec.js @@ -1410,6 +1410,50 @@ defineSuite([ expect(Cartesian3.fromArray(normals, 18)).toEqualEpsilon(Cartesian3.negate(Cartesian3.UNIT_Z, new Cartesian3()), CesiumMath.EPSILON7); }); + it('computeNormal computes normal of (0,0,1) for a degenerate triangle', function() { + var geometry = new Geometry({ + attributes: { + position: new GeometryAttribute({ + values: [0, 0, 0, 1, 0, 0], + componentsPerAttribute: 3, + componentDatatype : ComponentDatatype.FLOAT + }) + }, + indices : [0, 1, 0], + primitiveType: PrimitiveType.TRIANGLES + }); + + geometry = GeometryPipeline.computeNormal(geometry); + + expect(geometry.attributes.normal.values.length).toEqual(2*3); + expect(geometry.attributes.normal.values).toEqual([0, 0, 1, 0, 0, 1]); + }); + + it('computeNormal takes first normal for two coplanar triangles with opposite winding orders', function() { + var geometry = new Geometry({ + attributes: { + position: new GeometryAttribute({ + values: [0, 0, 0, 1, 0, 1, 1, 1, 1], + componentsPerAttribute: 3, + componentDatatype : ComponentDatatype.FLOAT + }) + }, + indices : [0, 1, 2, 2, 1, 0], + primitiveType: PrimitiveType.TRIANGLES + }); + + geometry = GeometryPipeline.computeNormal(geometry); + + var normals = geometry.attributes.normal.values; + expect(normals.length).toEqual(3*3); + + var a = Cartesian3.normalize(new Cartesian3(-1, 0, 1), new Cartesian3()); + + expect(Cartesian3.fromArray(normals, 0)).toEqualEpsilon(a, CesiumMath.EPSILON7); + expect(Cartesian3.fromArray(normals, 3)).toEqualEpsilon(a, CesiumMath.EPSILON7); + expect(Cartesian3.fromArray(normals, 6)).toEqualEpsilon(a, CesiumMath.EPSILON7); + }); + it('computeTangentAndBitangent throws when geometry is undefined', function() { expect(function() { GeometryPipeline.computeTangentAndBitangent(); From 37d0c4781f5ec5e02282c5b8ea059d6a3c320433 Mon Sep 17 00:00:00 2001 From: Tom Fili Date: Fri, 24 Feb 2017 16:48:29 -0500 Subject: [PATCH 2/3] Tweak. --- Source/Core/GeometryPipeline.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/GeometryPipeline.js b/Source/Core/GeometryPipeline.js index de4051e1407b..4bde63a3fe15 100644 --- a/Source/Core/GeometryPipeline.js +++ b/Source/Core/GeometryPipeline.js @@ -1186,7 +1186,7 @@ define([ } // We end up with a zero vector probably because of a degenerate triangle - if (Cartesian3.equals(Cartesian3.ZERO, normal)) { + if (Cartesian3.equalsEpsilon(Cartesian3.ZERO, normal, CesiumMath.EPSILON10)) { // Default to (0,0,1) normal.z = 1.0; } From b33ff44515b8202f706979a5fea7514f698dfc4e Mon Sep 17 00:00:00 2001 From: Tom Fili Date: Fri, 24 Feb 2017 17:08:52 -0500 Subject: [PATCH 3/3] Tweak. --- Source/Core/GeometryPipeline.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/GeometryPipeline.js b/Source/Core/GeometryPipeline.js index 4bde63a3fe15..96badd51a121 100644 --- a/Source/Core/GeometryPipeline.js +++ b/Source/Core/GeometryPipeline.js @@ -1180,7 +1180,7 @@ define([ } // We can run into an issue where a vertex is used with 2 primitives that have opposite winding order. - if (Cartesian3.equals(Cartesian3.ZERO, normal)) { + if (Cartesian3.equalsEpsilon(Cartesian3.ZERO, normal, CesiumMath.EPSILON10)) { Cartesian3.clone(normalsPerTriangle[normalIndices[vertexNormalData.indexOffset]], normal); } }