iTowns / itowns

A Three.js-based framework written in Javascript/WebGL for visualizing 3D geospatial data
http://www.itowns-project.org
Other
1.1k stars 298 forks source link

Terrain tiles not dividing if not covered by elevation data #2159

Open mgermerie opened 1 year ago

mgermerie commented 1 year ago

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.

AnthonyGlt commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

@gchoqueux WDYT ?

AnthonyGlt commented 3 months ago

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 commented 3 months ago

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
            },
Desplandis commented 3 months ago

@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 commented 2 months ago

@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]:

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.

jailln commented 2 months ago

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)

AnthonyGlt commented 1 month ago

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 ?