itowns icon indicating copy to clipboard operation
itowns copied to clipboard

Terrain tiles not dividing if not covered by elevation data

Open mgermerie opened this issue 1 year ago • 10 comments

Context

When displaying elevation data on a TiledGeometryLayer, the later stops subdividing its tiles when the attached ElevationLayer zoom range is overtaken.

Steps to Reproduce (for bugs)

This is a minimal example to reproduce the issue :

const placement = {
    coord: new itowns.Coordinates('EPSG:4326', 2.351323, 48.856712),
    range: 25000000,
}
const viewerDiv = document.getElementById('viewerDiv');
const view = new itowns.GlobeView(viewerDiv, placement);

itowns.Fetcher.json('./layers/JSONLayers/Ortho.json').then(function _(config) {
    config.source = new itowns.WMTSSource(config.source);
    view.addLayer(new itowns.ColorLayer('Ortho', config));
});

itowns.Fetcher.json('./layers/JSONLayers/WORLD_DTM.json').then((config) => {
    config.source = new itowns.WMTSSource(config.source);
    view.addLayer(new itowns.ElevationLayer(config.id, config));
});

While running it, enter view.controls.setZoom(17); in console.

Expected Behavior

Terrain tiles should subdivide to display ortho images with enough resolution.

Actual Behavior

Terrain tiles stop subdividing at some point, since no elevation data is available after zoom level 10.

Possible Cause/Fix/Solution

This is due to this part, called when updating the terrain layer.

mgermerie avatar Jul 27 '23 15:07 mgermerie

Do you have an idea about the real impact of deleting this code ? https://github.com/iTowns/itowns/blob/cba214626dc485318e3e3315aa442eecaaff99c2/src/Layer/TiledGeometryLayer.js#L402-L416?

Looks to me that it will solve the issue. The original code comes from https://github.com/iTowns/itowns/pull/538 https://github.com/iTowns/itowns/blob/179fa6b176f12aaa49bf1bc35ca75b0bd5e96c9d/src/Process/GlobeTileProcessing.js#L126-L131 and https://github.com/iTowns/itowns/pull/792 https://github.com/iTowns/itowns/blob/182ba92f3d854ac7305c840faea3d1e1c44a7e29/src/Process/GlobeTileProcessing.js#L129-L137

AnthonyGlt avatar Mar 29 '24 09:03 AnthonyGlt

Also, from this merge https://github.com/iTowns/itowns/pull/947 (Dec 7, 2018) to this merge https://github.com/iTowns/itowns/pull/2119 (Jun 27, 2023)

The condition was never true if (ratio < 1 / 2 ** this.maxDeltaElevationLevel) { because maxDeltaElevationLevel was undefined...

So, I think that we could just remove this code, what do you think @mgermerie ?

If I understand correctly, this code reduce the subdivision for better perf. Should we just change the value of maxDeltaElevationLevel ?

AnthonyGlt avatar Mar 29 '24 14:03 AnthonyGlt

By adding a 3d model below the terrain and removing this code, I have the following issue, so we should not remove this code.

Screencast from 29-03-2024 17:27:13.webm

AnthonyGlt avatar Mar 29 '24 16:03 AnthonyGlt

@gchoqueux WDYT ?

AnthonyGlt avatar Apr 18 '24 14:04 AnthonyGlt

By adding a 3d model below the terrain and removing this code, I have the following issue, so we should not remove this code. Screencast.from.29-03-2024.17.27.13.webm

I'm not able to reproduce this bug again, this is worrying. If we decide to go by removing this code, we should pay close attention if this bug appears again...

AnthonyGlt avatar Jun 24 '24 08:06 AnthonyGlt

We were able to reproduce again by loading a low range (580) and using the WORLD_DTM.json by removing the layerstrategy and adding these 2 in tileMatrixSetLimits

        "1": {
                "minTileRow": 0,
                "maxTileRow": 1,
                "minTileCol": 0,
                "maxTileCol": 4
            },
            "2": {
                "minTileRow": 0,
                "maxTileRow": 3,
                "minTileCol": 0,
                "maxTileCol": 8
            },

AnthonyGlt avatar Jul 05 '24 14:07 AnthonyGlt

@AnthonyGlt This is more of a problem of the strategy used by the elevation layer to fetch the texture that adding zoom levels.

We can reproduce this issue by removing the explicit stategy set in WORLD_DTM.json. Instead of the STRATEGY_GROUP strategy, the default strategy of iTowns (STRATEGY_MIN_NETWORK_TRAFFIC) cause the issue in the video above.

Desplandis avatar Jul 08 '24 13:07 Desplandis

@AnthonyGlt @mgermerie @jailln I got a bit more of context concerning fetching strategies for raster layers. This provide an explanation for both your bug (@AnthonyGlt) and the need to stop subdividing the terrain (i.e. this issue) to kinda "patch" this bug.

TL;DR: Until we fix the issue with our default strategy, we could use STRATEGY_DICHOTOMY to mitigate the issue with terrain.

@AnthonyGlt I'll let you submit a patch on this issue.

Raster Layer strategies

All fetching strategies take at least two mandatory parameters : the current node level nodeLevel and the last level of fetched texture currentLevel (which could be -1 if no texture have been fetched). Some strategies may have additional parameter.

STRATEGY_GROUP

The STRATEGY_GROUP level take as additional parameter a list of ordered levels (e.g. [3, 7, 12]) which maps intervals of levels to single level to fetch. For example, given [3, 7, 12]:

  • $\textbf{nodeLevel} \in \left[ -1, 3 \right] \implies 3$
  • $\textbf{nodeLevel} \in \left] 3, 7 \right] \implies 7$
  • $\textbf{nodeLevel} \in \left] 7, 12 \right] \implies 12$
  • $\textbf{nodeLevel} \in \left]12, +\infty\right] \implies 12$

STRATEGY_DICHOTOMY

The STRATEGY_DICHOTOMY (as its name suggests) fetches the mid-level between currentLevel and nodeLevel. For example, for a currentLevel = 0 and nodeLevel = 8, it should take 4 fetches to have the level 4 texture : 4, 6, 7 and 8. Note that we cache textures so we should not have redundant fetches.

STRATEGY_PROGRESSIVE

The STRATEGY_PROGESSIVE take as additional parameter a number which acts as a kinda step (defaults to 1) and fetches at currentLevel + step until we reach nodeLevel. For example, for a currentLevel = 0 and nodeLevel = 8 and stop = 1, it should take 8 fetches to have the level 8 texture.

STRATEGY_MIN_TRAFFIC

The STRATEGY_MIN_TRAFFIC fetches the currentLevel. Note that we always fallback to the best quality texture if it exists, this is the cause of the bug in the PR.

The bug

In your example, you initialize a view close to the terrain, say at level $16$, with the MIN_TRAFFIC strategy with an elevation layer with data up to $10$. In this context, no tile at level $<11$ have been loaded, and the MIN_TRAFFIC strategy only tries to fetch textures at level $16$. With no fallback texture, no elevation is applied to our tile.

Desplandis avatar Jul 12 '24 20:07 Desplandis

Thanks for this clear analysis @Desplandis :1st_place_medal:

Can you add it to the documentation please? (and add LayerUpdateStrategy in the doc config file because it's not there)

jailln avatar Jul 15 '24 17:07 jailln

It looks like it works because with STRATEGY_DICHOTOMY ALL the level are loaded (1 to 10). I don't think this is normal behavior. In STRATEGY_MIN_TRAFFIC, we have avoid this thanks to this condition: https://github.com/iTowns/itowns/blob/d091207146600f80dfd755c5616a16d9333718b0/src/Layer/LayerUpdateStrategy.js#L17-L18 (from eb237cc78cad20dac81fd2723462400d1c751ba5)

If we add this condition in STRATEGY_DICHOTOMY (I think we should), we don't load all the levels anymore but we have the same behavior as STRATEGY_MIN_TRAFFIC but the bug is back.

I think the issue could be corrected by adding:

    if (nodeLevel > maxZoom ) {
        return maxZoom;
    }

After https://github.com/iTowns/itowns/blob/d091207146600f80dfd755c5616a16d9333718b0/src/Layer/LayerUpdateStrategy.js#L53 So we don't try to reach unreachable level and fallback on the zoom max. After some tests, it looks like it resolves the issue.

WDYT @Desplandis ?

AnthonyGlt avatar Aug 26 '24 09:08 AnthonyGlt