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

Fix composite tile caching in ModelExperimental #10524

Merged
merged 6 commits into from
Jul 8, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Jul 8, 2022

Composite (.cmpt) tiles were not handled correctly in ModelExperimental. Since the ResourceCache uses the content URL for cache keys, this is not enough to distinguish multiple models inside a single composite. This PR simply adds a ?compositeIndex=i query parameter to the request, that's enough to distinguish contents in the cache.

Sandcastle for testing -- this includes both the Composite and Composite of Composite models.

As far as testing goes, if you set enableModelExperimental to true in Composite3DTIleContentSpec.js the tests should still pass. Since #10346 will be added soon, it will be simpler to just wait until we set the global flag to true rather than duplicate tests just to remove half of them later.

@j9liu could you review?

@ptrgags ptrgags requested a review from j9liu July 8, 2022 14:41
@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ptrgags ptrgags marked this pull request as ready for review July 8, 2022 14:44
@j9liu
Copy link
Contributor

j9liu commented Jul 8, 2022

@ptrgags this looks good to me, I tried out the 3D Tiles Format sandcastle and composite tileset now displays the intended number of cubes :)

Can you do one quick thing while you're at it -- in the development/3D Tiles Split sandcastle (which is also impacted by this PR), can you change the otherwise in the Sandcastle to catch? I think it's worth fixing that while we're here

@j9liu
Copy link
Contributor

j9liu commented Jul 8, 2022

@ptrgags looks good, just waiting for Travis to pass!

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.

3 participants