EllipsoidTerrainProvider does not conform to the TerrainProvider interface

[angrycat9000]

What happened?

TerrainProvider has availablity defined as TileAvailablity.

cesium/packages/engine/Source/Core/TerrainProvider.js

Line 84 in 82e59ed

* @type {TileAvailability}

However EllipsoidTerrainProvider returns undefined for availabilty. This does not satisfy TileAvailabity

cesium/packages/engine/Source/Core/EllipsoidTerrainProvider.js

Line 123 in 82e59ed

return undefined;

Found this because sampleHeightsMostDetailed takes a TerrainProvider but throws an error if EllipsoidTerrainProvider is passed.

In the app I was working with the terrain provider may change based on user input. So there isn't a good way to have a single code path that gets the most detailed height. Had to put in a check to to see if the current terrain provider was in instance of EllipsoidTerrainProvider before calling sampleHeightsMostDetailed

Reproduction steps

No response

Sandcastle example

No response

Environment

No response

[ggetz]

@angrycat9000 I think this is expected, as sampleTerrainMostDetailed does explicitly check for this condition.

If we can agree on that, then the fix here is that @type {TileAvailability} should be @type {TileAvailability|undefined} .

[angrycat9000]

@angrycat9000 I think this is expected, as sampleTerrainMostDetailed does explicitly check for this condition.

For consumers who know the implementation details. For a consumer just reading the documentation, passing EllipsoidTerrainProvider to sampleTerrainMostDetailed appears perfectly valid.

If we can agree on that, then the fix here is that type {TileAvailability} should be type {TileAvailability|undefined} .

That still leaves the case where sampleTerrainMostDetailed takes a TerrainProvider but only some implementations of that interface.

I would like to see the type interfaces / documentation to accurately reflect the implementation logic so there are fewer special situations where the consumer has to read the implementation to use the function. That makes it easier for consumers.

There could be a number of ways to acheive this. Haven't dug that deep into the code to know which is the best effort to value ratio.

• EllipsoidTerrainProvider has an availabilty that always returns true for isTileAvailable
• sampleTerrainHeightMostDetailed has a special case that for EllipsoidTerrainProvider that just returns height = 0
  • Does it even need that if it just provides an availabilty?
• Have a class hiearchy ofTerrainProvider -> TileBasedTerrainProvider and sampleTerrainMostDetailed only takes a TileBasedTerrainProvider to eliminate the possibilty of EllipsoidTerrainProvider not being passed to sampleTerrainMostDetailed
• Update the documentation to indicate that sampleTerrainMostDetailed will throw for EllipsoidTerrainProvider

[ggetz]

@angrycat9000 To confirm, we are talking about sampleTerrainMostDetailed here, not Scene.sampleHeightMostDetailed correct?

Scene.sampleHeightMostDetailed does not take any TerrainProvider arguments, nor would it need to based on how its implemented.

[angrycat9000]

@angrycat9000 To confirm, we are talking about sampleTerrainMostDetailed here, not Scene.sampleHeightMostDetailed correct?

Yes, sorry for the confusion. Edited to clarify

[javagl]

This is related to #11749 (comment) : There are places where the documentation claims a certain type, but erroneously does not mention that something can also be undefined (be it via @default values, or, as it is the case here, via implementations of getters).

(One could hope that something like that could be detected, after efforts like #8359 or #10455. Technically, detecting that inconsistency should be easy)

Without knowing all the details of the context (I probably should, in the meantime, but ... it's a lot), I cannot have a preference about how to handle this, but

1. The documenation already says "This property may be undefined if availability information is not available.", meaning that the @type has to be updated to {TileAvailability|undefined} anyhow (or all implementations have to be checked and that statement has to be removed)
2. Even when this is changed, the EllipsoidTerrainProvider could return some TileAvailability that always returns true. After all, it provides "artificial" tiles that always are available (assuming that level is not negative, and other santity conditions). This could be a nice form of the Null object pattern. But... ... TileAvailability is not an interface, and it could be really tricky to make sure that the behavior of "always returning true" is deeply and thoroughly consistent with functions like computeBestAvailableLevelOverRectangle (whatever that does - from quickly skimming over that, it might have to return something like Infinity, which would then be a very undesirable special case again 😬 )