[3D] Add support for a rotated 3d scene #57593
Conversation
ptitjano
force-pushed
the
3d-rotate-z-simple
branch
2 times, most recently
from
47f882f to
fe147a5
Compare
ptitjano
force-pushed
the
3d-rotate-z-simple
branch
4 times, most recently
from
7b99d22 to
c296a84
Compare
Tests failed for Qt 5One or more tests failed using the build from commit c296a84 mesh_terrain_1 (testMeshTerrain)mesh_terrain_1Test failed at testMeshTerrain at tests/src/3d/testqgsmesh3drendering.cpp:142 Rendered image did not match tests/testdata/control_images/3d/expected_mesh_terrain_1/expected_mesh_terrain_1.png (found 93750 pixels different) mesh3d (testMesh)mesh3dTest failed at testMesh at tests/src/3d/testqgsmesh3drendering.cpp:175 Rendered image did not match tests/testdata/control_images/3d/expected_mesh3d/expected_mesh3d.png (found 55520 pixels different) mesh3dOnFace (testMesh_datasetOnFaces)mesh3dOnFaceTest failed at testMesh_datasetOnFaces at tests/src/3d/testqgsmesh3drendering.cpp:217 Rendered image did not match tests/testdata/control_images/3d/expected_mesh3dOnFace/expected_mesh3dOnFace.png (found 48581 pixels different) mesh3d_filtered (testFilteredMesh)mesh3d_filteredTest failed at testFilteredMesh at tests/src/3d/testqgsmesh3drendering.cpp:331 Rendered image did not match tests/testdata/control_images/3d/expected_mesh3d_filtered/expected_mesh3d_filtered.png (found 35675 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
Tests failed for Qt 6One or more tests failed using the build from commit 0331341 line_rendering_2 (testLineRendering)line_rendering_2Test failed at testLineRendering at tests/src/3d/testqgs3drendering.cpp:742 Rendered image did not match tests/testdata/control_images/3d/expected_line_rendering_2/expected_line_rendering_2.png (found 43 pixels different) pointcloud_3d_filtered_scene_extent_rotation (testPointCloudFilteredSceneExtent)pointcloud_3d_filtered_scene_extent_rotationTest failed at testPointCloudFilteredSceneExtent at tests/src/3d/testqgspointcloud3drendering.cpp:521 Rendered image did not match tests/testdata/control_images/3d/expected_pointcloud_3d_filtered_scene_extent_rotation/expected_pointcloud_3d_filtered_scene_extent_rotation.png (found 1615 pixels different) The full test report (included comparison of rendered vs expected images) can be found here. Further documentation on the QGIS test infrastructure can be found in the Developer's Guide. |
|
@ptitjano this was just a partial review, I thought I'd post some early feedback to get things started 👍 |
|
Also @nirvn got any ideas on the new extent widget? I think it'll need a few tweaks to get the same level of UX polish as the existing one. |
ptitjano
force-pushed
the
3d-rotate-z-simple
branch
4 times, most recently
from
85f0a39 to
bf75548
Compare
This change introduces a new parameter `mZRotation` which stores the rotation of a 3d scene in degrees clockwise from North. It is already possible to limit the extent of a 3d scene. However, the 3d scene is always aligned the true north. This change will make it possible to define a Z rotation of the 3d scene extent. There are now 4 stored parameters to fully describe the 3d scene extent: - QgsRectangle `mExtent`: axis aligned extent - double `mZRotation`: Z rotation of the 3d scene - QgsGeometry `mRotatedExtent`: the rotated `mExtent` - QgsRectangle `mRotatedExtentBBox`: the bounding box of `mRotatedExtent` If there is no rotation, `mExtent`, `mRotatedExtent` and `mRotatedExtentBBox` are the same. `Qgs3DMapSettings::rotatedExtent()` returns a geometry of the rotated extent. `Qgs3DMapSettings::extent()` now returns the bounding box of the rotated extent. This parameter is not used at the moment. The next commits will update the loaders and widgets to take it into account.
… layer This is the same logic between `QgsRuleBasedChunkLoader` and `QgsVectorLayerChunkLoader`.
|
@ptitjano I've had another look over this, and would you mind splitting this into separate modular PRs? Eg I have big concerns about the changes made here to the point cloud request, and would prefer to discuss those in isolation. |
Because of the z rotation of the 3d scene, the extent of the node bounding box is not aligned with the 3d scene extent. Therefore, the request to get the geometry is no longer a rectangle (the extent of the tile). The intersection with the 3D scene also needs to be taken into account. To avoid any performance penalty a first request is done on the bounding box of the intersection of the tile extent and the scene extent. Then, for all the found features, discard them which do not intersect the exact extent.
This material on textures discards all the transparent pixels. It will be used in the following commits to handle the rotation of the terrain.
This will also be used by a new material in the next commit.
This material is similar to `QgsMaskedTexturedMaterial`. It discards all the transparent pixels. It will also be used in the following commit to handle the rotation of the terrain.
It has no effect at the moment because a terrain texture is always opaque. This will be used in the next commit because the pixels outside the intersection with the scene extent will be transparent.
Because of the z rotation of the 3d scene, the generated texture is no longer necessarily a rectangle. To correctly handle it, 2 changes are needed: 1. Compute the intersection between the tile extent and the 3D scene. Then, a `QPainterPath` is used to render only the pixels inside the intersection. 2. Outside the intersection, the pixels are transparent.
The `setFilterRect` allows to filter a pointcloud request by a rectangular extent but it does not allow to set any geometry. The change to `setFilterGeometry` allows to specify any geometry. The change in the decoder are minimal. They still check is inside the filter geometry. If the requested filter is a QgsRectangle, it can be transformed into a Geometry by using `QgsGeometry::fromRect`.
The filter Geometry used to request the pointcloud data is now the intersection between the oriented 3d scene extent and the render context extent.
The next commit will extend this widget to be able to define the Z rotation.
With this change, the 3D scene extent is updated as soon as one of the parameter from the extent configuration dialog is changed.
ptitjano
force-pushed
the
3d-rotate-z-simple
branch
from
bf75548 to
105b406
Compare
There was a problem hiding this comment.
I am afraid this is -1 for me.
This kind of functionality should be really implemented on GPU using clipping planes. OpenGL has a nice builtin functionality for that with gl_ClipDistance (and enabled by Qt3DRender::QClipPlane) that even allows one to do clipping in the vertex shader (!), thus saving a lot of processing time because fragment shader does not even need to be called for clipped parts of geometries (e.g. in WebGL this is not available and clipping has to be done in fragment shaders by discarding fragments). This is also a good intro to clipping planes: https://www.youtube.com/watch?v=dcfj5PctptE
If the clipping is done on GPU, this also means that any change to the clipping configuration is a simple update of some OpenGL uniforms, rather than expensive reload of all entities.
With gl_ClipDistance, one would only need small updates in the existing materials to respect clipping planes we would define.
Some more general comments about the current implementation:
-
zRotation() - initially I thought all coordinates in the scene were getting rotated by this angle - but in fact the coordinates stay the same (axis aligned with X,Y coordinates of the map projection), just some tricks are applied to clip the scene to make it look like it is rotated
-
zRotation() - we (Lutra + North Road) will be hopefully soon adding support to render 3D scene as a globe. In that kind of setting, having a Z-rotation does not really make much sense. If we want a "rotated" scene, we should use some better way to define the clipping area - e.g. a oriented bounding box.
-
it seems like unreasonable complication in the chunk loaders to have them all consider the rotated extent (passed as a polygon), especially all the changes in the point cloud infrastructure - as already pointed out by Nyall in 3D] Add support for a rotated 3d scene - pointcloud support #57618. In addition to that, mesh layer does not seem to be covered?
-
terrain texture generator + addition of two "masked" materials - this also adds a lot of complexity that would be otherwise not needed with clipping planes approach.
So, to sum up, I think the better and much simpler approach would be to:
- use oriented bounding box to set up clipping for a rotated scene
- leave chunk loading and point cloud infrastructure intact
- update materials to use OpenGL clipping planes
I totally agree with you and this now done in #57899. To be completely honest I wanted to do something similar in the first place. I did not do it because when we (@benoitdm-oslandia and I) first told you about it two years ago, you answered us that this was a bad idea because we would need to change all the shader files. This would be too hard to maintain. Basically, you discouraged us. I did not agree with the answer but I respected it I even showed you a first draft of this PR during the QGIS contributor meeting in s-Hertogenbosch. During this meeting, I also explained that it was very hard to add support for the terrain. You never mentioned the shader approach during this meeting. I ended up with the transparent approach which is, I think, a nice hack to solve an issue which should not occur in the first place. In summary, this is not like this PR is some secret work which comes out of nowhere. I am very disappointed this ends up in a situation like this. This made me waste a lot of time. I hope we can do better in the future. I look forward to it |
|
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
|
github-actions
bot
added
the
stale
|
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. |
Description
It is possible to limit the extent of a 3d view. However, the extents is always axis aligned: it is aligned with the true north. This PR adds the possibility to define a rotation angle around the Z axis. This way, the previously axis aligned extent can now have an arbitrary azimuth.
Previously, it was possible to fully describe the extent with a
QgsRectangle. Now, a more generalQgsGeometryneeds to be used. It has the following consequences onQgs3DMapSettings:This works for all the different kind of layers. The change on the 3d loaders is simple. Most of the time, it's enough to update the feature request to take into account the orientation.
However, the changes on the terrain are much more important. Indeed, the texture of a terrain tile is supposed to be rectangular. This is not the case anymore for a tile on the border and an arbitrary rotation. In order to solve this issue, the idea is to no draw anything for those pixels outside of an rotated extent. This means that those are pixels are transparent. Then, a
QPainterPathis used to render only the pixels inside the extent. New materials need to be introduced to handle those transparent pixels. They are derived for the usual layers but their fragment shader discard the transparent pixels to not display them and avoid any transparent issue.Finally, a new 3d extent widget is introduced. It allows to properly set a custom orientation
An example: