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

Add configurable extent buffer to symbols #59630

Merged

Conversation

JuhoErvasti
Copy link
Contributor

@JuhoErvasti JuhoErvasti commented Nov 28, 2024

Fixes #38579.

Allows the user to set an extent buffer per symbol. Useful with for example geometry generator symbols:

2024-12-10.09-58-33.mp4

You can set the value in different units and it can also be controlled by an expression (for example you can set a different value based on the map scale).

When rendering QgsRenderer finds the biggest buffer value that has been set in all of its symbols. The extent for the feature request in QgsVectorLayerRenderer is then modified according to that value.

Backport to 3.40 requested (if possible).

Funded by the National Land Survey of Finland.

@github-actions github-actions bot added this to the 3.42.0 milestone Nov 28, 2024
Copy link

github-actions bot commented Nov 28, 2024

🪟 Windows builds

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

🪟 Windows Qt6 builds

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

Copy link

Tests failed for Qt 6

One or more tests failed using the build from commit 371ee87

buffer_extent_zero

buffer_extent_zero

Test failed at testRenderWithExtentBuffer at tests/src/python/test_qgsvectorlayerrenderer.py:848

Rendered image did not match tests/testdata/control_images/vectorlayerrenderer/expected_buffer_extent_zero/expected_buffer_extent_zero.png (found 54 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.

@nyalldawson
Copy link
Collaborator

@JuhoErvasti thanks for tackling this annoying problem!

I have a few thoughts on the approach used here:

  • I don't think this needs to be a symbol-layer property, rather it makes more sense to be a setting on a whole symbol itself. What's the pressing use case where one symbol layer would need a different size to the rest of the symbol? In my opinion doing this at the symbol layer considerably complicates both the code + the user experience.
  • If we made this a symbol setting, then the setting could be moved to sit inside the symbol "advanced" menu. This would help avoid the UI clutter that results from adding a rarely-changed, advanced setting directly inside ALL symbol layer property windows. (There's also a side issue caused by adding this spin box in the horizontal line with the existing widgets, as it considerably increases the minimum width for the panel, which will cause issues on smaller screen sizes)
  • What's the use case for data-definable buffers? I can't see how we'd ever need to set this on a per-feature basis. And right now this could ONLY be used to decrease the buffer size from the fixed amount, which sounds like an incredibly esoteric requirement. If there's a need to set data-definable buffer size eg to handle logic with different map scales/crs/..., then it should be evaluated ONCE for the layer and not on a per-feature basis (and make sure that the widget for it doesn't expose feature related context variables)

@JuhoErvasti
Copy link
Contributor Author

@nyalldawson

I don't think this needs to be a symbol-layer property

I agree that making it a symbol layer property does complicate things and the use cases for separate buffer levels are probably few and far between. The reason I did it this way was based on your comment from the issue's discussion:

I'm +1 for a configurable buffer parameter, but it needs to be done at the symbol layer level.

I interpreted this to mean that it does need to be a symbol layer property, but reading the thread again you might've meant it as a symbol property (as opposed to layer/project property as suggested by others)? Either way if that isn't the requirement, I'm happy to change it to be on the symbol level.

If we made this a symbol setting, then the setting could be moved to sit inside the symbol "advanced" menu.

Sounds good.

What's the use case for data-definable buffers?

I figured it'd be good to be able to control the buffer with an expression (for example to make it scale-dependant as you mentioned) and since I was adding it as a symbol layer property it made sense to use a data-defined button since almost everything else has one. But if the location of the property is changed, this can of course be changed too.

Could it be even just a normal expression widget, not a data-defined override button?

If there's a need to set data-definable buffer size eg to handle logic with different map scales/crs/..., then it should be evaluated ONCE for the layer

If it's done on the symbol level, how should this work? If you're using f.e. the graduated renderer you could set a different expression/value for each symbol, same with the geometry generator subsymbols. Wouldn't it have to be evaluated at least once per each symbol in the renderer?

@nyalldawson
Copy link
Collaborator

@JuhoErvasti

I agree that making it a symbol layer property does complicate things and the use cases for separate buffer levels are probably few and far between. The reason I did it this way was based on your #38579 (comment) from the issue's discussion:

Sorry, must have been an accidental mis-phrasing at the time. I think I was referring to "should be at the symbol level" as opposed to "should be one setting at the project (or layer) level", but accidentally wrote "symbol layer".

I interpreted this to mean that it does need to be a symbol layer property, but reading the thread again you might've meant it as a symbol property (as opposed to layer/project property as suggested by others)? Either way if that isn't the requirement, I'm happy to change it to be on the symbol level.

I figured it'd be good to be able to control the buffer with an expression (for example to make it scale-dependant as you mentioned) and since I was adding it as a symbol layer property it made sense to use a data-defined button since almost everything else has one. But if the location of the property is changed, this can of course be changed too.

Could it be even just a normal expression widget, not a data-defined override button?

Elsewhere we do use data-defined buttons for layer-level rendering properties. Eg for raster layer opacity you can data-define, even though it's only evaluated once just before the layer render. I'd say this could be similar -- we could evaluate once upfront at the start of the vector layer renderer, and then just use that to control the feature request extent. Nice and simple and we then avoid ALL handling when doing per-symbol-rendering.

If it's done on the symbol level, how should this work? If you're using f.e. the graduated renderer you could set a different expression/value for each symbol, same with the geometry generator subsymbols. Wouldn't it have to be evaluated at least once per each symbol in the renderer?

For multi-symbol renderers I'd do it once per symbol upfront, and then take the max and that's the value used for the feature request. The worst case scenario there is that we end up rendering a few extra symbols which fall totally outside the map.

@JuhoErvasti
Copy link
Contributor Author

@nyalldawson

If I could ask for one more clarification:

I'd say this could be similar -- we could evaluate once upfront at the start of the vector layer renderer, and then just use that to control the feature request extent. Nice and simple and we then avoid ALL handling when doing per-symbol-rendering.

For multi-symbol renderers I'd do it once per symbol upfront, and then take the max and that's the value used for the feature request.

I might be missing something, but if the extent buffer should only be applied to the feature request and there shouldn't be any further handling what is the benefit of having separate values for different symbols? Doesn't that make it effectively a renderer-level property even if the values are set for the symbols?

@nyalldawson
Copy link
Collaborator

@JuhoErvasti good question!

My thinking is that this property is innately associated with the shape and size of a particular symbol, so should be attached to that. That way, the property will be stored inside the symbol if you eg copy it to another layer or save to the style library and reuse the symbol in a different layer, and won't require re-configuring the renderer setting each time.

@JuhoErvasti JuhoErvasti changed the title Add configurable extent buffer to symbol layers Add configurable extent buffer to symbols Dec 3, 2024
@JuhoErvasti JuhoErvasti force-pushed the fix_38579_symbol_rendering_out_of_extent branch 2 times, most recently from 5753e9c to 53c25ac Compare December 4, 2024 09:59
@JuhoErvasti
Copy link
Contributor Author

JuhoErvasti commented Dec 4, 2024

@nyalldawson

I've reworked this so that the extent buffer is a symbol property now if you want to take a look.

@nyalldawson
Copy link
Collaborator

Thanks @JuhoErvasti !

One other consideration raised by @nirvn -- shouldn't we also add a unit selection for this setting? It would be more portable between different layers if the user could set the buffer size in eg millimeters instead of just map units (which may differ in size from layer to layer).

QgsRenderContext.convertToMapUnits could be used to convert the size + unit to a map unit size when required.

@JuhoErvasti JuhoErvasti force-pushed the fix_38579_symbol_rendering_out_of_extent branch from 70ef57c to 1a2fdd0 Compare December 5, 2024 17:45
@JuhoErvasti
Copy link
Contributor Author

@nyalldawson

Thank you for the review! I've made the changes you suggested. I ended up dropping negative buffers since I didn't really have a clear use case for the in mind. I originally thought that I might as well allow it in case someone has an obscure use case for them but as you pointed out now that the extent buffer isn't in the symbol layer it wouldn't work that well.

I also added the option to change the size unit.

/**
* Returns the units for the buffer size.
*
* \see setExtentBufferSizeUnit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* \see setExtentBufferSizeUnit()
* \see extentBuffer()
* \see setExtentBufferSizeUnit()
*
* \since QGIS 3.42

/**
* Sets the \a unit used for the extent buffer.
*
* \see extentBufferSizeUnit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* \see extentBufferSizeUnit()
* \see setExtentBuffer()
* \see extentBufferSizeUnit()
*
* \since QGIS 3.42

@nyalldawson
Copy link
Collaborator

Thanks @JuhoErvasti -- a few more minor comments here

@JuhoErvasti JuhoErvasti force-pushed the fix_38579_symbol_rendering_out_of_extent branch from 27fbdd6 to 7089713 Compare December 9, 2024 10:00
@JuhoErvasti
Copy link
Contributor Author

JuhoErvasti commented Dec 9, 2024

@nyalldawson

Thanks, I've addressed the points. Hopefully everything's OK now. Also I realized that the data defined property still could've produced a negative buffer so that should be handled now, as well as the @value variable

@nyalldawson
Copy link
Collaborator

@JuhoErvasti there's one bit missed in 7089713 -- we still need the \since added to extentBufferSizeUnit / setExtentBufferSizeUnit

@JuhoErvasti
Copy link
Contributor Author

@nyalldawson

Sorry, added now!

@nyalldawson
Copy link
Collaborator

Great, thanks for your hard work @JuhoErvasti !

@nyalldawson nyalldawson merged commit c130eb7 into qgis:master Dec 9, 2024
31 checks passed
@DelazJ
Copy link
Contributor

DelazJ commented Dec 10, 2024

@JuhoErvasti thanks for fixing that issue. Could you please update your top message to reflect the changes before we tag it for docs and changelog?

@JuhoErvasti
Copy link
Contributor Author

@DelazJ

I've updated the message now.

@DelazJ DelazJ added Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Dec 10, 2024
@qgis-bot
Copy link
Collaborator

@JuhoErvasti
This pull request has been tagged as requiring documentation.

A documentation ticket will be opened at https://github.com/qgis/QGIS-Documentation when this PR is merged.

Please update the description (not the comments) with helpful description and screenshot to help the work from documentors.
Also, any commit having [needs-doc] or [Needs Documentation] in will see its message pushed to the issue, so please be as verbose as you can.

Thank you!

@qgis-bot
Copy link
Collaborator

@JuhoErvasti

This pull request has been tagged for the changelog.

  • The description will be harvested so please provide a "nearly-ready" text for the final changelog
  • If possible, add a nice illustration of the feature. Only the first one in the description will be harvested (GIF accepted as well)
  • If you can, it's better to give credits to your sponsor, see below for different formats.

You can edit the description.

Format available for credits
  • Funded by NAME
  • Funded by URL
  • Funded by NAME URL
  • Sponsored by NAME
  • Sponsored by URL
  • Sponsored by NAME URL

Thank you!

@qgis-bot
Copy link
Collaborator

@JuhoErvasti
A documentation ticket has been opened at qgis/QGIS-Documentation#9461
It is your responsibility to visit this ticket and add as much detail as possible for the documentation team to correctly document this change.
Thank you!

@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 Feb 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangelogHarvested This PR description has been harvested in the Changelog already. Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geometry generator symbol not rendered if the feature itself is not on the map canvas
5 participants