-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Thank you for the pull request, @lukemckinstry! ✅ We can confirm we have a CLA on file for you. |
There are definitely visibility checks for the tiles (nodes), it's just not in a neat array like |
For Cesium3DTileset tileVisible, the 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) |
It may be worth considering making the nodes part of the public API, and if they and |
There was a problem hiding this 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.
|
||
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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
@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? |
Bump @jjhembd for two questions regarding voxel content above. @lukemckinstry Let me know when this is ready for another pass. |
We only unload tiles when I deleted that |
Description
Takes the following events from Cesium3DTileset and adds them to VoxelPrimitive
allTilesLoaded
initialTilesLoaded
loadProgress
tileLoad
tileUnload
Also makes
VoxelPrimitive
useCesium3DTilesetStatistics
class, but only makes use of 2 of the stats in the class so far.ToDo
selectedTiles
(fromCesium3DTileset
) inVoxelPrimitive
, if so I believe this would be simple.Cesium3dTilesetSpec
toVoxelPrimitiveSpec
.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
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change