[3D] Correctly compute near/far planes + culling when terrain offset is not null

[ptitjano]

Description

When the terrain offset is not null, the culling methods and near/far planes computations are wrong because they only rely on the bounding boxes which do not take into account the offset.

This is fixed by adding a terrainElevationOffset method to the entities and adding it to the near/far plane computation and culling logic.

A new unit test for terrain offset is also added.

Wrong behavior: current master

offset_wrong_compress.webm

Fixed behavior: this PR

offset_ok_compress.webm

cc @benoitdm-oslandia @soaubier

[uclaros]

This was on my radar for some time. Should we not just update the entities' bboxes instead so that bbox() returns their actual adjusted bbox?

[wonder-sk]

Thanks for looking into this issue - but indeed the correct solution would be what @uclaros suggests.

QgsChunkedEntity at this point has no ties to terrain whatsoever, and I think that should stay like that - we should not need to have a virtual function terrainElevationOffset() in there. So rather than doing any modifications in QgsChunkedEntity, we should be modifying terrain-related code to return correct bboxes (otherwise if bboxes are not correct, this kind of problem will appear elsewhere as well). I guess we'll need to modify places like:

• QgsTerrainGenerator::rootChunkBbox() - include terrain offset there
• QgsTerrainEntity - stop using mTerrainTransform and leave it for individual chunk entities to set up correct transform
• QgsDemTerrainTileLoader::createEntity() - apply vertical offset in its transform + include vertical offset when updating node's bbox with setExactBbox() call

I would also suggest to remove QgsTerrainEntity::terrainElevationOffset() that exists already, it is only used in QgsCameraController::setViewFromTop() and it is just a proxy for Qgs3DMapSettings::terrainElevationOffset() anyway...

(by the way, the QgsRuleBasedChunkedEntity::applyTerrainOffset() implementation would be best suited for a separate PR, as it is not directly related to the main problem this PR is trying to solve...)

[ptitjano]

Thanks for looking into this issue - but indeed the correct solution would be what @uclaros suggests.

QgsChunkedEntity at this point has no ties to terrain whatsoever, and I think that should stay like that - we should not need to have a virtual function terrainElevationOffset() in there. So rather than doing any modifications in QgsChunkedEntity, we should be modifying terrain-related code to return correct bboxes (otherwise if bboxes are not correct, this kind of problem will appear elsewhere as well). I guess we'll need to modify places like:

* QgsTerrainGenerator::rootChunkBbox() - include terrain offset there

* QgsTerrainEntity - stop using mTerrainTransform and leave it for individual chunk entities to set up correct transform

* QgsDemTerrainTileLoader::createEntity() - apply vertical offset in its transform + include vertical offset when updating node's bbox with setExactBbox() call

I would also suggest to remove QgsTerrainEntity::terrainElevationOffset() that exists already, it is only used in QgsCameraController::setViewFromTop() and it is just a proxy for Qgs3DMapSettings::terrainElevationOffset() anyway...

(by the way, the QgsRuleBasedChunkedEntity::applyTerrainOffset() implementation would be best suited for a separate PR, as it is not directly related to the main problem this PR is trying to solve...)

Thanks for your comment. However, there is something I don't fully understand. Based on your suggestion, I understand that only the bboxes of the terrain need to be updated. However, the bounding boxes of QgsVectorLayerChunkedEntity and QgsRuleBasedChunkedEntity need to be upated because these entities are also translated if the terrain offset applies to them. That's the main reason for introducing the QgsRuleBasedChunkedEntity::applyTerrainOffset() commit and the introduction of terrainElevationOffset()

[ptitjano]

* QgsTerrainEntity - stop using mTerrainTransform and leave it for individual chunk entities to set up correct transform

I also don't understand why you want to remove mTerrainTransform and replace with one by individual chunk entities. Could you please explain?

[github-actions[bot]]

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.

[ptitjano]

@wonder-sk gentle ping.

[github-actions[bot]]

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]

Based on your suggestion, I understand that only the bboxes of the terrain need to be updated. However, the bounding boxes of QgsVectorLayerChunkedEntity and QgsRuleBasedChunkedEntity need to be upated because these entities are also translated if the terrain offset applies to them.

Yeah those will likely need updating as well to include the terrain offset in their bounding boxes - my comment meant to just add some pointers where I would be expecting modifications, rather than a full list, sorry for not being clear about that.

I also don't understand why you want to remove mTerrainTransform and replace with one by individual chunk entities. Could you please explain?

IIRC the mTerrainTransform will also transform bounding boxes entity (if you have display of bounding boxes enabled) because they are parented to the terrain entity, so if the transform is limited to individual chunk entities, the bounding boxes entity should then display correctly.

[github-actions[bot]]

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[bot]]

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[bot]]

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[bot]]

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[bot]]

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[bot]]

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.

[github-actions[bot]]

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[bot]]

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[bot]]

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[bot]]

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.