Fix coarse DEM elevation

[benoitdm-oslandia]

This PR improves the precision of the QgsDemTerrainGenerator::heightAt function. This function is used to compute the elevation of vector data (Z coordinate) when, for example, the user choose an altitude clamping based on the terrain elevation.

We add a slot function in QgsDemTerrainGenerator class to catch when new dem tiles (height map data) are received from theQgsDemHeightMapGenerator. This height map data are stored in a cache according to the ChunkNodeId (depth/x/y) of the received dem tile.

When a vector layer calls the heightAt function, we look (in the cache) for the best height map data, if not present we fallback to the old (coarse) algorithm.

As vector data can be already loaded when height map data are received, the vector data elevations may not be accurate and need to be updated.

To do so, when we received an height map with high precision (ie. when ChunkNodeId depth is near the max depth of the dem layer), we emit a signal caught by Qgs3DMapScene. The slot function in Qgs3DMapScene:

• looks for scene entities from vector layers whom style uses a terrain altitude clamping
• searchs for chunk nodes intersecting the extent of the height map tile
• asks to refresh these chunk nodes

For now, as a test, we emit the update signal each time a new heigh map is received. This induces more tile updates than needed but produces a smoother rendering. This excess of updates can penalize the user experience when rendering heavy projects.

Sponsored by:

• Frankfurt
• Oslandia @ptitjano @lbartoletti

[github-actions]

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit b9e85af)

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 4a17b20)

🍎 MacOS Qt6 builds

Download MacOS Qt6 builds of this PR for testing.
This installer is not signed, control+click > open the app to avoid the warning
(Built from commit b9e85af)

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[wonder-sk]

I have various reservations about this PR - both at the high level about the overall approach, and also about how things are implemented. I am -1 to merge at this point.

At the high level:

• I am worried about excessive amounts of loads and reloads of tiles - especially if there are multiple vector layers, this can trigger a long cascade of updates as new vector chunks and new terrain tiles get loaded

• with the proposed approach, QgsTerrainGenerator::heightAt() can arbitrarily change returned values over time based on how camera is moving - in my opinion this is quite unfortunate and may likely cause problems later. Right now terrain heights are only used for altitude clamping of vector layer chunks, but if this ever gets extended to other functionality, we would extra mechanisms to be able to react to height changes as they happen as the camera moves

• things seem very hard-coded towards DEM terrain, missing abstractions (e.g. notifications from terrain generator when heights have changed). How about support of other terrain types (e.g. quantized mesh) - is that out of scope? (I am -1 to skip other terrain types)

For the high level approach, my suggestion is to use what CesiumJS does: whenever we need heights at some X,Y coordinates, we should fetch the most detailed terrain tile to satisfy the request and use height from there. In this way, elevations of vector layer chunks never need to be updated - this is very important. And for a single tile, there are typically very few tile requests needed. Some may argue that with this approach, terrain and objects clamped to it may not look right together - but here my argument is that the vertical misalignment happens only when looking from far away and thus minor misalignment is barely noticeable (after all, even in the approach in this PR the vector layer chunks do not get altitudes updates to less accurate terrain tiles as camera moves away).

There are further issues with the code design in this PR:

• Qgs3DMapScene now expects signals from QgsDemTerrainGenerator and directly updates chunks of vector layers - that kind of code does not really belong to a general class like Qgs3DMapScene - proper abstractions should be used

• QgsDemTerrainGenerator now expects signals from QgsDemHeightMapGenerator, even though these two classes are meant to be completely independent from each other - this kind of communication through signals significantly increases code complexity

• QgsDemHeightMapGenerator now deals with raw pointers to QgsChunkNode, although it only needs tile IDs, just because these will be passed in a signal to QgsDemTerrainGenerator - this also looks like bad design

• QgsDemTerrainGenerator has a new member - mRootNode - but the root node owned by QgsTerrain, and later QgsTerrain may get deleted, while terrain generator stays! There isn't 1:1 relation between QgsDemTerrainGenerator and a root chunk node, so this approach is going to cause issues

Some more implementation isses:

• mLoaderMap - it is a dictionary that grows without any bounds - that should not really happen, or we may use loads of RAM to keep terrain tile heightmaps even when they are not needed

• mLoaderMap - why use QString instead of QgsChunkNodeId as a key?

• thread safety issues with mLoaderMap/mRootNode - mutex is not always locked when mRootNode or mLoaderMap is accessed

• QgsTerrainGenerator::qualityAt() is a new method, but not used in production code - should it exist? On top of that, quality is returned as "int" which is quite misleading - in 3D we typically deal with "float" geometry error specified in world units

• QgsDemTerrainGenerator::heightAndQualityAt() - I am struggling to understand how a node's chunk loader can be used to extract height map - the chunk loader instances are meant to exist only for a short period of time, and no code outside of QgsChunkedEntity should deal with them. The whole method is quite complicated - why do we use both mLoaderMap and chunk loader to fetch heightmap, and when is used one or the other? (my expectation would be that only mLoaderMap is needed)

• Qgs3DMapScene::onNewDemTileReceived() - how about updates of nodes that are loaded, but they are not active? Those do not seem to be getting updated elevation values, which seems wrong

• There are various code changes in QgsChunkedEntity, Qgs3DMapScene, QgsTessellator that seem completely unrelated to this PR (e.g. static casts) - this makes the review even more complicated

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[benoitdm-oslandia]

Hi @wonder-sk, it has been a long time since I wanted to answer to your review. But I have been on other projects since. First, I want to thank you for the review process. I should have put this PR as a draft as it was mainly an improved POC to check what can be done.

I will try to answer to yours questions even if we disagree on the final solution.

I have various reservations about this PR - both at the high level about the overall approach, and also about how things are implemented. I am -1 to merge at this point.

At the high level:

• I am worried about excessive amounts of loads and reloads of tiles - especially if there are multiple vector layers, this can trigger a long cascade of updates as new vector chunks and new terrain tiles get loaded

Only the tiles affected by the elevation data updates will be impacted, not the entire scene. I would also like to point out that currently, every time a style on a layer or when a layer is enabled/disabled then the layer entities are dropped and redrawn. Also when terrain configuration or 3D effect are changed, the entire scene is reloaded. This current workflow is already in use and does not seem to cause any problems.

• with the proposed approach, QgsTerrainGenerator::heightAt() can arbitrarily change returned values over time based on how camera is moving - in my opinion this is quite unfortunate and may likely cause problems later.

The elevation data loading has been done on purpose as this is an incremental elevation update approach made to keep coherency in tile resolutions (fine resolution tiles get fine elevation). Currently vector data are loaded by chunk as the camera moves and it does not seem to create issues. Have you some specific issues in mind?

• Right now terrain heights are only used for altitude clamping of vector layer chunks, but if this ever gets extended to other functionality, we would extra mechanisms to be able to react to height changes as they happen as the camera moves

And there is already a new signal emitted when new fine DEM tiles are retrieved. Are you thinking about something else?

• things seem very hard-coded towards DEM terrain, missing abstractions (e.g. notifications from terrain generator when heights have changed). How about support of other terrain types (e.g. quantized mesh) - is that out of scope? (I am -1 to skip other terrain types)

We mainly focused on DEM terrain because they are most commonly used. I agree to add a new abstraction level for any terrain type. This PR will implement the DEM case and other type can be added later.

For the high level approach, my suggestion is to use what CesiumJS does: whenever we need heights at some X,Y coordinates, we should fetch the most detailed terrain tile to satisfy the request and use height from there. In this way, elevations of vector layer chunks never need to be updated - this is very important.

This will produce long load time when the scene is brought up. If we load all DEM data at 3D scene startup time, it looks like we do not respect all the chunk/quad tree mechanisms (used as the 3d scene roots)? Why should we not respect that idiom?

And for a single tile, there are typically very few tile requests needed. Some may argue that with this approach, terrain and objects clamped to it may not look right together - but here my argument is that the vertical misalignment happens only when looking from far away and thus minor misalignment is barely noticeable (after all, even in the approach in this PR the vector layer chunks do not get altitudes updates to less accurate terrain tiles as camera moves away).

Even if the height differences won't be seen at this distance, for a big scene you will need to fetch all the fine elevation data. This can be a huge data set depending a the DEM precision and this would not be desirable.

after all, even in the approach in this PR the vector layer chunks do not get altitudes updates to less accurate terrain tiles as camera moves away

Indeed but reloading at lower res the tiles when the camera moves away will generate an extra and useless cpu works.

There are further issues with the code design in this PR:

• Qgs3DMapScene now expects signals from QgsDemTerrainGenerator and directly updates chunks of vector layers - that kind of code does not really belong to a general class like Qgs3DMapScene - proper abstractions should be used

There is already finalizeNewEntity or onLayerRenderer3DChanged functions doing likewise works in Qgs3DMapScene. I put it there because when a new highres tile is retrieved we need to update all vector layers but more precisely, only the outdated chunk nodes. This mean we need to have access to all vector layers. Can you tell me more about the kind of abstraction are you thinking about?

• QgsDemTerrainGenerator now expects signals from QgsDemHeightMapGenerator, even though these two classes are meant to be completely independent from each other - this kind of communication through signals significantly increases code complexity

I do not understand QgsDemHeightMapGenerator is only used by QgsDemTerrainGenerator (it is only instanciated in QgsDemTerrainGenerator), they do not look so completely independent from each other. Also slot/signals are the core engine of Qt and are used widely in QGIS code. How can this significantly increases code complexity compare to other QGIS classes? I should have missed something here.

• QgsDemHeightMapGenerator now deals with raw pointers to QgsChunkNode, although it only needs tile IDs, just because these will be passed in a signal to QgsDemTerrainGenerator - this also looks like bad design

Indeed this can be fixed

• QgsDemTerrainGenerator has a new member - mRootNode - but the root node owned by QgsTerrain, and later QgsTerrain may get deleted, while terrain generator stays! There isn't 1:1 relation between QgsDemTerrainGenerator and a root chunk node, so this approach is going to cause issues

Indeed this can be fixed

Some more implementation isses:

• mLoaderMap - it is a dictionary that grows without any bounds - that should not really happen, or we may use loads of RAM to keep terrain tile heightmaps even when they are not needed

Can be fixed by listening when tiles are unloaded so the the map data can be unloaded. Also this dictionnary will never reach the size needed to load the full DEM.

• mLoaderMap - why use QString instead of QgsChunkNodeId as a key?

to avoid creating a QgsChunkNodeId each time we need a value in the map. Can be fixed easily

• thread safety issues with mLoaderMap/mRootNode - mutex is not always locked when mRootNode or mLoaderMap is accessed

Will be fixed

• QgsTerrainGenerator::qualityAt() is a new method, but not used in production code - should it exist? On top of that, quality is returned as "int" which is quite misleading - in 3D we typically deal with "float" geometry error specified in world units

It is mainly linked to the fact we can have diffent tile coarseness in the QMap (as we do not load all the high res DEM tiles). The int choice is arbitrary and the quality value can be express in a better way. For now it is not used, but I think it can be useful for a user to know the accuracy of an entity elevation (when snapping for example). It also can be dropped if I am wrong.

• QgsDemTerrainGenerator::heightAndQualityAt() - I am struggling to understand how a node's chunk loader can be used to extract height map - the chunk loader instances are meant to exist only for a short period of time, and no code outside of QgsChunkedEntity should deal with them. The whole method is quite complicated - why do we use both mLoaderMap and chunk loader to fetch heightmap, and when is used one or the other? (my expectation would be that only mLoaderMap is needed)

Indeed, this function can be stripped off

• Qgs3DMapScene::onNewDemTileReceived() - how about updates of nodes that are loaded, but they are not active? Those do not seem to be getting updated elevation values, which seems wrong

Indeed this should be fixed

• There are various code changes in QgsChunkedEntity, Qgs3DMapScene, QgsTessellator that seem completely unrelated to this PR (e.g. static casts) - this makes the review even more complicated

clang tidy is not stranger to theses changes. Somes could be reverted.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[benoitdm-oslandia]

Hi @wonder-sk, can you have a look to this PR please? Regards.

[github-actions]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[github-actions]

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.