fix(PlanarLayer): Fix delete new extent from globalExtentTMS const

[ketourneau]

Proposed fix for #2244

Motivation and Context

When PlanarLayer is deleted, it doesn't remove extent from globalExtentTMS const. It's why we got some trouble when we try to reload itowns with new extent. It keep previous extent added to globalExtentTMS.

I don't know if it's better to reset globalExtentTMS when view is disposed or remove extent from globalExtentTMS if PlanarLayer is deleted.

I think my fix may cause a problem if two PlanarLayer use the same extent and one was deleted.

What do you think ?

[jailln]

Thanks for finding and correcting this bug :slightly_smiling_face:

It actually corrects the issue but I think that we need to re-think the globalExtentTMS implementation/usage as it doesn't allow to have multiple views with the same CRS and different extents at the same time.

It seems that globalExtentTMS is used for two things:

• actually storing the "global extent" (or default extent) or a CRS (like EPSG:3857 global extent)
• storing the view extent in a global variable and use it to update layers (e.g. in LayeredMaterialNodeProcessing, LabelLayer); in contexts where we already have access to the view and therefore to its extent (with context.view).

I propose that we limit the usage of globalExtentTMS to the first usecase and therefore:

• don't set new values in PlanarLayer constructor.
• Either force to provide an extent for TMSSource or set TMSSource.extent in _preprocessLayer instead than in TMSSource constructor. (I like the second option more)

WDYT @ketourneau @Desplandis ?

Disclaimer: I tried to explain my thoughts as clear as possible but we may discuss it in a voice call if needed :grin:

[ketourneau]

I agree with you. I also wondered whether globalExtentTMS should be used for global extent (like EPSG:3857).

[jailln]

I also wondered whether globalExtentTMS should be used for global extent (like EPSG:3857).

You mean we should remove it totally?

[ketourneau]

No, I just wanted to say that I agree with your suggestion. I'm waiting to see what you decide.

[Desplandis]

@jailln @ketourneau : According to @mgermerie, the root cause of this issue seems to be that tiles of the terrain (as created by the view) does not match that of an actual TMS. He detailed different solutions for this issue in the https://github.com/iTowns/itowns/issues/2290 proposal, which could fix other related issues in iTowns.

[ketourneau]

@Desplandis @mgermerie We also have the same error with cog example when we try to dispose view and load another cog.

If you Load 1 band sample and then Load RGB sample with this commit : https://github.com/bloc-in-bloc/itowns/commit/833fc686e2cc6227fa5262b86deeab5c6ad39382 you will got this result :

It's the same error, when we call view.dispose(true) it doesn't remove extent from globalExtentTMS const. So if we reload view with another extent we got problem.

[Desplandis]

@ketourneau Yeah I think this again a mix between the issue described in #2290 and cache coherency...