Skip to content

Comments

qgsmaterial: Fix removal of parameters when clipping is disabled#61135

Merged
nyalldawson merged 5 commits intoqgis:masterfrom
ptitjano:material-clipping-fix-removal-parameter
Mar 23, 2025
Merged

qgsmaterial: Fix removal of parameters when clipping is disabled#61135
nyalldawson merged 5 commits intoqgis:masterfrom
ptitjano:material-clipping-fix-removal-parameter

Conversation

@ptitjano
Copy link
Collaborator

@ptitjano ptitjano commented Mar 21, 2025

Description

Plane clipping uses two QParameters named
CLIP_PLANE_ARRAY_PARAMETER_NAME and
CLIP_PLANE_MAX_PLANE_PARAMETER_NAME. When clipping is disabled both
parameters need to be removed to prevent side effects on the shader
code. However, the loops which iterates through the parameters breaks
as soon one parameter is found. This means that the second parameter
is never removed.

This issue is fixed by removing the break condition. This way, both
parameters are always removed from the material.

The first commit is factored out from #61057 for backport.

Withalion and others added 3 commits March 21, 2025 15:35
Plane clipping uses two `QParameters` named
`CLIP_PLANE_ARRAY_PARAMETER_NAME` and
`CLIP_PLANE_MAX_PLANE_PARAMETER_NAME`. When clipping is disabled both
parameters need to be removed to prevent side effects on the shader
code. However, the loops which iterates through the parameters breaks
as soon one parameter is found. This means that the second parameter
is never removed.

This issue is fixed by removing the `break` condition. This way, both
parameters are always removed from the material.
This will make it easier to test enabling/disabling clipping multiple
times with the other reference image.
@ptitjano ptitjano added Bug Either a bug report, or a bug fix. Let's hope for the latter! backport queued_ltr_backports Queued Backports backport release-3_42 labels Mar 21, 2025
@ptitjano ptitjano self-assigned this Mar 21, 2025
@github-actions github-actions bot added this to the 3.44.0 milestone Mar 21, 2025
@ptitjano ptitjano added the 3D Relates to QGIS' 3D engine or rendering label Mar 21, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Mar 21, 2025

🪟 Windows Qt6 builds

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

🪟 Windows builds

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

This tests that enabling and disabling clipping multiple times works.
This tests that enabling and disabling clipping multiple times works.
@ptitjano ptitjano force-pushed the material-clipping-fix-removal-parameter branch from 728b1c1 to f00a644 Compare March 21, 2025 18:39
@nyalldawson nyalldawson merged commit 2d480db into qgis:master Mar 23, 2025
31 checks passed
@qgis-bot
Copy link
Collaborator

The backport to queued_ltr_backports failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply ce697de56fa... testqgs3drendering: Disable opacity for the polygons clipping test
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config set advice.mergeConflict false"

stdout
[backport-61135-to-queued_ltr_backports 58eec0d0594] qgsmaterial: Fix removal of parameters when clipping is disabled
 Author: Matej Bagar <matej.bagar@lutraconsulting.co.uk>
 Date: Fri Mar 21 15:27:27 2025 +0100
 1 file changed, 1 deletion(-)
[backport-61135-to-queued_ltr_backports b0429c837ca] testqgs3dmaterial: Add tests for clipping
 Author: Jean Felder <jean.felder@oslandia.com>
 Date: Fri Mar 21 17:42:47 2025 +0100
 2 files changed, 145 insertions(+)
Auto-merging tests/src/3d/testqgs3drendering.cpp
warning: Cannot merge binary files: tests/testdata/control_images/3d/expected_polygon3d_extrusion_clipping/expected_polygon3d_extrusion_clipping.png (HEAD vs. ce697de56fa (testqgs3drendering: Disable opacity for the polygons clipping test))
Auto-merging tests/testdata/control_images/3d/expected_polygon3d_extrusion_clipping/expected_polygon3d_extrusion_clipping.png
CONFLICT (content): Merge conflict in tests/testdata/control_images/3d/expected_polygon3d_extrusion_clipping/expected_polygon3d_extrusion_clipping.png

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-queued_ltr_backports queued_ltr_backports
# Navigate to the new working tree
cd .worktrees/backport-queued_ltr_backports
# Create a new branch
git switch --create backport-61135-to-queued_ltr_backports
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick b2c158d3aa19243435a74342ff610d8bf94fe10e,d1b62f478b9b386e6ad7b675fa10ad73d1261a4b,ce697de56faa5e0c77eca16cb76424d738d51af6,dc0ecc26387afeeb2abe2d37794c592b83573580,f00a644d51244daae50c478a43d0e3ef2d5d5f99
# Push it to GitHub
git push --set-upstream origin backport-61135-to-queued_ltr_backports
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-queued_ltr_backports

Then, create a pull request where the base branch is queued_ltr_backports and the compare/head branch is backport-61135-to-queued_ltr_backports.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Mar 23, 2025
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 backport queued_ltr_backports Queued Backports Bug Either a bug report, or a bug fix. Let's hope for the latter! failed backport The automated backport attempt failed, needs a manual backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants