Safeguards project from undefined cartographic value

[mzschwartz5]

Description

#12150 discovered an issue with rendering objects (primitives, specifically) at the origin. The root cause comes from a cartesian -> cartographic conversion, where a cartesian value of (0,0,0) is undefined in cartographic coordinates. Many locations in code use this conversion function without checking for an undefined return value.

This PR addresses a class of cases that consumed this return value without any safeguards: namely, the case where a projection function is called on undefined cartographic coordinates. In these cases, we can implement an easy workaround of conditionally projecting, if the cart coords are defined, and simply assigning a projected value of (0, 0, 0) when they are not.

This PR does NOT address every site that calls cartesianToCartographic, only those which immediately feed the results into a projection. For a more complete list of vulnerable calls, see the list attached here.

Issue number and link

#12150

Testing plan

Used the sandbox linked in the original issue to test the fix; primitives can now be spawned at the origin.

TODO: more unit test coverage? pending feedback from reviewer.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @mzschwartz5!

✅ We can confirm we have a CLA on file for you.

[mzschwartz5]

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.

[ggetz]

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

[mzschwartz5]

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)

[mzschwartz5]

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.

[lukemckinstry]

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.

[javagl]

The way how exactly the undefined case is handled here may still cause problems later.

Disclaimer: I don't know whether this is relevant here! Figuring that out would require to read each affected part of the code veeery thoroughly, with lots of full text searches and such...

But I'll just mention it for now:

• Before this fix, when cartesianToCartographic returned undefined and this was passed to a projection.project call, it did crash.
• After this fix, the projection.project will no longer be called with undefined, preventing the crash. Instead, the constant value Cartesian3.ZERO will be used.

But ... this really is constant, as in immutable. Consider the following:

const scratchCartographic = new Cesium.Cartographic();
const projectedCenterScratch = new Cesium.Cartesian3();
const ellipsoid = Cesium.Ellipsoid.WGS84;
const projection = new Cesium.GeographicProjection(ellipsoid);

// Old implementation, without undefined check
function computeOld(center) {
  const projectedCenter = projection.project(
    ellipsoid.cartesianToCartographic(center, scratchCartographic),
    projectedCenterScratch,
  );  
  return projectedCenter;
}

// New implementation, with undefined check
function computeNew(center) {
  const cartographic = ellipsoid.cartesianToCartographic(
    center,
    scratchCartographic,
  );
  const projectedCenter = Cesium.defined(cartographic)
    ? projection.project(cartographic, projectedCenterScratch)
    : Cesium.Cartesian3.ZERO;
  return projectedCenter;
}

const centerA = new Cesium.Cartesian3(0,0,0);

// Crashed before
//const resultOld = computeOld(centerA);
//console.log(resultOld);

// Does not crash now
const resultNew = computeNew(centerA);
console.log(resultNew);

// Crashes now
resultNew.x = 1.2;

If someone stored the result of that (attempted) project call, and later tried to modify it, then it will now crash, because that Cartesian3 object really is the immutable ZERO object.

I think that in the places that are affected here (from quickly skimming over the code!), it should be possible to resolve that in most cases, by changing the pattern of

  const projectedCenter = Cesium.defined(cartographic)
    ? projection.project(cartographic, projectedCenterScratch)
    : Cesium.Cartesian3.ZERO;

to

  const projectedCenter = Cesium.defined(cartographic)
    ? projection.project(cartographic, projectedCenterScratch)
    : Cartesian3.clone(Cesium.Cartesian3.ZERO, projectedCenterScratch);

This will - in this example - have the effect that the projectedCenter is always the projectedCenterScratch, no matter whether the ? or the : path was taken.

(That may turn out to be a bit repetitive, and at some point, I'd consider some projectOptional(mayBeUndefined...) utility function, but that's likely not in the scope of this PR)

[mzschwartz5]

@javagl Good feedback, thanks. I like your approach better than what I did; less redundant and it promotes better unit test coverage by centralizing the changes. Next commit will retry things this way.

[javagl]

I'm not sure what the "approach" referred to, but to avoid misunderstandings: When I mentioned something like projectOptional then this was not a proposal/suggestion to introduce such a method. (Adding new methods to interfaces is a big thing...).

[javagl]

I probably was to slow here 😬 ... let's see what Gabby says...

[mzschwartz5]

😅 Well, we can always roll back to the previous commit + replace the usage of Cartesian3.ZERO with cloning. Though Gabby's comment on the issue does suggest a wrapper method, which safeProject (what I just introduced) is, pretty much.

I can see how modifying an interface is a big deal (breaking change), but is adding to an interface so bad? While it does clutter the interface, it's at least not as intrusive/breaking as returning a special value from cartesianToCartographic (instead of undefined) would have been.

[javagl]

Adding a new abstract function to an existing interface is a breaking change ... for implementors. If someone has implemented some MyVerySpecialProjection that implements MapProjection, then this will break. I think that this is very unlikely, but ... that's just a gut feeling. And trivial to work around. And... in this case... it may not even be necessary: I think that the implementation of this new function is currently identical in GeographicProjection and WebMercatorProjection, and one could make the case that this function definition should be in MapProjection (as a non-abstract function).

But before "just doing that", it might make sense to wait for feedback from Gabby, about whether or not such a function should actually exist.

(I don't want to further hold up what could have been a simple fix if I hadn't chimed in)

[ggetz]

So I agree with both of the following:

1. Returning new values or throwing new errors from an existing functionality would be a breaking change we want to avoid
2. Adding a new function to the public Projection interface, and using it unconditionally in other parts of the API as it is here in Transforms, would require any other implementors of Projection to implement this new function or potentially break. So adding a new function here is also be a breaking change, albeit one that would likely impact fewer consumers.

If we want to keep the same approach as safeProjection uses, I think we could go ahead and apply it directly to the existing projection functions. Since the change here would be to allow for an additional parameter type, undefined in addition to cartographic, no consumers should be depending on that behavior. And nothing is changing about how we're using the project function– We're just fixing the bug.

One could argue that it's still a breaking change since it changes the interface. We can note that this is a breaking change in CHANGES.md. Or, we could introduce a new function to replace projection and deprecate the existing function over a few release versions.

What are your thoughts?

[javagl]

Not sure who that question was aimed at, but regardless of that, the clarification may be useful:

What you proposed is to not introduce a new function, and not check the cartographic before passing it in, but just allow undefined as the first parameter for the project function, and return a clone(ZERO) in this case, ist that correct?

If this was the intention:

Yes, this would not really be a "breaking change". (Technically, it would be as in expect(callWithUndefined()).toThrow() would fail, but let's focus on the actual usage). It could be worthwhile to think through whether this is the intended behavior, though. While the "Rendering has stopped"-category of errors are the "worst case", they at least tell you what went wrong. Silently ignoring invalid inputs and defaulting to "arbitrary" values (like (0,0)) can lead to errors like this #5726 (comment) (which in fact, may or may not be an actual instance of this problem), which are usually faaar harder to debug: Where do these (0,0)'s come from?

No strong opinion for now, just something to keep in mind.

[mzschwartz5]

I like Gabby's idea, from a standpoint of keeping changes minimal and non-breaking (for all intents and purposes). I see what @javagl's point though - can we perhaps log a (dev) warning (i.e. gets stripped out via debug pragma) in the case of an undefined first arg?

In my mind, that's a best-of-both worlds approach; avoid a complete crash, but don't be completely silent about it.

edit- after more thought, if we go ^ this route, we should still address our own instances of potentially passing undefined into project, otherwise we'll be logging unavoidable warnings whenever someone instantiates an object at the origin.

[ggetz]

can we perhaps log a (dev) warning (i.e. gets stripped out via debug pragma) in the case of an undefined first arg?

This would actually be functionally very similar to an "official" deprecation period, during which we'd log a developer error when the parameter is undefined. After that, we could strictly require the value, and throw an error with a more helpful message when it is not defined. (Arguably, we could go with a RuntimeError over a stripped out DeveloperError as it's common for this function to be called lower down in the stack and could change based on the data loaded at runtime.)

after more thought, if we go ^ this route, we should still address our own instances of potentially passing undefined into project, otherwise we'll be logging unavoidable warnings whenever someone instantiates an object at the origin.

Yes, definitely!

[ggetz]

To clarify, what I'm suggesting in my last comment is that we could:

1. Start by choose a deprecation period (i.e. 3-5 releases or so). We'd log a deprecation warning when an undefined value is passed, with a message that this will throw an error in the future version. We would update all of our internal APIs to avoid passing undefined and logging the deprecation warning during normal operation.
2. After the deprecation period ends, we remove the deprecation warning an start throwing an actual Error.

The error could be either a DeveloperError that will be stripped out in production builds (which is typically how we check for undefined values) or a RuntimeError that would include a more descriptive error than the error that arises now.

This is not deprecating the function, but it is deprecating existing behavior.

[mzschwartz5]

Wait, technically an error is already thrown when you pass an undefined cartographic argument into project - because we try to read properties of cartographic, so a "Cannot read properties of undefined..." will be thrown.

So I don't think we need to log a deprecation warning because no one is depending on the behavior of being able to pass in undefined here. We can just throw a RuntimeError with something more descriptive, and update our internal calls to check for undefined first.

The only real question is whether is should be a RuntimeError or a DeveloperError. I personally think DeveloperError is more appropriate here as passing in undefined is something that should be caught during development and testing.

[ggetz]

Thanks for the update here @mzschwartz5! I have a few more comments based on the in-context fixes.

[ggetz]

Is this intentionally cloning a clone?

[mzschwartz5]

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

[ggetz]

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

[ggetz]

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.

[mzschwartz5]

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?

[ggetz]

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