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

Virtual point clouds - Tile Labels #59726

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

Conversation

Withalion
Copy link
Contributor

Description

In this PR we add the option to show labels when working with virtual point clouds. The labels even if set are only shown when the bounding boxes of point clouds are visible and if they fit inside. Users can also modify the format of the label text via QgsTextFormatButton next to it.

image

Funded by: Klimadatastyrelsen

@github-actions github-actions bot added this to the 3.42.0 milestone Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

src/core/pointcloud/qgspointcloudrenderer.h Outdated Show resolved Hide resolved
src/core/pointcloud/qgspointcloudrenderer.h Outdated Show resolved Hide resolved
src/core/pointcloud/qgspointcloudrenderer.h Outdated Show resolved Hide resolved
src/core/pointcloud/qgspointcloudextentrenderer.cpp Outdated Show resolved Hide resolved
@wonder-sk wonder-sk changed the title Virtual point clouds - Tile Lables Virtual point clouds - Tile Labels Dec 5, 2024
@Withalion Withalion force-pushed the vpc-labels branch 2 times, most recently from 6245123 to 6b7b611 Compare December 5, 2024 13:34
src/core/pointcloud/qgspointcloudextentrenderer.h Outdated Show resolved Hide resolved
src/core/pointcloud/qgspointcloudextentrenderer.cpp Outdated Show resolved Hide resolved
// set some better defaults for label format
QgsTextFormat textFormat = mLayer->renderer()->labelTextFormat();
QgsTextBufferSettings settings;
settings.setEnabled( true );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this mean it's impossible to remove the buffer?

I think instead this should be done in the QgsPointCloudLayerRenderer constructor, so that users CAN manually remove the buffer if they don't want it.

Also the format should default to QgsStyle::defaultStyle()->defaultTextFormat() (again, in QgsPointCloudLayerRenderer constructor)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users are always able to change all the text settings.
These options should just represent some reasonable options which are set when the layer is loaded.

I changed the default textFormat to use QgsStyle::defaultStyle()->defaultTextFormat(). However I think it's better to keep the settings here instead of QgsPointCloudLayerRenderer constructor as the preset is more oriented towards the text format widget which is the source of truth not the renderer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Users are always able to change all the text settings.

That's not the case -- as soon as they open the layer in this widget, it will reset the format to apply a buffer, even if they'd previously unchecked that!

In any case it's bad practice to set defaults in a widget -- the behavior will be fragile, and eg won't apply if you're creating layers directly via API from a headless environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh okay, I think I understand what you mean now. It's funny that I tried it now and the setting gets reapplied as it is visible in the text format widget, however the actual rendered labels don't change :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants