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

interpolated line symbol layer for vector layer #42676

Merged
merged 4 commits into from
May 5, 2021

Conversation

vcloarec
Copy link
Member

@vcloarec vcloarec commented Apr 6, 2021

This PR introduces a new renderer symbol layer for line vector layers. This symbol layer is the porting of the mesh layer's edge rendering.

interpolatedLine

The user can associate attributes or expressions to each line string's extremity (or curve). Value is interpolated along the line string between extremities.
The rendering is done with color and/or width
varying depending on the interpolated value, resulting in a color and width varying line.
The color rendering is set with a color ramp shader widget (like raster and mesh layers).

Funded by Lutra Consulting

Note: text above was changed following the discussion below.

@vcloarec vcloarec added Feature Symbology Related to vector layer symbology or renderers labels Apr 6, 2021
@nirvn
Copy link
Contributor

nirvn commented Apr 6, 2021

Ok, first, cool :)

Follow up question: why a renderer instead of a symbol layer? The latter would be more flexible in terms of cartographic possibilities.

@roya0045
Copy link
Contributor

roya0045 commented Apr 6, 2021

If this is a port, does that duplicate the code in some ways? Or if some aspects are reused, must the two be fully independent ?

Though it might be nice for distance representation if length is used as the max.

@vcloarec
Copy link
Member Author

vcloarec commented Apr 6, 2021

@nirvn good point! I had thinking about, but I don't remember why, there were some reasons to not make it as a symbol... Maybe I missed something... I look again.

@roya0045 the rendering code is not duplicated. The base of mesh edges rendering is enough low level to be used with other rendering without implicate mesh stuff.

@nirvn
Copy link
Contributor

nirvn commented Apr 6, 2021

The obvious gains here are that we could use the current categorized,graduated, and rule based renderer with this new symbol layer.

@github-actions github-actions bot added this to the 3.20.0 milestone Apr 7, 2021
@vcloarec
Copy link
Member Author

vcloarec commented Apr 7, 2021

@nirvn
One problem I see with this rendering as a symbol layer, it is with the clip on extent option that is set and apply at the QgsSymbol level.
With this clipping, the interpolated line rendering would not work well. Indeed, the rendering depends on the position on the entire line.
Maybe a solution could be to use the same workflow than the QgsGeometryGeneratorSymbolLayer, here:

void QgsSymbol::renderUsingLayer( QgsSymbolLayer *layer, QgsSymbolRenderContext &context )

But I am not sure, for now, I do not know enough symbol layer mechanisms...
I take all opinions!

@vcloarec
Copy link
Member Author

vcloarec commented Apr 8, 2021

@nirvn
I've almost implemented this rendering as a symbol layer, but I haven't tested it for now. I am wondering if I succeed if we could keep the renderer too. So we could have both, the renderer and the symbol layer. I find simpler for the user to access this rendering by the renderer. And for advanced rendering, the user could use the symbol layer.

@zacharlie zacharlie added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Apr 8, 2021
@nirvn
Copy link
Contributor

nirvn commented Apr 9, 2021

I'm -0.5 to keep the renderer. If it merely serves as a way to highlight one specific symbol type vs. serving an actual technical need, then I say let's not have the renderer. There are other ways to highlight cool symbologies (e.g., you could one favorite saved symbols in our style manager, that would pop up in people's UI load and clear :) )

@nirvn
Copy link
Contributor

nirvn commented Apr 9, 2021

An actually, on that note, another benefit from having it as a symbol layer is being able to see those cool styles in saved style symbol previews.

@vcloarec vcloarec force-pushed the variable_width_color_vectorLayer branch from 634ba36 to 4a1dc9f Compare April 9, 2021 02:42
@vcloarec
Copy link
Member Author

vcloarec commented Apr 9, 2021

Symbol layer now implemented.

About the render, if I am +1, the sum is +0.5 :)
For the renderer, it is not only for highlight it, but I really find simpler for the user to access this symbology from renderer.
Other reason to keep it: I am not sure we can have the color ramp in the legend with symbol layer. With renderer, we have it.
Last reason, its implementation ask me quite big effort for discovering the depths of layer vector rendering :)

@nirvn
Copy link
Contributor

nirvn commented Apr 9, 2021

For the renderer, it is not only for highlight it, but I really find simpler for the user to access this symbology from renderer.

Being devil's advocate here, wouldn't that logic have us create a renderer for all symbol layer types we like to make it simpler to access? 😉

The legend item is a fair point, yet I suspect it's possible to implement that without going down the path of a renderer. @nyalldawson would know better.

@vcloarec vcloarec force-pushed the variable_width_color_vectorLayer branch from a651cbd to 4d6e1b8 Compare April 9, 2021 03:34
@nyalldawson
Copy link
Collaborator

A couple of questions jump to mind based on the below screenshot:

image

  • Could we make first and last value as simple numeric controls, with a data defined button next to them for expression/field based values? That would be more in keeping with how all the other symbol layers operate
  • "Strock" -> "Stroke" 😆
  • What does the "Load" button do?
  • For consistency with other symbol layers, the range of stroke widths should be implemented as two spin boxes instead of a button
  • It's not possible to vary the color by one attribute and the width by another is it?

@vcloarec
Copy link
Member Author

vcloarec commented Apr 9, 2021

Thanks for this comments.

* Could we make first and last value as simple numeric controls, with a data defined button next to them for expression/field based values? That would be more in keeping with how all the other symbol layers operate

Here, the idea is to link extremities of line with varying value, typically field values. That why we have these field expession widgets. With simple numeric control, the idea is not exactly the same. Even the value can be overridden, in first approach for the user, it would be a fixed value, it is not the idea.

* What does the "Load" button do?

The load button load the min/max value of the field or expression "First value"/ "Second value" for all features. In case of fixed values, that will be the fixed values...

* For consistency with other symbol layers, the range of stroke widths should be implemented as two spin boxes instead of a button

Spin boxes are behind the button:

button

This widget is associated with the width varying, already implemented for mesh edges.

* It's not possible to vary the color by one attribute and the width by another is it?

No, only one value is interpolated along the line, color and width are both
derived from this value.

@DelazJ DelazJ added the Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. label Apr 11, 2021
@github-actions
Copy link

@vcloarec
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!

@wonder-sk
Copy link
Member

I would also prefer that all this functionality would be implemented as a symbol layer. Having a dedicated renderer should not be necessary - and having two options (both symbol layer and renderer) would be confusing for users.

As for GUI, I think there are two things worth addressing:

  • having a single range ("first value" and "second value") is maybe unnecessary limitation - it would be good to have one range for pen widths and another for colors. I think generally there would be three use cases for interpolated lines:
    1. varying pen width, single color
    2. varying color, single pen width
    3. varying pen width + varying color
  • choice of GUI widget - I understand you have tried to reuse widgets from mesh layer GUI, but there's a room for improvement to make things look more consistent with other layer styling widgets, and to hide some more advanced things:
    0. a single widget to pick one of the three modes as outlined above ... or maybe two separate switches to enable/disable varying width/color - to control what widgets would be shown
    1. widgets for varying width:
      • field/expression for "value 1" (at the start) + field/expression for "value 2" (at the end)
      • min/max range for input values, min/max range for output widths
      • [optionally] "advanced" button for more detailed setup of input value -> output width like what we have for data-defined size assistant
    2. widgets for varying color:
      • field/expression for "value 1" (at the start) + field/expression for "value 2" (at the end)
      • min/max range for input values
      • color ramp
      • interpolation method
      • [optionally] "advanced" button for more detailed setup - to essentially set up the "graduated" renderer
    3. widgets for non-varying color:
      • just simple color widget
    4. widgets for non-varying pen width:
      • just simple width widget

May something like this - showing two different modes:

image

By the way, this is the existing data-defined size assistant dialog in QGIS - maybe we could reuse it for the advanced configuration:
image

@vcloarec
Copy link
Member Author

with @wonder-sk suggestion for the GUI

interpolatedLine

Only symbol layer, no renderer.

@nirvn and @nyalldawson , if you are ok with this principle, I will go on this way.

@roya0045
Copy link
Contributor

@vcloarec Could you go with tab instead of radio buttons? Having buttons change the UI is strange and unexpected. Having it as something like a tab would better indicate the change of UI. Maybe that's just me.

@vcloarec
Copy link
Member Author

Could you go with tab instead of radio buttons? Having buttons change the UI is strange and unexpected. Having it as something like a tab would better indicate the change of UI. Maybe that's just me.

@roya0045 , thanks for your comment. What have you exactly in mind one tab for fixed and one tab for varying? And one group of tabs for stroke width and one for color?
Like below for stroke width?
interpolatedLine_tab

If not, can you point another part of QGIS that it is similar of what you think?

@nirvn
Copy link
Contributor

nirvn commented Apr 26, 2021

-1 for tabs, it's more confusing that anything else and we've never used focused tab as a way to activate a state.

The radio box isn't ideal but it's already a great improvement.

You could use a combo box instead with nice icons to express what the mode does. That'd fit better with what we have elsewhere.

Thanks for your flexibility so far, and for this great feature.

@roya0045
Copy link
Contributor

-1 for tabs, it's more confusing that anything else and we've never used focused tab as a way to activate a state.

The radio box isn't ideal but it's already a great improvement.

You could use a combo box instead with nice icons to express what the mode does. That'd fit better with what we have elsewhere.

Thanks for your flexibility so far, and for this great feature.

I feel like the radio box provide a choice but not something that has ramification or would change the UI. Two radio button on a single line shouldn't change the rest of the section IMO. With the Tab this indicate that the underlying change is more involved and has more ramification. But it can also be seen as a concurrent thing rather than a choice between two option.

Though I'm sure there is a way to not have the 'fixed' panel and integrate this in the other one.

But otherwise as long as it works I don't mind.

@vcloarec vcloarec force-pushed the variable_width_color_vectorLayer branch from 4d6e1b8 to ef595c6 Compare April 28, 2021 03:30
@vcloarec
Copy link
Member Author

PR updated for symbol layer.
The description on the top was updated to take account of modification.

@nirvn indeed, for fixed varying, combo box is better than radio button, for icons, it will be great to have the same as the preview icon in the layer tree but corresponding with fixed or varying. But maybe later...

@vcloarec vcloarec changed the title interpolated line renderer for vector layer interpolated line symbol layer for vector layer Apr 28, 2021
@vcloarec vcloarec closed this Apr 28, 2021
@vcloarec vcloarec reopened this Apr 28, 2021
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.

Looking great, just minor bits before merging it...

src/core/symbology/qgssymbollayerregistry.cpp Outdated Show resolved Hide resolved
src/core/symbology/qgsinterpolatedlinerenderer.cpp Outdated Show resolved Hide resolved
src/core/symbology/qgsinterpolatedlinerenderer.cpp Outdated Show resolved Hide resolved
src/gui/symbology/qgsinterpolatedlinesymbollayerwidget.h Outdated Show resolved Hide resolved
src/gui/symbology/qgsinterpolatedlinesymbollayerwidget.h Outdated Show resolved Hide resolved
}

return colorRampShader;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm I am not a big fan of this implementation to read/write color ramp shader from/to QVariantMap:

  • source color ramp properties are mixed with shader properties - can we keep them separated? (like it is done with "color_ramp_shader_items_list")
  • source color ramp properies could be probably loaded/saved with QgsSymbolLayerUtils::colorRampToVariant() + QgsSymbolLayerUtils::loadColorRamp() - this would avoid the need to call create() method of various color ramp types directly
  • legend settings are loaded/stored as embedded XML, which is suboptimal. And it looks like we do not need legend settings of the shader for interpolated line renderer anyway (though I understand you have added it for completeness)

I wonder if we could instead move these two functions as private methods to interpolated line symbol layer class... So it will be just implementation detail of the symbol layer, and not "official" API of QgsColorRampShader. By doing that, we could also afford to leave out the bits of information we don't need (like the mentioned legend settings). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@github-actions
Copy link

github-actions bot commented May 5, 2021

@vcloarec
A documentation ticket has been opened at qgis/QGIS-Documentation#6692
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 Jun 5, 2021
@vcloarec vcloarec deleted the variable_width_color_vectorLayer branch September 7, 2021 16:55
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. Feature Needs Documentation When merging a labeled PR, an issue will be created in the Doc repo. Symbology Related to vector layer symbology or renderers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants