Rendering error for entity at origin

[javagl]

When creating an entity at (0,0,0), it crashes with

TypeError: Cannot read properties of undefined (reading 'longitude')

Sandcastle:

https://sandcastle.cesium.com/index.html#c=dY/BSsQwEIZfZeiphZK6eFu7RehV8CB46iVtRh02nZRM2nWVfXeT9rLoCnPI/Pm+n2RwLAEWwhN6OADjCVoUmkf1umZ5lw3r3joOmhh9lxUPHW+GQg4UCEVpY/LvjgEmJzFxvL/uarUP8aT5Pr8rIU1RJrh3n3tYNQBDI7JEU/5TdyWkKRJ/if4lPSQrs1rC2WKz1QA80jg5H2D2NleqCjhOVseOqp+HIwY1iCQxoXV1rdaGFiBzuPFlGKwWiTdvs7Uv9IVd1tRV5P+o1mlD/P68oLf6nLCPXfO0hUqpuorrbTM4Z3vtfzX/AA

It's pretty simple:

const viewer = new Cesium.Viewer("cesiumContainer");
viewer.entities.add({
  position: new Cesium.Cartesian3(0, 0, 0),
  box: {
    dimensions: new Cesium.Cartesian3(1, 1, 1)
  },
});

This is caused here: https://github.com/CesiumGS/cesium/blob/89d15af5d2c0338a59f91f3dfa666ad8478f694b/packages/engine/Source/Scene/Primitive.js#L1530

The cartesianToCartographic function is returning undefined when receiving the center.

(I'll just put it at (0, 0, 1e-15) for now...)

[ggetz]

Here's another Sandcastle which breaks with the same stack

[javagl]

Adding a

osmBuildingsTileset.modelMatrix = Cesium.Matrix4.fromTranslation(
  new Cesium.Cartesian3(0.001, 0.001, 0.001));

"fixes" that. (Not sure which smiley to insert here).

I think a viable approach for a first "triage" pass on this issue would be to search for all calls to cartesianToCartographic, see what is done with the result, and handle the undefined return case accordingly. In both cases described here so far, it was passed to the respective projection, and I could imagine that some of that could be solved somewhat pragmatically, with something like

if (!defined(cartographic)) {
    // Input was origin...
    cartesian.x = 0.0;    
    cartesian.y = 0.0;
} else { ... project ... }

[mzschwartz5]

@javagl Building on your approach, I think it would be nice to have Ellipsoid.cartesianToCartographic return a special value (something like Cartographic.GLOBE_CENTER = Object.freeze(new Cartographic(...negative values or something invalid?...)), instead of undefined, and check against that in GeographicProjection.project. In my opinion,undefined should be reserved for unset values, and by using a named special value, GeographicProjection.project makes sense to readers without needing comments or having to go find out what causes a Cartographic to be undefined.

Thoughts?

Side note- after a little testing, it seems that putting a model at the origin (rather than a box primitive) avoids this issue. I need to figure out what the code path is there and compare to see how it's side stepping the problem, so that we treat both code paths the same way.

[mzschwartz5]

Ah, to answer my side note above, it's due to Primitives being batched, while Models are not. To determine batch groups, the bounding spheres of Primitives are used, which requires a projection (where the error occurs). Since that doesn't occur for Models, no error occurs.

[ggetz]

Another possibility could be to throw a differentiated error in the case where a Cartographic does not exist.

Though, regardless, we'll need to be careful about introducing any breaking changes to the existing API. Throwing a new error or returning new values from existing functions would likely be breaking for those who are already relying on that function. Maybe consider a new function which wraps the existing function in the new behavior for special cases.

[javagl]

I agree that one should usually not use undefined/null as a "special value". There may be cases where it's hard, though. Sometimes, existing libraries and patterns already use that approach, meaning that it can be justified for the sake of consistency. In other cases, the question of what should be done instead can be tricky to answer.

Roughly, there are three options:

• A dedicated "special value", like the GLOBE_CENTER, is one option, and can make sense in many cases. But here, the height depends on the longitude and latitude, and there simply is no longitude and latitude for that point. (And we certainly don't want any NaN here...)

• Another option is Optional<Cartographic>, but that's more common in typed languages, and not so much in JavaScript. (Maybe it should be...?)

• The generic option is to throw an error...

(One could argue that developers will ignore that in the same way as they ignored the undefined result. And one could argue that it does not make much difference for the end-user whether the error message is "TypeError: Cannot read properties of undefined" or "Cannot compute a cartographic for origin". But for developers, it may matter: Being specific, with a helpful stack trace, and bailing out immediately can be important. If someone stores the result of cartesianToCartographic somewhere, the undefined can lead to crashes anywhere, and it's hard to figure out here it came from)

But as Gabby already said: The cartesianToCartographic function cannot be changed. That would be a breaking change with unpredictable side effects.

Quickly skimming over the ~100 appearances in the code, I have seen only one case where the return value was checked. In many cases, the return value is not even actively used, because it's just a call that receives the 'result' as the last parameter (which will simply not be filled when the call fails, but it will not cause crashes).

One could pragmatically fix this special case here, by inserting some if (defined(...)) else shrug(); near that place.

Otherwise, the task could become bigger, and turn into the triage that I mentioned above :

• Search for all appearances of cartesianToCartographic
• Check whether the return value is used (this may already rule out most of them)
• Check how the return value is used (if it's just some projection, there may be a sensible default value that can be used instead)
• Identify the places that cannot trivially be fixed

Not sure how much time to spend with that...

[mzschwartz5]

Ah right, overlooked that this call is part of the public API.

Maybe consider a new function which wraps the existing function in the new behavior for special cases.

Before considering this option, I think it makes sense to go further down the path @javagl describes, and see how broad the impact of an undefined result is here anyway. If it's just a couple trouble spots, probably makes the most sense to just fix those spots instead of increasing code complexity.

[mzschwartz5]

I looked through all the occurrences and limited it down to the ones that could directly have runtime crashes as a result of a (0, 0, 0) Cartesian value. Results file: 85 total across 35 files. There are other occurrences that only use the result param, and might be more resilient to crashes, but would require deeper investigation.

I think it would be a little dangerous to try to address all of these at once, as they occur in varying contexts. For now, I'm going to address the calls that feed the results into projections, and get things to a point where you can at least create primitives at the origin without a crash.

edit- there are also a number of references to scaleToGeodeticSurface (which is called by cartesianToCartographic) that do not handle a potential undefined result, as well.