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(OGC3DTilesLayer): handle multiple views #2435

Merged
merged 4 commits into from
Oct 18, 2024

Conversation

AnthonyGlt
Copy link
Contributor

@AnthonyGlt AnthonyGlt commented Oct 11, 2024

Description

Handle the 3dtiles cache in each view when multiple views are created

Motivation and Context

#2426

Modifications

Adding an id to the view. Incremented at each new view.
Adding a list of cache and queues in OGC3DTilesLayer.js, to share them if in in the same view

Improvements

@jailln @Desplandis

How to handle a View disposing ?
Also, a "viewers" constant already exist in View.js, maybe this one could be used ?

const viewers = [];

@@ -22,19 +22,14 @@ import PointsMaterial, {
} from 'Renderer/PointsMaterial';

const _raycaster = new THREE.Raycaster();

const viewers = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment saying that it stores ids of viewers where tilesets have been added and explaining why and how it is related to the cache please ? You can also link the issue I think

@jailln
Copy link
Contributor

jailln commented Oct 11, 2024

@AnthonyGlt when a view is disposed, all layers added to this view are removed and their object3Ds are removed from the scene so it should be handled automatically; but it's worth checking that everything works fine :)

@AnthonyGlt
Copy link
Contributor Author

@AnthonyGlt when a view is disposed, all layers added to this view are removed and their object3Ds are removed from the scene so it should be handled automatically; but it's worth checking that everything works fine :)

I meant that in the viewers of OGC3DTilesLayer here

https://github.com/iTowns/itowns/pull/2435/files#diff-05568d5784ac6e47fd8c2b288edaf3965abce4a92c99c0b8bec7de4ec68a5f77R29

We will store cache & queues for each view.

Once a view is disposed, the corresponding cache & queue will still be stored in the viewers, so we never remove anything from viewers

@jailln

@jailln
Copy link
Contributor

jailln commented Oct 11, 2024

If you want to remove it from viewers I think you should do it here (called when a view holding this layer is disposed)

@AnthonyGlt
Copy link
Contributor Author

If you want to remove it from viewers I think you should do it here (called when a view holding this layer is disposed)

@jailln it should be disposed when the view is disposed, not the layer, because we share it between all layers of a same view.
It could be linked to the disposal of the view, right ?

itowns/src/Core/View.js

Lines 283 to 292 in d73f0a1

* Dispose viewer before delete it.
*
* Method dispose all viewer objects
* - remove control
* - remove all layers
* - remove all frame requester
* - remove all events
* @param {boolean} [clearCache=false] Whether to clear all the caches or not (layers cache, style cache, tilesCache)
*/
dispose(clearCache = false) {

@jailln
Copy link
Contributor

jailln commented Oct 14, 2024

@jailln it should be disposed when the view is disposed, not the layer, because we share it between all layers of a same view. It could be linked to the disposal of the view, right ?

Yes you're right it should be disposed when the view is disposed. Normally, when a layer is disposed, the tiles of this layer (only) should be removed from the cache (this should be handled by 3DTilesRenderer with tilesRenderer.dispose()). When a view is disposed, all layers from this view are disposed, so the lruCache cache corresponding to this view should be emptied (also because tilesRenderer.dispose()) will be called for each layer). Therefore, in my opinion nothing more should be done for emptying the cache. WDYT? It's worth checking this out in practice though :) Can you validate it's working correctly?

What you may want to do in addition is to remove the view and its associated lruCache, downloadQueue and parseQueue from the viewers when a view is disposed? You can setup an event when a view is disposed to do that for instance?

@AnthonyGlt AnthonyGlt requested a review from jailln October 14, 2024 10:03
@AnthonyGlt
Copy link
Contributor Author

AnthonyGlt commented Oct 14, 2024

@jailln

What you may want to do in addition is to remove the view and its associated lruCache, downloadQueue and parseQueue from the viewers when a view is disposed? You can setup an event when a view is disposed to do that for instance?

That was what I was talking about :)

I've pushed some edits, maybe a bit overkill to pass the view as parameter but it works.
WDYT ?

Copy link
Contributor

@jailln jailln left a comment

Choose a reason for hiding this comment

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

thanks :)

@AnthonyGlt AnthonyGlt requested a review from jailln October 15, 2024 07:54
@jailln
Copy link
Contributor

jailln commented Oct 15, 2024

Ok for me, thanks for the changes :) @Desplandis @mgermerie do you want to take a look at it or should we merge?

@Desplandis
Copy link
Contributor

Okay for me, but we'll need to have a better way to handle shared ressources between layers of a given view!

@AnthonyGlt AnthonyGlt merged commit b991878 into iTowns:master Oct 18, 2024
9 checks passed
jailln pushed a commit to jailln/itowns that referenced this pull request Oct 30, 2024
fix(OGC3DTilesLayer): handle multiple views cache
jailln pushed a commit to jailln/itowns that referenced this pull request Oct 31, 2024
fix(OGC3DTilesLayer): handle multiple views cache
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