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

Don't identify 3d layers when clicking the terrain #59790

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

Conversation

uclaros
Copy link
Contributor

@uclaros uclaros commented Dec 9, 2024

Description

Layers that have a 3d renderer set should only be identified by the 3d ray intersection test and not by the 2d identify tool on the point clicked on the terrain.

@uclaros uclaros added 3D Relates to QGIS' 3D engine or rendering Bug Either a bug report, or a bug fix. Let's hope for the latter! labels Dec 9, 2024
@github-actions github-actions bot added this to the 3.42.0 milestone Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

🪟 Windows Qt6 builds

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

🪟 Windows builds

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

* \see restoreCanvasPropertiesOverrides()
* \since QGIS 3.4
*/
void setCanvasPropertiesOverrides( double searchRadiusMapUnits );
void setCanvasPropertiesOverrides( double searchRadiusMapUnits, bool skip3DLayers = true );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a bool argument, could you use a flag enum instead? That's a little more future proof and avoids the risk of multiple true, false, true style arguments (and also the risk of accidental automatic int/double to bool conversion if arguments are specified in the wrong order)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future proofness, wouldn't it be better to replace parameters with a variant map, or better a simple CanvasPropertiesOverrides struct? The flag can only handle bool-ish parameters

Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be better to replace parameters with a variant map, or better a simple CanvasPropertiesOverrides struct? The flag can only handle bool-ish parameters

Not a variant map, cos that's extremely fragile -- but yes, a CanvasPropertiesOverrides struct would be even better. It's just more work vs a enum :)

@uclaros uclaros force-pushed the fix-identify-3d-on-terrain branch from 77b3dfb to 9c5900a Compare December 11, 2024 13:45
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 Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants