Skip to content

Fix availability check in VoxelTraversal for procedural tilesets #12458

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

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Feb 3, 2025

Description

This PR fixes a small bug introduced in #12430 affecting procedural tilesets.

The requestData helper function in VoxelTraversal included a check of provider._implicitTileset.availableLevels to avoid generating spurious tileFailed events. However, custom procedural providers (as used in some of our sandcastle examples) do not have an ._implicitTileset.

This PR adds a public availableLevels property to VoxelProvider, which is allowed to be undefined. VoxelTraversal now verifies the property is defined before checking its value.

Issue number and link

Testing plan

Load the "Voxels" Sandcastle and verify that all datasets render without errors.

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 Fixes a bug not present in the previous release
  • 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

@jjhembd jjhembd requested a review from lukemckinstry February 3, 2025 17:09
Copy link

github-actions bot commented Feb 3, 2025

Thank you for the pull request, @jjhembd!

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

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @jjhembd!

Would it be possible to add a unit test for this case to prevent a similar regression in the future? I'm surprised we don't have one that failed already!

@jjhembd
Copy link
Contributor Author

jjhembd commented Feb 3, 2025

Would it be possible to add a unit test for this case to prevent a similar regression in the future? I'm surprised we don't have one that failed already!

@ggetz The specs that actually loaded tiles were all rendering one of the test 3D Tiles voxel datasets. I added a spec to load a minimal procedural provider. This spec fails in main, and succeeds in this branch.

@lukemckinstry
Copy link
Contributor

@jjhembd this fix looks good to me

@ggetz
Copy link
Contributor

ggetz commented Feb 3, 2025

Thanks @jjhembd and @lukemckinstry!

@ggetz ggetz added this pull request to the merge queue Feb 3, 2025
Merged via the queue into main with commit 8e05598 Feb 3, 2025
9 checks passed
@ggetz ggetz deleted the voxel-availability branch February 3, 2025 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants