Allow external tilesets in multiple contents #12440
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #11960
Description
It removes the check that previously did throw a
RuntimeError
with the messagewhen 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:
Cesium3DTileset.requestContent
for a certain tileCesium3DTile.requestContent
. When the tile contained thecontents
array, this delegates toCesium3DTile.requestMultipleContents
Multiple3DTileContent
on which it callsrequestInnerContents
. This callsrequestInnerContent
for each content, then callscreateInnerContents
which waits for the requests, and then callscreateInnerContent
for each response.Multiple3DTileContent.createInnerContent
did throw the error when it found that the content type wasEXTERNAL_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 aTileset3DTileContent
.Caveats....
For single-content external tilesets, an external tileset caused the
tile.hasTilesetContent = true;
to be set. And thishasTilesetContent
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
turnstrue
, it causesdestroySubtree
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 beingtrue
there is fine as well.The
Cesium3DTile#hasRenderableContent
may be the most critical one. It was implemented asA 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:true
fortile.contentAvailable
to ever betrue
true
fortile.hasUnloadedRenderableContent
to betrue
. ThehasUnloadedRenderableContent
is checked in various places. It has effects on loading, often in the context of expiration.true
fortile.unloadContent
to do anything(!)applyDebugSettings
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:
hasTilesetContent
also has to be set totrue
when the external tileset is one of multiple contents.hasRenderableContent
must usually betrue
, except for the case of 1. empty content, 2. single external, 3. single implicitSo made
hasRenderableContent
an explicit variable (not just a getter that returns the&&
-result of three others). This has to be set totrue
orfalse
, consistent with its previous behavior and the assumptions.Previously,
hasRenderableContent
depended onhasEmptyContent
,hasTilesetContent
, andhasImplicitContent
. What makes this more difficult are the comments inhasTilesetContent
andhasImplicitContent
that sayThis 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, thenhasRenderableContent
will returntrue
by default. Ouch.So what is done here:
hasRenderableContent
is a variable that starts astrue
except for empty contentfalse
when encountering a single content that is an external tileset or implicit contentIssue 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();
inCesium3DTileset.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:
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 🙂
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change