Merge pull request #12255 from p-skakun/fix-collinear-lines-corners

Fix for possible normalization issue for line corner points
CesiumGS · Oct 30, 2024 · 6e6ed43 · 6e6ed43
2 parents 192ab06 + 3af8393
commit 6e6ed43
Show file tree

Hide file tree

Showing 5 changed files with 102 additions and 6 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -32,6 +32,8 @@
 - Fix flickering issue caused by bounding sphere retrieval being blocked by the bounding sphere of another entity. [#12230](https://github.com/CesiumGS/cesium/pull/12230)
 - Fixed `ImageBasedLighting.imageBasedLightingFactor` not affecting lighting. [#12129](https://github.com/CesiumGS/cesium/pull/12129)
 
+- Fix error with normalization of corner points for lines and corridors with collinear points. [#12255](https://github.com/CesiumGS/cesium/pull/12255)
+
 ### 1.122 - 2024-10-01
 
 #### @cesium/engine

diff --git a/packages/engine/Source/Core/CorridorGeometryLibrary.js b/packages/engine/Source/Core/CorridorGeometryLibrary.js
@@ -256,10 +256,6 @@ CorridorGeometryLibrary.computePositions = function (params) {
       Cartesian3.subtract(nextPosition, position, forward),
       forward,
     );
-    cornerDirection = Cartesian3.normalize(
-      Cartesian3.add(forward, backward, cornerDirection),
-      cornerDirection,
-    );
 
     const forwardProjection = Cartesian3.multiplyByScalar(
       normal,
@@ -284,6 +280,10 @@ CorridorGeometryLibrary.computePositions = function (params) {
     );
 
     if (doCorner) {
+      cornerDirection = Cartesian3.normalize(
+        Cartesian3.add(forward, backward, cornerDirection),
+        cornerDirection,
+      );
       cornerDirection = Cartesian3.cross(
         cornerDirection,
         normal,

diff --git a/packages/engine/Source/Core/PolylineVolumeGeometryLibrary.js b/packages/engine/Source/Core/PolylineVolumeGeometryLibrary.js
@@ -434,8 +434,6 @@ PolylineVolumeGeometryLibrary.computePositions = function (
     }
     forward = Cartesian3.subtract(nextPosition, position, forward);
     forward = Cartesian3.normalize(forward, forward);
-    cornerDirection = Cartesian3.add(forward, backward, cornerDirection);
-    cornerDirection = Cartesian3.normalize(cornerDirection, cornerDirection);
     surfaceNormal = ellipsoid.geodeticSurfaceNormal(position, surfaceNormal);
 
     const forwardProjection = Cartesian3.multiplyByScalar(
@@ -461,6 +459,8 @@ PolylineVolumeGeometryLibrary.computePositions = function (
     );
 
     if (doCorner) {
+      cornerDirection = Cartesian3.add(forward, backward, cornerDirection);
+      cornerDirection = Cartesian3.normalize(cornerDirection, cornerDirection);
       cornerDirection = Cartesian3.cross(
         cornerDirection,
         surfaceNormal,

diff --git a/packages/engine/Specs/Core/CorridorGeometryLibrarySpec.js b/packages/engine/Specs/Core/CorridorGeometryLibrarySpec.js
@@ -0,0 +1,38 @@
+import { CorridorGeometryLibrary, Ellipsoid } from "../../index.js";
+
+describe("Core/CorridorGeometryLibrary", () => {
+  describe("computePositions", () => {
+    it("should not compute corners for collinear points", () => {
+      const options = {
+        positions: [
+          { x: 1, y: 1, z: 1 },
+          { x: 2, y: 2, z: 2 },
+          { x: 3, y: 3, z: 3 },
+          { x: 4, y: 4, z: 4 },
+        ],
+        width: 0.15,
+        ellipsoid: Ellipsoid.WGS84,
+      };
+
+      // The fact it doesn't throw an error also verifies the fix #12255
+      const result = CorridorGeometryLibrary.computePositions(options);
+      expect(result.corners.length).toEqual(0);
+    });
+
+    it("should compute corners for non-collinear points", () => {
+      const options = {
+        positions: [
+          { x: 0, y: 0, z: 1 },
+          { x: 1, y: 0, z: 2 },
+          { x: 1, y: 1, z: 3 },
+          { x: 0, y: 1, z: 4 },
+        ],
+        width: 0.15,
+        ellipsoid: Ellipsoid.WGS84,
+      };
+
+      const result = CorridorGeometryLibrary.computePositions(options);
+      expect(result.corners.length).toEqual(2);
+    });
+  });
+});
diff --git a/packages/engine/Specs/Core/PolylineVolumeGeometryLibrarySpec.js b/packages/engine/Specs/Core/PolylineVolumeGeometryLibrarySpec.js
@@ -0,0 +1,56 @@
+import {
+  Cartesian3,
+  Cartesian2,
+  BoundingRectangle,
+  PolylineVolumeGeometry,
+  Ellipsoid,
+  PolylineVolumeGeometryLibrary,
+} from "../../index.js";
+
+describe("Core/PolylineVolumeGeometryLibrary", () => {
+  describe("computePositions", () => {
+    // Tests the fix #12255
+    it("should not throw error if positions are collinear after scaling to geodetic surface", () => {
+      const positions = [
+        new Cartesian3(1, 1, 1),
+        new Cartesian3(2, 2, 2),
+        new Cartesian3(3, 3, 3),
+        new Cartesian3(4, 4, 4),
+      ];
+
+      const shape = [
+        new Cartesian2(-0.15, -0.15),
+        new Cartesian2(0.15, -0.15),
+        new Cartesian2(0.15, 0.15),
+        new Cartesian2(-0.15, 0.15),
+      ];
+
+      const ellipsoidStub = new Ellipsoid(
+        6378137.0,
+        6378137.0,
+        6356752.3142451793,
+      );
+      // It's easier to stub the function than to predict the values to be collinear after calling the function
+      ellipsoidStub.scaleToGeodeticSurface = function (cartesian, result) {
+        return Cartesian3.clone(cartesian, result);
+      };
+
+      const boundingRectangle = new BoundingRectangle(-0.15, -0.15, 0.3, 0.3);
+      const geometry = new PolylineVolumeGeometry({
+        polylinePositions: positions,
+        shapePositions: shape,
+      });
+      geometry._ellipsoid = ellipsoidStub;
+
+      expect(() =>
+        PolylineVolumeGeometryLibrary.computePositions(
+          positions,
+          shape,
+          boundingRectangle,
+          geometry,
+          true,
+        ),
+      ).not.toThrowDeveloperError();
+    });
+  });
+});