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

Add tile loading events for Voxels #12430

Merged
merged 14 commits into from
Jan 27, 2025
Merged

Add tile loading events for Voxels #12430

merged 14 commits into from
Jan 27, 2025

Conversation

lukemckinstry
Copy link
Contributor

@lukemckinstry lukemckinstry commented Jan 10, 2025

Description

Takes the following events from Cesium3DTileset and adds them to VoxelPrimitive

  • allTilesLoaded
  • initialTilesLoaded
  • loadProgress
  • tileLoad
  • tileUnload

Also makes VoxelPrimitive use Cesium3DTilesetStatistics class, but only makes use of 2 of the stats in the class so far.

ToDo

  • Did not carry over these event types
  • Still need to add tests, but these look straighforward to carry over from Cesium3dTilesetSpec to VoxelPrimitiveSpec.
  • move code to call events out of VoxelTraversal.printDebugInformation.

Issue number and link

related to #12297

Testing plan

Many events currently fire inside of VoxelTraversal.printDebugInformation. Enable this but comment out the last line of the function which logs the debug output so you can read the output from listeners you set up to listen to the new events.

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

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

@ggetz
Copy link
Contributor

ggetz commented Jan 14, 2025

@lukemckinstry

tileVisible - not sure is there is an equivalent to selectedTiles (from Cesium3DTileset) in VoxelPrimitive, if so I believe this would be simple.

There are definitely visibility checks for the tiles (nodes), it's just not in a neat array like selectedTiles are unfortunately. I believe all the visibility checks happen within generateOctree– Once the else condition is reached in buildOctree and the function no longer recurses, I think that is the highest resolution node which has been marked as renderable.

@lukemckinstry
Copy link
Contributor Author

lukemckinstry commented Jan 15, 2025

There are definitely visibility checks for the tiles
I see now where tileVisible would fire within the generateOctree function.

For Cesium3DTileset tileVisible, the Tile is passed to the event listener so the user can glean info and modify it before the next render. For VoxelPrimitive, the spatialNode is not part of the public API, so do we want to pass it to event listener in this same way?

If not, would we want to use this event, does it make sense to raise the tileVisible event without passing any info about the specific spatialNode? (I implemented this way for now fcd9a9c)

@ggetz
Copy link
Contributor

ggetz commented Jan 17, 2025

For Cesium3DTileset tileVisible, the Tile is passed to the event listener so the user can glean info and modify it before the next render. For VoxelPrimitive, the spatialNode is not part of the public API, so do we want to pass it to event listener in this same way?

It may be worth considering making the nodes part of the public API, and if they and Cesium3DTile should conform to some interface. Probably something worth noting in #12297, but a bit much to bite off in this PR. For now, I think it's fine not to pass anything.

@lukemckinstry
Copy link
Contributor Author

lukemckinstry commented Jan 17, 2025

This is ready for review. @ggetz @jjhembd
ToDo item

  • I do not have a unit test yet for the tileUnload event. There is an existing xited out spec covering the voxel unload process. This seems like it would be the right place to test the tileUnload event.

@lukemckinstry lukemckinstry marked this pull request as ready for review January 17, 2025 21:36
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.

Looking good so far @lukemckinstry! A few comments, mostly around the inline documentation.

packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved
packages/engine/Source/Scene/VoxelPrimitive.js Outdated Show resolved Hide resolved

if (defined(promise)) {
that._simultaneousRequestCount++;
keyframeNode.state = KeyframeNode.LoadState.RECEIVING;
promise.then(postRequestSuccess).catch(postRequestFailure);
} else {
keyframeNode.state = KeyframeNode.LoadState.FAILED;
that._primitive.tileFailed.raiseEvent();
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a question beyond the scope of this PR, but when a provider returns undefined instead of a promise, is that actually a failure state? Typically when undefined is returned from a function like this, it signals that the request couldn't be scheduled this frame and will be tried again next frame.

CC @jjhembd

Copy link
Contributor

Choose a reason for hiding this comment

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

For both Cesium3DTilesVoxelProvider and the procedural providers in the Sandcastles, an undefined return happened when the tile didn't exist. I agree that this is not really expected behavior.

In #12432 I changed the providers to return a rejected Promise if the tile doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a few comments in #12432 that mentioned undefined may still be a valid return value that needs to be accounted for. But, yes, it should never be a failure state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #12432 I changed the providers to return a rejected Promise if the tile doesn't exist.

This change here as described above

function postRequestSuccess(result) {

This simplifies the logic on whether to raise the tileLoad or tileFailed event
Something like this at the end of the loadTileSuccess would be all that is needed.

    if (keyframeNode.state === KeyframeNode.LoadState.PROCESSING) {
      that._primitive.tileLoad.raiseEvent();
    }
    that._primitive.tileFailed.raiseEvent();

@ggetz
Copy link
Contributor

ggetz commented Jan 23, 2025

@jjhembd Would you be able to provide any tips on testing the unload event since you were recently looking into that part of the code?

@ggetz
Copy link
Contributor

ggetz commented Jan 27, 2025

Bump @jjhembd for two questions regarding voxel content above.

@lukemckinstry Let me know when this is ready for another pass.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 27, 2025

@jjhembd Would you be able to provide any tips on testing the unload event since you were recently looking into that part of the code?

We only unload tiles when Megatexture.prototype.isFull() AND we are loading a new tile. The datasets we are using in the specs are all single-tile data, so it is not possible to trigger an unload.

I deleted that xit spec in #12432, since it also had other issues with the way it handled async loading. To get a meaningful spec, we will need to start with a bigger test dataset.

@ggetz ggetz added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 4e6dde5 Jan 27, 2025
5 checks passed
@ggetz ggetz deleted the issue-12297-voxel-stats branch January 27, 2025 20:37
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