-
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
3D Tiles - Support tile expiration #4136
Conversation
9733e3b
to
3f0479c
Compare
What is standard with KML network links? And is it still a good idea today? |
@@ -154,6 +154,9 @@ define([ | |||
* Part of the {@link Cesium3DTileContent} interface. | |||
*/ | |||
Batched3DModel3DTileContent.prototype.initialize = function(arrayBuffer, byteOffset) { | |||
// Destroy expired resources if they exist. | |||
destroyResources(this); |
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.
Throughout the content providers, this is a bit awkward. Is it the cleanest solution? Would it be better to call destroyResources
as part of unloadContentAndMakeEmpty
?
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 kind of agree. The main reason I did that is the i3dm and b3dm contents resolves the contentReadyToProcessPromise
after they have created their resources, so there was no good sync point to destroy the resources from the outside. But if I move those promises to the start of initialize
it would be easier to handle.
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.
So it might be a little more complicated than just that...
Anyway one benefit of the approach here is that it's not tied to the expire or unloading systems. You can just call tile.requestContent
and it will automatically update the content. This is how it works right now; all the more complicated unloadeExpired****
functions are only called when the request fails.
We can talk more tomorrow or after Siggraph.
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.
Let's discuss briefly today when you are free.
Added above:
|
Source/Scene/Cesium3DTileset.js
Outdated
function recheckRefinement(tile) { | ||
var ancestor = getAncestorWithContent(tile); | ||
if (defined(ancestor) && (ancestor.refine === Cesium3DTileRefine.REPLACE)) { | ||
prepareRefiningTiles([ancestor]); |
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.
Does this happen a lot? Is this array allocation OK here?
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.
It doesn't happen much, only when a tile expires for good.
This does not sound ideal to me, but I agree the complexity could be painful. Let's see how this looks in practice and we can write the spec so that the client has some flexibility as to when to stop displaying the old content and start displaying the new content, e.g., the most trivial client might just unload the content before making the request, and the most complex client might alpha blend the old/new contents. |
I need to take a more careful look at this after SIGGRAPH, but I think there is enough here for a demo in our tutorial. |
KML has multiple triggers for refreshing
Since caching is a property of the server/browser working in concert, caching isn't an issue in Google Earth because they can ignore headers and refresh the link without doing anything special. However, we can't control how content gets cached client side because the browser handles that. As far as I know, this leaves us only two options:
The first option puts the burden on the client but is standard and is easy to implement, the second option puts burden on the server and would be easy to get wrong. So I would strongly recommend 1 unless there is a third option I'm not aware of. This is only a problem the browser-based implementations of the spec, since thick clients would be free to ignore headers, just like Google Earth. |
It would be nice if the request included the current cesium time (timeline). Might be out of the scope of this PR, but tiles should also 'expire' if you move backwards in time before the tile should be valid. Since streaming tiles looks to be far out the above changes should allow us to create 3D tiles dynamically on the server side. |
1d68cd8
to
d3aeef7
Compare
d3aeef7
to
6aaff1b
Compare
This is built on top of the To see it working point to
This comment still applies. I'll check how noticeable this is now that we have mixed-lod rendering. Another difference from above is tiles are no longer made empty when the request fails, instead they stay in the failed state which has the same effect. I may rethink this, but its pretty clean this way. |
How did this go? I don't think this is going to be acceptable behavior for most users - do you think folks would use CZML if content could disappear between time intervals? Let me know if you want to discuss in person about how we could come up with a clean implementation. If it is not feasible in the near-term, then let's push tile expiration post 1.0 since it is a niche feature and I do not want to distract us from the core. |
I haven't tried yet, but I'll report back soon.
If we can force the processing stage to be synchronous then I can get around the flicker. Alternatively a tile could store the expired and new content during the frames in which the new content is processing. I think either approach is doable. I am more worried about expired external tilesets since a newly fetched subtree needs to coexist with the old subtree until it is ready to swap in its place (whenever that would be...). A brief period of emptiness might be more acceptable in this case. |
I went with this approach which fixed the empty frames. |
808f4e5
to
df3b31f
Compare
f0ea129
to
39a8c6c
Compare
39a8c6c
to
63b899d
Compare
Sounds good. What is the latest with external tilesets? We could also limit this in the spec to control the scope, but I think this will be a common use case...we need to balance this with the implementation complexity. |
External tilesets will destroy the subtree immediately as it expires, and then load the new one. So there will be empty frames as it reloads. Getting a seamless experience for external tilesets is much harder. |
OK, please write the spec accordingly to allow for this. Is this ready for review? |
The code is basically ready, but I want to write the tests first to be sure. Should be ready soon. |
Updated with tests. |
get : function() { | ||
return this._requestServer; | ||
return this.contentReady || (defined(this._expiredContent) && this._contentState !== Cesium3DTileContentState.FAILED); |
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 could be tweaked depending on the behavior we want. If a request fails do we want the tile to not be rendered, or do we want to render the expired content? This is doing the former.
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.
The former is OK for now. The spec might need some flexibility though, e.g., would some apps want to show the previous content but mark that it is expired? I suspect so. At some point we might need to provide app-level hooks to support this.
@@ -118,8 +120,6 @@ define([ | |||
*/ | |||
this.computedTransform = computedTransform; | |||
|
|||
this._transformDirty = true; |
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.
Removed this since it was only used in point clouds and it was easy to get around.
// Restore properties set per frame to their defaults | ||
this.distanceToCamera = 0; | ||
this.visibilityPlaneMask = 0; | ||
this.selected = false; |
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.
These aren't needed to be reset anymore since they are always updated right away in the traversal.
@lilleyse can you update the tasklist at the top of this PR? |
At quick glance, the code looks good to me. Definitely needs a Sandcastle example. @bagnell could you please review this? It would be good for you to become more familiar with the core 3D Tiles engine. |
The code looks good to me. There is a failing test and +1 for a Sandcastle example. |
For the Sandcastle demo would it be acceptable to modify |
It would make sense to hold off on merging this until the traversal refactor (#5254) is in. |
Not really, because the Sandcastle example wouldn't work with other servers, such as the one we use to deploy on cesiumjs.org. We don't have any Sandcastle examples that rely on server functionality (outside of the proxy, but that's different). |
Ok that's a pretty clear deal breaker. I'm not sure I can include a Sandcastle example for this PR. But at least we have one in https://github.com/AnalyticalGraphicsInc/3d-tiles-samples |
This is ready to go. |
Thanks @lilleyse! |
For CesiumGS/3d-tiles#99
Summary:
When a tile is created it looks at
expire.duration
orexpire.date
to create an expire date. When the tile's content is downloaded, it will update its expire data ifexpireDuration
is defined.In
selectTiles
it checks whether a tile is past it's expire date. If so, the content is marked asEXPIRED
and new content is requested. There are two situations now: the request comes back successfully with new content, or the request fails and the tile is unloaded. If the request succeeds, the old resources will be destroyed in each content'sinitialize
stage right before the new resources are created. Likewise if the content is an external tileset, the old subtree will be unloaded immediately before the new subtree is created inloadTileset
.If the request fails, the tile is unloaded. There are three cases for unloading: the tile is a leaf tile, the tile is a non-leaf tile with content, or the tile content is an external tileset. In the first case the tile is unloaded and removed from the tree. In the second case, the content is unloaded and the tile becomes an empty tile - since this tile may still have children it can't be removed. In the last case, the whole subtree is unloaded and removed from the tree.
With the current design there may be a few frames of emptiness between when the new content is downloaded and when it is ready to render. I don't think this will be a problem, but I'll look out for it as I'm testing. The alternative is a messier to handle because then you have to manage two sets of resources - the old ones that are still rendering and the new ones that are loading. Not to mention handling two subtrees when dealing with external tilesets.
To do:
expire.date
when fetching new content: Tile expiration 3d-tiles#99 (comment)Questions: