Allow external tilesets in multiple contents

[javagl]

Addresses #11960

Description

It removes the check that previously did throw a RuntimeError with the message

"External tilesets are disallowed inside multiple contents"

when trying to load a tileset where an external tileset was found in the (multiple) contents array.

Detailed description

The relevant part of the "trace" that leads to this point is drafted here:

• It starts at Cesium3DTileset.requestContent for a certain tile
• This calls Cesium3DTile.requestContent. When the tile contained the contents array, this delegates to Cesium3DTile.requestMultipleContents
• This creates the Multiple3DTileContent on which it calls requestInnerContents. This calls requestInnerContent for each content, then calls createInnerContents which waits for the requests, and then calls createInnerContent for each response.
• Finally, Multiple3DTileContent.createInnerContent did throw the error when it found that the content type was EXTERNAL_TILESET.

The place that originally threw up can in fact handle the creation of external tileset content. By just commenting out the check, the issue was fixed, and it "worked": It just used the Cesium3DTileContentFactory to create the content., which then was just a Tileset3DTileContent.

Caveats....

For single-content external tilesets, an external tileset caused the tile.hasTilesetContent = true; to be set. And this hasTilesetContent is checked in various places.

In most places, it seems to be used in the meaning that there are some children to traverse. For example, when contentExpired turns true, it causes destroySubtree to be called. This should be fine.

In getPriorityReverseScreenSpaceError, it is taken into account in a non-obvious way. I don't see a reasonable way to understand the implications of this flag at this point, but hope that having it being true there is fine as well.

The Cesium3DTile#hasRenderableContent may be the most critical one. It was implemented as

  /**
   * Determines if the tile's content is renderable. <code>false</code> if the
   * tile has empty content or if it points to an external tileset or implicit content
   *
   * @memberof Cesium3DTile.prototype
   *
   * @type {boolean}
   * @readonly
   *
   * @private
   */
  hasRenderableContent: {
    get: function () {
      return (
        !this.hasEmptyContent &&
        !this.hasTilesetContent &&
        !this.hasImplicitContent
      );
    },
  },

A somewhat shallow way of putting it is: With multiple contents, the tile may have renderable content even though it has a tileset content!

So I tried to examine that more thoroughly. And the hasRenderableContent is checked in various places:

• It must be true for tile.contentAvailable to ever be true
• It must be true for tile.hasUnloadedRenderableContent to be true. The hasUnloadedRenderableContent is checked in various places. It has effects on loading, often in the context of expiration.
• It must be true for tile.unloadContent to do anything(!)
• It affects colors in applyDebugSettings
• It affects the traversal, for example, for the refinement, but also at many other places.

The effect of hasRenderableContent (and its dependent, hasUnloadedRenderableContent) are scattered and not obvious. They usually appear in private (aka undocumented) functions. I had to stop "zooming" into this at some point.

Trying to deal with the caveats...

The change here revolves around two assumptions:

• I assume that hasTilesetContent also has to be set to true when the external tileset is one of multiple contents.
• I assume that hasRenderableContent must usually be true, except for the case of 1. empty content, 2. single external, 3. single implicit

So made hasRenderableContent an explicit variable (not just a getter that returns the &&-result of three others). This has to be set to true or false, consistent with its previous behavior and the assumptions.

Previously, hasRenderableContent depended on hasEmptyContent, hasTilesetContent, and hasImplicitContent. What makes this more difficult are the comments in hasTilesetContent and hasImplicitContent that say
This is <code>false</code> until the tile's (implicit) content is loaded.
meaning that the return value of hasRenderableContent would change through the loading process. What's even more quirky: When nothing is loaded yet, then hasRenderableContent will return true by default. Ouch.

So what is done here:

• hasRenderableContent is a variable that starts as true except for empty content
• It is only set to false when encountering a single content that is an external tileset or implicit content

Issue number and link

#11960

Testing plan

I tried out the test case that was posted in the issue, and other tilesets that involve external tilesets. For (somewhat helpless) testing, I inserted a brutal this._cache.trim(); in Cesium3DTileset.postPassesUpdate, to basically "force" the unloading/reloading in each frame, and then played with moving the tileset out of the viewport and back in again, to ensure that it is properly reloaded each time.

Beyond that, the difficulty of really testing this is why I added so many "Details" above.

In terms of "specs": There are currently two distinct specs:

• "multiple contents": "raises tileFailed for external tileset inside multiple contents"
• "3DTILES_multiple_contents: "raises tileFailed for external tileset inside multiple contents (legacy)"

Theoretically, only the second one should keep throwing.

But in practice, nobody is ever going to use 3DTILES_multiple_contents, any more, and if someone uses it for external tilesets (i.e. not compliant to the spec), and this happens to work in CesiumJS, that could be OK.

(Otherwise, we'd have to carry along the information of whether something was created from 3DTILES_multiple_contents or 3D Tiles 1.1, just to throw an "unnecessary" error at the right place ...)

The specs are not updated yet. I have no idea why the tests are currently passing.
I'll have to investigate that. Hey. Passing tests being wrong. That's new 🙂
Update: It looks like they are failing now. That's good.

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, @javagl!

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

[ggetz]

@javagl Let me confirm my understanding about the state of this PR: This is ready for review on the approach, then you'll proceed with updating the specs accordingly. Correct?

@lilleyse Would you be able to take a pass on the approach here?

[javagl]

This is ready for review on the approach, then you'll proceed with updating the specs accordingly.

Yes, given the depth of possible implications, I think that it could be worthwhile to review the approach itself, with scrutiny, by someone who might come up with ideas of what might go wrong in areas where I couldn't zoom to the "bottom".

Updating the specs could then be simple: There are four failing specs right now. The ones that expected the error to be thrown could be changed to just not expect the error to be thrown. One talks about color - that could be a fluke. The one at https://github.com/CesiumGS/cesium/actions/runs/12836753145/job/35799051528?pr=12440#step:6:43 warrants another look. It is about refinement, which might be a side-effect of the changes here (although not immediately obvious).

[javagl]

The failing test was setting root.hasEmptyContent to false, to // mock content, as it says.

Specifically, this had the effect that this call to hasRenderableContent returned true.

So this "mocking" relied on the very intricate implementation of hasRenderableContent. I fixed this in cefb360 to set the hasRenderableContent (which now is an explicit boolean property) to true.

[javagl]

The spec that just said expected color also did set hasEmptyContent=false, and I fixed it the same way.

Now, the remaining failing ones are 🤞 only the ones where the expectation has to be changed from 'expecting an error' to 'expecting no error'.

Futher specs should be considered - like one that checks that this external tileset actually is traversed. But I'd address that when the general approach is confirmed.

[ggetz]

Bump @lilleyse, would you have time to take a pass on this?

[lilleyse]

@ggetz yes, should have a chance this week.

[javagl]

The specs update here had still been pending. Although it might not make its way into the next release, I wanted to wrap this up, in doubt so that it may be merged shortly after the release and undergo some sort of additional "implicit testing" before the next one.

The specs update consisted of these two steps:

The spec
"raises tileFailed for external tileset inside multiple contents (legacy)"
has been removed. While this extension did disallow external tilesets in multiple contents, it's probably not worth keeping this spec: It would be necessary to keep track of where the tileset came from, just to throw an (now unnecessary) error...

The spec
"raises tileFailed for external tileset inside multiple contents"
has been replaced with
"renders external tilesets in multiple contents"

This spec uses the tileset that was added in the issue. It renders the following (debug canvas):

and checks that 4 tiles are selected and visited (the root of the main tileset, and the three roots of the external tilesets).

[lilleyse]

Should hasRenderableContent be false if all contents are external tilesets?

Otherwise, the approach makes sense. I'm less worried about the case where there's a mix of renderable content and external tilesets. It's harder to reason about but also less likely to occur in practice (I think).

[javagl]

I think that this should make sense, and I considered it at one point. The reasons why I did not implement this initially:

• I naively started with setting hasRenderableContent to false at the beginning, because ... initially, the tile certainly does not have renderable content. But then, no traversal took place at all...
• The breadth of possible implications of doing this - for example, in unloadContent (but also all the other places mentioned above)
• The way how the "inner contents" of a Multiple3DTileContent are created

The latter refers to the fact that it generates some promises, waits for them, and eventually has an array of content objects without explicit information about their type. This means that there are two options for implementing this:

• Add some flag like isTilesetContent in the content itself (and afterwards, count how many of the inner contents have this flag set to true)
• Keep track of the number of tileset contents in the Multiple3DTileContent itself

For now, I took the second approach: There now is a _externalTilesetCount that counts how many of the contents have been external tilesets, and sets tile.hasRenderableContent to false if it was all of them.

One caveat: There might be cases where an inner content is cancelled, failed, or its loading is interrupted, and depending on where exactly that can happen, it's hard to say whether this counter reflects the truth. If you think that the more invasive, wide-ranging, but somehow "safer" approach of keeping track of an isTilesetContent in each content would be better, I'll change it accordingly.

(I also added a spec for this case, based on the existing one, by just wrapping the red cube into another tileset)

[lilleyse]

I prefer the second approach (the one you took). Just update CHANGES.md and I'll merge.

[javagl]

Done.
(I wonder whether the links in the CHANGES.md should be to the issue or to the pull - but they probably are connected enough so that it does not really matter...)