Skip to content

Safeguards project from undefined cartographic value #12671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+132 −52
Open

Safeguards project from undefined cartographic value #12671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
95ec0fe
Safeguards project from undefined cartographic value
mzschwartz5 Jun 16, 2025
78be243
Creates safeProject method to handle undefined cartographics
mzschwartz5 Jun 17, 2025
8cc5c04
Revert "Creates safeProject method to handle undefined cartographics"
mzschwartz5 Jun 20, 2025
468c1a1
Check for undefined args in project
mzschwartz5 Jul 8, 2025
4b478d0
Removes misused scratch variables
mzschwartz5 Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions packages/engine/Source/Core/EllipseGeometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,13 @@ function computeTopBottomAttributes(positions, options, extrude) {
let bitangent = scratchBitangent;

const projection = new GeographicProjection(ellipsoid);
const projectedCenter = projection.project(
ellipsoid.cartesianToCartographic(center, scratchCartographic),
projectedCenterScratch,
const cartographic = ellipsoid.cartesianToCartographic(
center,
scratchCartographic,
);
const projectedCenter = defined(cartographic)
? projection.project(cartographic, projectedCenterScratch)
: Cartesian3.clone(Cartesian3.ZERO, projectedCenterScratch);

const geodeticNormal = ellipsoid.scaleToGeodeticSurface(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computing the surface normal can run into division by zero errors when the position in question is the origin.

I'd suggest restructuring the flow, both here and in those other similar cases in this file, to use a function something like the following (it also removes some redundant work):

function getProjectedPositionAndNormal(position, result = []) {
  if (Cartesian3.equalsEpsilon(position, Cartesian3.ZERO, CesiumMath.EPSILON14)) {
    result[0] = Cartesian3.clone(position, result[0]);
    result[1] =  Cartesian3.clone(Cartesian3.UNIT_Z, result[1]);
    return result;
  }
  
  const cartographic = ellipsoid.cartesianToCartographic(
    position,
    scratchCartographic,
  );
  
  result[0] = projection.project(cartographic, result[0]);
  result[1] = ellipsoid.geodeticSurfaceNormalCartographic(cartographic, result[1]);
  return result;
}

const [projectedCenter, geodeticNormal] = getProjectedPositionAndNormal(center);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be helpful at this point to add test cases where the input is the origin to the unit tests for the fixes throughout this PR, assuming they're straightforward to create.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point... in fact, it's made me questions my overall approach. As I took a look through the other changed files for instances where we might want a getProjectedPositionAndNormal function, I realized that it's pretty hard to tell what happens downstream in general.

What I mean is, in some cases (like this one), it's easy to see the value gets normalized immediately. In others, however, it's just a function that returns a value, and who knows how it gets consumed. I did some digging but it would be a non-trivial effort to trace all code paths for every file I changed here (some of which are very core, like Primitive).

Maybe this PR should be more focused towards the case that actually causes the bug reported? Taking an even bigger step back - is there ever even a use case for spawning things at (0, 0, 0)? If we really want to support it, maybe we can just jitter objects spawned at the origin by some epsilon?

center,
Expand Down Expand Up @@ -132,10 +135,13 @@ function computeTopBottomAttributes(positions, options, extrude) {
position,
scratchCartesian2,
);
const projectedPoint = projection.project(
ellipsoid.cartesianToCartographic(rotatedPoint, scratchCartographic),
scratchCartesian3,
const cartographic = ellipsoid.cartesianToCartographic(
rotatedPoint,
scratchCartographic,
);
const projectedPoint = defined(cartographic)
? projection.project(cartographic, scratchCartesian3)
: Cartesian3.clone(Cartesian3.ZERO, scratchCartesian3);
Cartesian3.subtract(projectedPoint, projectedCenter, projectedPoint);

texCoordScratch.x =
Expand Down Expand Up @@ -482,10 +488,13 @@ function computeWallAttributes(positions, options) {
let bitangent = scratchBitangent;

const projection = new GeographicProjection(ellipsoid);
const projectedCenter = projection.project(
ellipsoid.cartesianToCartographic(center, scratchCartographic),
projectedCenterScratch,
const cartographic = ellipsoid.cartesianToCartographic(
center,
scratchCartographic,
);
const projectedCenter = defined(cartographic)
? projection.project(cartographic, projectedCenterScratch)
: Cartesian3.clone(Cartesian3.ZERO, projectedCenterScratch);

const geodeticNormal = ellipsoid.scaleToGeodeticSurface(
center,
Expand Down Expand Up @@ -524,10 +533,13 @@ function computeWallAttributes(positions, options) {
position,
scratchCartesian2,
);
const projectedPoint = projection.project(
ellipsoid.cartesianToCartographic(rotatedPoint, scratchCartographic),
scratchCartesian3,
const cartographic = ellipsoid.cartesianToCartographic(
rotatedPoint,
scratchCartographic,
);
const projectedPoint = defined(cartographic)
? projection.project(cartographic, scratchCartesian3)
: Cartesian3.clone(Cartesian3.ZERO, scratchCartesian3);
Cartesian3.subtract(projectedPoint, projectedCenter, projectedPoint);

texCoordScratch.x =
Expand Down
5 changes: 5 additions & 0 deletions packages/engine/Source/Core/GeographicProjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Cartographic from "./Cartographic.js";
import defined from "./defined.js";
import DeveloperError from "./DeveloperError.js";
import Ellipsoid from "./Ellipsoid.js";
import Check from "./Check.js";

/**
* A simple map projection where longitude and latitude are linearly mapped to X and Y by multiplying
Expand Down Expand Up @@ -52,6 +53,10 @@ Object.defineProperties(GeographicProjection.prototype, {
* created and returned.
*/
GeographicProjection.prototype.project = function (cartographic, result) {
//>>includeStart('debug', pragmas.debug);
Check.defined("cartographic", cartographic);
//>>includeEnd('debug');

// Actually this is the special case of equidistant cylindrical called the plate carree
const semimajorAxis = this._semimajorAxis;
const x = cartographic.longitude * semimajorAxis;
Expand Down
10 changes: 5 additions & 5 deletions packages/engine/Source/Core/GroundPolylineGeometry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -872,11 +872,11 @@ function projectNormal(
);
}

normalEndpointCartographic.height = 0.0;
const normalEndpointProjected = projection.project(
normalEndpointCartographic,
result,
);
const normalEndpointProjected = defined(normalEndpointCartographic)
? projection.project(normalEndpointCartographic, result)
: Cartesian3.clone(Cartesian3.ZERO, result);
normalEndpointProjected.height = 0.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to use a ternary statement (consistent with other changes in this PR), I changed the "set height to 0" statement to occur after the projection step. This order change should not make a functional difference since projection does not depend on height, and simply sets the result height to the input height.

Open to push back here if this seems unnecessarily risky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fine, but let's make sure we're not normalizing (0, 0, 0) right below this.


result = Cartesian3.subtract(
normalEndpointProjected,
projectedPosition,
Expand Down
16 changes: 8 additions & 8 deletions packages/engine/Source/Core/Transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,10 +1189,10 @@ Transforms.basisTo2D = function (projection, matrix, result) {
scratchCartographic,
);

projectedPosition = projection.project(
cartographic,
scratchCartesian3Projection,
);
projectedPosition = defined(cartographic)
? projection.project(cartographic, scratchCartesian3Projection)
: Cartesian3.clone(Cartesian3.ZERO, scratchCartesian3Projection);

Cartesian3.fromElements(
projectedPosition.z,
projectedPosition.x,
Expand Down Expand Up @@ -1245,10 +1245,10 @@ Transforms.ellipsoidTo2DModelMatrix = function (projection, center, result) {
center,
scratchCartographic,
);
const projectedPosition = projection.project(
cartographic,
scratchCartesian3Projection,
);
const projectedPosition = defined(cartographic)
? projection.project(cartographic, scratchCartesian3Projection)
: Cartesian3.clone(Cartesian3.ZERO, scratchCartesian3Projection);

Cartesian3.fromElements(
projectedPosition.z,
projectedPosition.x,
Expand Down
5 changes: 5 additions & 0 deletions packages/engine/Source/Core/WebMercatorProjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import defined from "./defined.js";
import DeveloperError from "./DeveloperError.js";
import Ellipsoid from "./Ellipsoid.js";
import CesiumMath from "./Math.js";
import Check from "./Check.js";

/**
* The map projection used by Google Maps, Bing Maps, and most of ArcGIS Online, EPSG:3857. This
Expand Down Expand Up @@ -98,6 +99,10 @@ WebMercatorProjection.MaximumLatitude =
* @returns {Cartesian3} The equivalent web mercator X, Y, Z coordinates, in meters.
*/
WebMercatorProjection.prototype.project = function (cartographic, result) {
//>>includeStart('debug', pragmas.debug);
Check.defined("cartographic", cartographic);
//>>includeEnd('debug');

const semimajorAxis = this._semimajorAxis;
const x = cartographic.longitude * semimajorAxis;
const y =
Expand Down
5 changes: 4 additions & 1 deletion packages/engine/Source/Renderer/UniformState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1344,7 +1344,10 @@ function setSunAndMoonDirections(uniformState, frameState) {
uniformState._sunPositionWC,
sunCartographicScratch,
);
projection.project(sunCartographic, uniformState._sunPositionColumbusView);

uniformState._sunPositionColumbusView = defined(sunCartographic)
? projection.project(sunCartographic, uniformState._sunPositionColumbusView)
: Cartesian3.clone(Cartesian3.ZERO, uniformState._sunPositionColumbusView);
}

/**
Expand Down
26 changes: 18 additions & 8 deletions packages/engine/Source/Scene/Camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,10 +562,10 @@ function convertTransformFor2D(camera) {
scratchCartographic,
);

const projectedPosition = projection.project(
cartographic,
scratchCartesian3Projection,
);
let projectedPosition = defined(cartographic)
? projection.project(cartographic, scratchCartesian3Projection)
: Cartesian3.clone(Cartesian3.ZERO, scratchCartesian3Projection);

const newOrigin = scratchCartesian4NewOrigin;
newOrigin.x = projectedPosition.z;
newOrigin.y = projectedPosition.x;
Expand All @@ -584,7 +584,10 @@ function convertTransformFor2D(camera) {
);
ellipsoid.cartesianToCartographic(xAxis, cartographic);

projection.project(cartographic, projectedPosition);
projectedPosition = defined(cartographic)
? projection.project(cartographic, projectedPosition)
: Cartesian3.clone(Cartesian3.ZERO, projectedPosition);

const newXAxis = scratchCartesian4NewXAxis;
newXAxis.x = projectedPosition.z;
newXAxis.y = projectedPosition.x;
Expand All @@ -605,7 +608,10 @@ function convertTransformFor2D(camera) {
);
ellipsoid.cartesianToCartographic(yAxis, cartographic);

projection.project(cartographic, projectedPosition);
projectedPosition = defined(cartographic)
? projection.project(cartographic, projectedPosition)
: Cartesian3.clone(Cartesian3.ZERO, projectedPosition);

newYAxis.x = projectedPosition.z;
newYAxis.y = projectedPosition.x;
newYAxis.z = projectedPosition.y;
Expand Down Expand Up @@ -1313,7 +1319,9 @@ function setViewCV(camera, position, hpr, convert) {
position,
scratchSetViewCartographic,
);
position = projection.project(cartographic, scratchSetViewCartesian);
position = defined(cartographic)
? projection.project(cartographic, scratchSetViewCartesian)
: Cartesian3.clone(Cartesian3.ZERO, scratchSetViewCartesian);
}
Cartesian3.clone(position, camera.position);
}
Expand Down Expand Up @@ -1348,7 +1356,9 @@ function setView2D(camera, position, hpr, convert) {
position,
scratchSetViewCartographic,
);
position = projection.project(cartographic, scratchSetViewCartesian);
position = defined(cartographic)
? projection.project(cartographic, scratchSetViewCartesian)
: Cartesian3.clone(Cartesian3.ZERO, scratchSetViewCartesian);
}

Cartesian2.clone(position, camera.position);
Expand Down
5 changes: 4 additions & 1 deletion packages/engine/Source/Scene/GlobeSurfaceTile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ function getPosition(encoding, mode, projection, vertices, index, result) {
position,
scratchCartographic,
);
position = projection.project(positionCartographic, result);
position = defined(positionCartographic)
? projection.project(positionCartographic, result)
: Cartesian3.clone(Cartesian3.ZERO, result);

position = Cartesian3.fromElements(
position.z,
position.x,
Expand Down
10 changes: 7 additions & 3 deletions packages/engine/Source/Scene/PolylineCollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1778,10 +1778,14 @@ PolylineBucket.prototype.getSegments = function (polyline, projection) {
for (let n = 0; n < length; ++n) {
position = positions[n];
p = Matrix4.multiplyByPoint(modelMatrix, position, p);
const cartographic = ellipsoid.cartesianToCartographic(
p,
scratchCartographic,
);
newPositions.push(
projection.project(
ellipsoid.cartesianToCartographic(p, scratchCartographic),
),
defined(cartographic)
? projection.project(cartographic)
: Cartesian3.clone(Cartesian3.ZERO),
);
}

Expand Down
24 changes: 12 additions & 12 deletions packages/engine/Source/Scene/Primitive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1521,10 +1521,11 @@ function updateBatchTableBoundingSpheres(primitive, frameState) {
center,
scratchBoundingSphereCartographic,
);
const center2D = projection.project(
cartographic,
scratchBoundingSphereCenter2D,
);
const center2D = defined(cartographic)
? projection.project(cartographic, scratchBoundingSphereCenter2D)
: Cartesian3.clone(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentionally cloning a clone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, thought I caught that but seems I forgot to commit it.

Cartesian3.clone(Cartesian3.ZERO, scratchBoundingSphereCenter2D),
);
encodedCenter = EncodedCartesian3.fromCartesian(
center2D,
scratchBoundingSphereCenterEncoded,
Expand Down Expand Up @@ -1589,18 +1590,17 @@ function updateBatchTableOffsets(primitive, frameState) {
center,
scratchBoundingSphereCartographic,
);
const center2D = projection.project(
cartographic,
scratchBoundingSphereCenter2D,
);

const center2D = defined(cartographic)
? projection.project(cartographic, scratchBoundingSphereCenter2D)
: Cartesian3.clone(Cartesian3.ZERO, scratchBoundingSphereCenter2D);

const newPoint = Cartesian3.add(offset, center, offsetScratchCartesian);
cartographic = ellipsoid.cartesianToCartographic(newPoint, cartographic);

const newPointProjected = projection.project(
cartographic,
offsetScratchCartesian,
);
const newPointProjected = defined(cartographic)
? projection.project(cartographic, offsetScratchCartesian)
: Cartesian3.clone(Cartesian3.ZERO, scratchBoundingSphereCenter2D);

const newVector = Cartesian3.subtract(
newPointProjected,
Expand Down
8 changes: 6 additions & 2 deletions packages/engine/Source/Scene/SceneTransitioner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,14 @@ SceneTransitioner.prototype.morphToColumbusView = function (
);
Matrix4.inverseTransformation(toENU, toENU);

scene.mapProjection.project(
ellipsoid.cartesianToCartographic(position, scratchToCVCartographic),
const cartographic = ellipsoid.cartesianToCartographic(
position,
scratchToCVCartographic,
);
position = defined(cartographic)
? scene.mapProjection.project(cartographic, position)
: Cartesian3.clone(Cartesian3.ZERO, position);

Matrix4.multiplyByPointAsVector(toENU, direction, direction);
Matrix4.multiplyByPointAsVector(toENU, up, up);
}
Expand Down
34 changes: 34 additions & 0 deletions packages/engine/Specs/Scene/PrimitiveSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,40 @@ describe(
primitive.destroy();
expect(primitive.isDestroyed()).toEqual(true);
});

Copy link
Contributor Author

@mzschwartz5 mzschwartz5 Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only authored this one test for now, so I can get feedback before potentially going in the wrong direction.

Questions:

  1. Is this an appropriate way to test the changed behavior? I.e. should I add similar coverage for the other files I changed?
  2. Is there something I should be checking with expects that I'm not? (E.g. maybe something to do with the batch table code? Though that seems like internal implementation, the sort of thing that isn't usually covered by unit tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also, forgot to remove this line:

expect(frameState.commandList.length).toEqual(1); which is causing failures. I'll address that with the next push.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a solid approach to test the issue you are addressing, locating primitives at the origin.

As to whether you need to add unit tests for the other areas you added the fix, I think the initial questions to ask are 1) do existing unit tests break and (if yes then fix) 2) without the fix are we able to reproduce the error by placing something at the origin. If yes to 2 I think a unit test makes sense.

it("does not throw an error when rendering a primitive at the origin", function () {
const modelMat = Matrix4.fromTranslation(Cartesian3.ZERO);

const boxInstance = new GeometryInstance({
geometry: BoxGeometry.fromDimensions({
vertexFormat: PerInstanceColorAppearance.VERTEX_FORMAT,
dimensions: new Cartesian3(500000.0, 500000.0, 500000.0),
}),
id: "box",
attributes: {
color: new ColorGeometryInstanceAttribute(1.0, 1.0, 0.0, 0.5),
// This triggers batching to run, which is the code path where an origin-centered object needs special treatment.
distanceDisplayCondition:
new DistanceDisplayConditionGeometryInstanceAttribute(
0,
10000000.0,
),
},
modelMatrix: modelMat,
});

primitive = new Primitive({
geometryInstances: boxInstance,
appearance: new PerInstanceColorAppearance({
closed: true,
}),
asynchronous: false,
});

expect(function () {
primitive.update(frameState);
}).not.toThrow();
});
},
"WebGL",
);