Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow external tilesets in multiple contents #12440

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jan 17, 2025

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:

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 🙂

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

Copy link

Thank you for the pull request, @javagl!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant