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 (part 1) #57899

Merged
merged 26 commits into from
Oct 17, 2024

Conversation

ptitjano
Copy link
Contributor

@ptitjano ptitjano commented Jun 26, 2024

Description

This PR adds support for gl_clipdistance as suggested by @wonder-sk in #57593 (review)
This allows to properly cut objects which are outside of the 3d scene extent. Contrary to #57593 it does not contain the rotation part. It will be added in a following PR. This is still useful in the general case to propery cut the objects at the border of the scene:

without gl_clipdistance

cut_before

with gl_clipdistance

cut_after

The PR itself is quite simple. 3 changes are needed:

  • Add 4 Qt3DRender::QClipPlane in the framegraph to add gl_clipDistance support
    For each material:
    -call Qgs3DUtils::addBoundingBoxParametersToEffect to define the plane equation as uniform for the equation
  • compute gl_clipistance in each vertex or geometry shader by calling setClipDistance (see shaders/clipplane.inc)

The other changes provide some cleanup in the materials to limit the number of shader files and reduce the maintenance burden.

This should work for all the data sources (raster, vector, mesh, pointcloud, etc..)

Update October 8, 2024

This PR has been completely rewritten based on @wonder-sk feedback in #57899 (comment)

This PR introduces the necessary APIs to add support for clipping (gl_ClipDistance). The main logic is handled in Qgs3DMapScene:

- enableClipping() to enable opengl clipping
- disableClipping() to disable opengl clipping

The `enableClipping()` function achieves 3 things:
- it stores the clipping plane equations
- it adds clip planes to the framegraph
- it enables the clipping on the material of the existing entities

`disableClipping()` does the opposite things.

The clip planes equations need to be stored to handle the dynamic
addition of new entities. Indeed, when a new entity is added, clipping
needs to be enabled on its material if necessary. This is achieved
in `finalizeNewEntity` which is called on all new entities.
However, this is not true for the terrain entity. That's why,
`mTerrain` now listens to the `newEntityCreated` signal to call
`handleClippingOnEntity()`.

All the end user Material now inherit from a new QgsMaterial class. This allows to enable/disable on all the entities of a scene.

I have added unit tests to ensure that clipping works for all the existing types handled by QGIS 3D.

@github-actions github-actions bot added this to the 3.40.0 milestone Jun 26, 2024
@ptitjano ptitjano self-assigned this Jun 26, 2024
@ptitjano ptitjano added the 3D Relates to QGIS' 3D engine or rendering label Jun 26, 2024
Copy link

github-actions bot commented Jun 26, 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 0bb1e81)

Copy link

github-actions bot commented Jun 26, 2024

Tests failed for Qt 6

One or more tests failed using the build from commit 162308c

pointcloud_3d_singlecolor_clipping (testPointCloudSingleColorClipping)

pointcloud_3d_singlecolor_clipping

Test failed at testPointCloudSingleColorClipping at tests/src/3d/testqgspointcloud3drendering.cpp:319

Rendered image did not match tests/testdata/control_images/3d/expected_pointcloud_3d_singlecolor_clipping/expected_pointcloud_3d_singlecolor_clipping.png (found 10028 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.

Copy link

github-actions bot commented Jun 26, 2024

Tests failed for Qt 5

One or more tests failed using the build from commit b3e8f7e

pointcloud_3d_singlecolor (testPointCloudSingleColor)

pointcloud_3d_singlecolor

Test failed at testPointCloudSingleColor at tests/src/3d/testqgspointcloud3drendering.cpp:276

pointcloud_3d_colorramp (testPointCloudAttributeByRamp)

pointcloud_3d_colorramp

Test failed at testPointCloudAttributeByRamp at tests/src/3d/testqgspointcloud3drendering.cpp:316

pointcloud_3d_classification (testPointCloudClassification)

pointcloud_3d_classification

Test failed at testPointCloudClassification at tests/src/3d/testqgspointcloud3drendering.cpp:353

pointcloud_3d_classification_pointsizes (testPointCloudClassificationOverridePointSizes)

pointcloud_3d_classification_pointsizes

Test failed at testPointCloudClassificationOverridePointSizes at tests/src/3d/testqgspointcloud3drendering.cpp:392

pointcloud_3d_filtered_classification (testPointCloudFilteredClassification)

pointcloud_3d_filtered_classification

Test failed at testPointCloudFilteredClassification at tests/src/3d/testqgspointcloud3drendering.cpp:430

pointcloud_3d_filtered_scene_extent (testPointCloudFilteredSceneExtent)

pointcloud_3d_filtered_scene_extent

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

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-part1 branch 2 times, most recently from 656cf3c to 6718c15 Compare July 5, 2024 21:35
@ptitjano ptitjano changed the title [3D] add support for gl_clipdistance [3D] add support for gl_clipdistance (part 1) Jul 10, 2024
Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs minor comment changes, but LGTM.

src/3d/qgs3dutils.cpp Outdated Show resolved Hide resolved
src/3d/shaders/lines.geom Outdated Show resolved Hide resolved
src/3d/shaders/lines.geom Outdated Show resolved Hide resolved
@wonder-sk
Copy link
Member

hi @ptitjano - sorry for taking so long to review this PR (holiday season is in full swing!)

Thanks a lot for your efforts, going the route of clipping planes!

It would be great to split this kind of work into smaller bits, it would make the review easier (e.g. one PR for addition of new materials, one PR for phong shaders refactoring, one PR for addition of clipping planes).

As for the code changes:

  • I like that all materials we use are now contained in the QGIS code, and we can customize them further as needed
  • good to see the phong "constant" and "data-defined" shaders merged together using #ifdefs
  • addition of QgsDiffuseSpecularMaterial
    • why is it still discarding fragments if alpha is not 1.0 ? is it still needed?
    • my understanding is that it is now used only for terrain, but there are also very similar materials created in QgsPhongMaterialSettings and QgsPhongTexturedMaterialSettings - and I strongly feel those materials should be also covered by the newly created QgsDiffuseSpecularMaterial class, given they have pretty much the same use case
    • it is quite confusing in addition to the existing diffuseSpecular.frag, this material adds diffusespecular.frag with different implementation - this should be cleaned up
  • clipping planes implementation
    • clipping should be an optional feature, not something that is enabled all the time - materials should apply clipping only when it is requested
    • clipping should not be tied to extent() of Qgs3DMapSettings - that is really limiting to how it can be used. There should be a getter/setter in Qgs3DMapSettings to get/set clipping planes (in world coordinates), independent from the extent() - whenever extent() gets changed, we need to do reloading of data, but we should be able to modify clipping planes just by a much "lighter" approach, where the existing materials will only update their uniforms, without heavy rebuilding of the whole scene.
    • I agree with Nyall that the introduction of Qgs3DMapSettings in toMaterial() is not a good design choice. In addition to the worry about use of QObject in threads:
      • materials should not need to know about the whole 3d map settings
      • materials should not have clipping planes fixed at the time the material is created - that should be configurable during the lifetime of the material object (just like e.g. diffuse color property of a material)
      • addition of it heavily pollutes many places in the 3D code and adds access to 3D map settings where it should not be available
    • what would make sense to me is that:
      1. there would be some kind of base material class derived from QMaterial, that adds function like setClippingPlanes(QVector planes)
      2. all "our" materials derive from it, and respect the clipping planes
      3. at any time, material's clipping planes could be changed, to update the clipping setup
      4. there is some convenience utility function e.g. in Qgs3DUtils that can generate clipping planes (e.g. take a 2D/3D extent and return the planes)

I think the Phong material implementation needs to be cleaned up and clipping planes implementation needs to be updated before we can merge this...

Copy link

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 Jul 30, 2024
@ptitjano ptitjano removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 5, 2024
Copy link

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 20, 2024
@ptitjano ptitjano removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 20, 2024
Copy link

github-actions bot commented Sep 4, 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 Sep 4, 2024
@ptitjano ptitjano removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 4, 2024
@nyalldawson nyalldawson added the Freeze Exempt Feature Freeze exemption granted label Sep 13, 2024
This is achieved by calling `setClipDistance` in the vertex shader if
the clipping is enabled.
This is achieved by calling `setClipDistance` in the geometry shader
if the clipping is enabled.
This adds two functions to setup clip planes on a 3d scene:
- enableClipping() to enable opengl clipping
- disableClipping() to disable opengl clipping

The `enableClipping()` function achieves 3 things:
- it stores the clipping plane equations
- it adds clip planes to the framegraph
- it enables the clipping on the material of the existing entities

`disableClipping()` does the opposite things.

The clip planes equations need to be stored to handle the dynamic
addition of new entities. Indeed, when a new entity is added, clipping
needs to be enabled on its material if necessary. This is achieved
in `finalizeNewEntity` which is called on all new entities.
However, this is not true for the terrain entity. That's why,
`mTerrain` now listens to the `newEntityCreated` signal to call
`handleClippingOnEntity()`.
@ptitjano
Copy link
Contributor Author

ptitjano commented Oct 11, 2024

Please can I ask not to force-push commits during review? That makes it impossible to properly see what exactly has changed since the last round of review, and makes things more frustrating for reviewer...

I'm used to projects where force push during review is considered a good practice. So, it has become a second nature.
Also, based on QGIS git history, one can see that most of the time the merged PRs are either:

  • squashed into one commit
  • the "review commits" are kept

In the end, we lose most of the benefits of git tooling :

  • It the PR has been squashed, it's impossible to do a revert of a subpart of a change if there is an issue. It's also impossible to study the commit because the message contains all the previous individuals messages and there are so many changes that it becomes practically impossible to understand the individual changes
  • when the "review commits" are kept, it's also impossible to do a proper revert. Git blame information is lost. Also, it's impossible to study an individual commit because the information is split between the original and "the review commit".

I also think that github is well suited to handle review with force pushes. From the "Files Changed" tab, you can select "changes since your last review" which creates the same diffs than if there had not been no force push

@wonder-sk
Copy link
Member

I also think that github is well suited to handle review with force pushes. From the "Files Changed" tab, you can select "changes since your last review" which creates the same diffs than if there had not been no force push

Nope... if I pick "changes since your last review", I get this because of the force push:

image

The best github can give me is to unmark changed files as "viewed", but good luck re-reviewing files containing many changes...

There's no way of knowing what has changed after a force push unless I would be keeping a local copy of the branch...

@nyalldawson
Copy link
Collaborator

@ptitjano we're all well aware there's a review people power problem here, let's not push for non-crucial development practices which make it in ANY way harder to review 🤣👍

@ptitjano
Copy link
Contributor Author

I also think that github is well suited to handle review with force pushes. From the "Files Changed" tab, you can select "changes since your last review" which creates the same diffs than if there had not been no force push

Nope... if I pick "changes since your last review", I get this because of the force push:

image

The best github can give me is to unmark changed files as "viewed", but good luck re-reviewing files containing many changes...

There's no way of knowing what has changed after a force push unless I would be keeping a local copy of the branch...

My bad. Sorry for the inconvenience. I have never encountered this issue while reviewing PR on other github projects even when force pushed.

@ptitjano
Copy link
Contributor Author

@ptitjano we're all well aware there's a review people power problem here, let's not push for non-crucial development practices which make it in ANY way harder to review 🤣👍

I really don't want to derail this PR or write a rant... And I really thank you and @wonder-sk for all the time you spend reviewing. But as I tried to explain earlier, merged PR which contain "review commits" we lost most of the benefit of git: revert/ history/blame because the commit are not atomic anymore. And unfortunately, this happens often. QGIS has a really big code base and I'm still quite new at this thing. So, I spend a lot of times looking at the history to study the commits. Unfortunately, this is sometimes really hard and frustrating (I really mean it) because I sometimes study one commit for a long time, it does not seem to make sense, and a few commits later I see a commit named something like 'fix based on review' and I realize that if "fixes" the commit that I did not understand. It's not always easy to understand how QGIS internal work and if you can't use the commits, it makes the task much harder.
So yes, the main reason for my force-push are to keep atomic commits. I did not realize it made the reviews that much harder. But, I really think that, we, as a community, should always rebase before merging because git history if for ever. It can not be undone.

@nyalldawson
Copy link
Collaborator

@ptitjano

My personal thoughts on "best practice" would be:

  1. if a pr has never been reviewed, go ahead and force push and clean up and rebase
  2. if it's mid review, just add new commits until the review process is finished
    3a. Either tag with squash for a final squash merge, or
    3b. cleanup the history and force push ONLY right before merge.

3a works if it makes sense for a single commit to represent the whole change set. 3b works otherwise, but isn't ideal from a security point of view (the force pushed commits could potentially contain anything, and won't be rereviewed before merging). There's no "perfect" approach here 😝. (That said -- You'll see everyone bends the rules at times and will merge messy things with commits with eg GitHub generated names after applying suggestions via the web, just because the benefit of a super clean history doesn't always outweigh the pain of cleanup/force push/wait hours for ci to finish.)

I agree with @wonder-sk in that force pushed mid review make things really tricky, and force the reviewer to waste time rereading through all the old discussions and manually checking that everything's been addressed. It's slow and frustrating as a result.

The worst option (in my view) is merge commits containing the full history. Those really mess up the git history and break blame/bisect/... etc. 🤮.

@wonder-sk
Copy link
Member

But as I tried to explain earlier, merged PR which contain "review commits" we lost most of the benefit of git: revert/ history/blame because the commit are not atomic anymore

I would disagree here, but in the end, it is a matter of personal preference what exactly an atomic commit should be. My personal preference is to have even bigger features all done in one "atomic" commit - at least it is easy to see what are all the ingredients. And "review commits" never bothered me really, so it is surprising to me they may be frustrating for others. After all, when doing git blame or looking into git history, one typically needs to look at the original PR anyway to get better context, so it does not matter if the work is done in one, two or ten commits...

Agreed with the suggested "best practice" from @nyalldawson 👍

@ptitjano
Copy link
Contributor Author

But as I tried to explain earlier, merged PR which contain "review commits" we lost most of the benefit of git: revert/ history/blame because the commit are not atomic anymore

I would disagree here, but in the end, it is a matter of personal preference what exactly an atomic commit should be. My personal preference is to have even bigger features all done in one "atomic" commit - at least it is easy to see what are all the ingredients. And "review commits" never bothered me really, so it is surprising to me they may be frustrating for others. After all, when doing git blame or looking into git history, one typically needs to look at the original PR anyway to get better context, so it does not matter if the work is done in one, two or ten commits...

I guess we will agree that we disagree on this point. Anyway, I will try to follow the recommendations of @nyalldawson

This PR should be ready now.

Copy link
Member

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Looks good to me now - thanks for your efforts!

@wonder-sk
Copy link
Member

@ptitjano The PR is freeze exempt - would you like to get it merged now, or shall we merge it after the 3.40 release?

@nyalldawson nyalldawson added Frozen Feature freeze - Do not merge! Freeze Exempt Feature Freeze exemption granted and removed Freeze Exempt Feature Freeze exemption granted Frozen Feature freeze - Do not merge! labels Oct 17, 2024
@ptitjano
Copy link
Contributor Author

@ptitjano The PR is freeze exempt - would you like to get it merged now, or shall we merge it after the 3.40 release?

@ptitjano The PR is freeze exempt - would you like to get it merged now, or shall we merge it after the 3.40 release?

I would like to get it merged now please.

@wonder-sk wonder-sk merged commit 444c41b into qgis:master Oct 17, 2024
34 checks passed
@zacharlie zacharlie added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Oct 19, 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 ChangelogHarvested This PR description has been harvested in the Changelog already. Feature Freeze Exempt Feature Freeze exemption granted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants