Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3D] add support for gl_clipdistance + scene rotation (part 2) #58051

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

ptitjano
Copy link
Contributor

Description

This PR achieves the same result as #57593 but with the gl_clipDistance approach from #57899

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 general QgsOrientedBox3D is used to describe the limits of the scene. This allows to define a custom Z rotation. Then, the ClipPlane equations from #57899 are updated to take into account the rotation.

A new 3d extent widget is introduced. It allows to properly set a custom orientation

image

An example

image

image

This PR contains the commits from #57899

This is not used at the moment. It will be used in the following
commits to define the clip plane equations.
There is no need to define a special vertex shader code for this
material, it can use the default case. This way, it reduces the number
of vertex shader files.
This will be used by `QgsPhongMaterialSettings` in the next commit.
This commit makes two changes to `QgsPhongMaterialSettings`:
1. It merges `dataDefinedMaterial()` and `constantColorMaterial` into
one new function: `buildMaterial`.

2. The fragment shader files `phongDataDefined.frag` and
`phongConstant.frag` are merged into a new file `phong.frag`. This is
achieved by a `#define` logic. If `DATA_DEFINED` is defined, this is
the dataDefined case. Otherwise, this is the constant case.
This is similar to the previous QgsPhongMaterialSettings change:

This commit makes two changes to `QgsGoochMaterialSettings`:
1. It merges `dataDefinedMaterial()` and `constantColorMaterial` into
one new function: `buildMaterial`.

2. There is one fragment shader file called `gooch.frag`. This is
achieved by a `#define` logic. If `DATA_DEFINED` is defined, this is
the dataDefined case. Otherwise, this is the constant case.
There is no need to define a special vertex shader code for this
material, it can use the default case. This way, it reduces the number
of vertex shader files.
This is the same material as the qt3d one:
`Qt3DExtras::QTextureMaterial`. It will also be used in the following
commits to define the gl_ClipDistance for the textured terrain.
This is the same material as the qt3d one:
`Qt3DExtras::QgsMaskedTexturedMaterial`. It will also be used in the
following commits to define the gl_ClipDistance for the non texture
case of the terrain.
There is no functional change. It is simply needed to add
gl_ClipDistance support to the terrain shaders in the next commits.
`Qt3DRender::QClipPlane` enable OpenGL clipping plane that can be used
in shaders using gl_ClipDistance.

This change adds 4 clipPlane. This will be used to clip the 3dScene at
Xmin / Xmax / Ymin / Ymax. This change has no effect at the moment
because  `gl_ClipDistance` needs to be computed in each vertex or
geometry shader file. This will be achieved in the following commits.
@github-actions github-actions bot added this to the 3.40.0 milestone Jul 10, 2024
@ptitjano ptitjano self-assigned this Jul 10, 2024
@ptitjano ptitjano added Feature 3D Relates to QGIS' 3D engine or rendering labels Jul 10, 2024
@ptitjano ptitjano force-pushed the 3d-clip-part2-rotate branch 2 times, most recently from e24ff44 to 8ea3d0b Compare July 10, 2024 09:59
Copy link

github-actions bot commented Jul 10, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit f7b807c)

Copy link

github-actions bot commented Jul 10, 2024

Tests failed for Qt 6

One or more tests failed using the build from commit c5e2cfc

flat_terrain_filtered_3 (testFilteredFlatTerrain)

flat_terrain_filtered_3

Test failed at testFilteredFlatTerrain at tests/src/3d/testqgs3drendering.cpp:1489

Rendered image did not match tests/testdata/control_images/3d/expected_flat_terrain_filtered_3/expected_flat_terrain_filtered_3.png (found 17267 pixels different)

pointcloud_3d_filtered_scene_extent_rotation (testPointCloudFilteredSceneExtent)

pointcloud_3d_filtered_scene_extent_rotation

Test failed at testPointCloudFilteredSceneExtent at tests/src/3d/testqgspointcloud3drendering.cpp:522

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 ptitjano force-pushed the 3d-clip-part2-rotate branch 2 times, most recently from c5e2cfc to f7b807c Compare July 10, 2024 17:45
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.

The associated unit test needs to be updated because the objects at
the border of the scene are now partially cut.
All the existing parameters are stored in
`Qgs3DMapSettings`. Therefore, `Qgs3DMapSettings` can be directly
used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This one is a little bit to the other ones. Indeed, lines has a
geometry shader with redefine gl_Position. Therefore, gl_ClipDistance
needs to be computed in the geometry shader file.

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This is achieved with two changes:
1. define the plane equation as uniform with the
`Qgs3DUtils::addBoundingBoxParametersToEffect` call
2. include `clipplane.inc` file in the vertex shader file and call
setClipDistance()

With these changes, the planes defined in the framegraph are used.
This ensures to always have the correct extent used by the terrain.
There is no way to define the 3d scene Z extent. Use very high
parameters in that case to ensure that the scene will not be cut.
This is a direct copy-paste from QMatrix4x4.
This change introduces a new parameter `mBox` which stores the 3d
extent used limit the 3d view. `mExtent` is now computed from `mBox`
and stores the 2d horizontal bounding box of `mBox`.
`setExtent` now calls `setBox`. If the current box vertical component
is already set, if is kept, otherwise, the default min/max depth is
used.

By using a `QgsOrientedBox3D`, it is also possible to define a Z
rotation of the 3d scene extent. This rotation Returns the z rotation
is defined as clockwise from North.

The next commits will make it possible to define custom Z rotation.
The next commit will extend this widget to be able to define the Z
rotation.
This allows to define a Z rotation of the 3d scene. Rotation is
clockwise from North.
With this change, the 3D scene extent is updated as soon as one of the
parameter from the extent configuration dialog is changed.
@wonder-sk
Copy link
Member

The usability of the 3D extent widget worries me a lot - if I understand correctly, one needs to pick a 2D extent (axis aligned), and then set rotation afterwards? That would be IMHO quite tricky to properly pick some rotated area of interest.

I believe what would work better is some kind of 3D box (or 2D box in 2D map canvas) that can be moved around interactively with mouse in 3D or 2D canvas, expanded/collapsed and rotated - just like what CloudCompare or Potree offer...

@ptitjano
Copy link
Contributor Author

The usability of the 3D extent widget worries me a lot - if I understand correctly, one needs to pick a 2D extent (axis aligned), and then set rotation afterwards? That would be IMHO quite tricky to properly pick some rotated area of interest.

I believe what would work better is some kind of 3D box (or 2D box in 2D map canvas) that can be moved around interactively with mouse in 3D or 2D canvas, expanded/collapsed and rotated - just like what CloudCompare or Potree offer...

I agree that this would be a nice feature. However, the main use case for this rotation parameter is for people who already know the coordinates of the box. They won't draw a box on the 3D/2D canvas. They need a dialog box to enter Xmin/Xmax Ymin/Ymax and the rotation angle. This is exactly what this PR achieves.

At the moment, it is only possible to draw an axis aligned box on the 2d canvas. With this PR, there is no usability regression, it is still possible. I agree that this is tricky to draw a rotated area of interest. But on current master, it is impossible. Besides, when I first proposed it, the first comments noted that this was an edge case. Again, I'm trying to accommodate people who already know the coordinates of the area of interest and want to type them.

I have already made the following changes:

  • do some opengl clipping
  • store the coordinates in a QgsOrientedBox3D
  • completely rewrite the material logic with the introduction of a base class

I agreed to make all these changes because I truly believe they are good things and it would have been difficult to improve the situation with the previous approach.

The feature you are proposing will likely take months to develop. I would like to move forward. We can defer it to a future PR and discuss it later to have a solution we all agree on.

Thanks for all your comments and review work.
Also, I'm gonna be on vacation for the next two weeks. So don't be surprised if I don't answer.

@nyalldawson
Copy link
Collaborator

I agree that this would be a nice feature. However, the main use case for this rotation parameter is for people who already know the coordinates of the box. They won't draw a box on the 3D/2D canvas. They need a dialog box to enter Xmin/Xmax Ymin/Ymax and the rotation angle. This is exactly what this PR achieves.

For the record, I don't particularly like the new widget either. I don't think all the near-duplicate code is needed vs the existing extent widget, and I don't like that it's appearance is completely different from the other one.

Here's what I'd propose as a middle ground/way forward

  • Leave the API as is, with the rectangle extent + rotation property
  • Drop the custom extent+rotation widget, and consequently don't show the rotation property as a user-settable property by default
  • Make sure the rotation property is exposed to sip and accessible to plugins, and then write a small plugin which allows users to set the rotation clipping for 3d views

What do you think?

Copy link

github-actions bot commented Aug 7, 2024

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 github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3D Relates to QGIS' 3D engine or rendering Feature stale Uh oh! Seems this work is abandoned, and the PR is about to close.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants