[FEATURE] Selective masking

[mhugo]

Description

This PR covers the implementation of "selective masking" as depicted in qgis/QGIS-Enhancement-Proposals#63

Both the core part and the GUI are present.

I've added some design documentation with examples in a new doc/design folder (don't know if this is the right place) because the subject is a bit complex ...

So, two types of "masks" can be defined:

• masks defined by a text buffer around labels
• masks defined by (marker) symbols

Each mask points to a set of symbol layers in different layers that must be "un-rendered" so to say.

Masks defined by marker symbols are defined by the introduction of a new symbol layer : it contains a sub-symbol that lets the user use whatever complex symbol he wants to define the mask shape (only the opacity of this shape is of interest, the color is ignored).

The fact that the masking must be done at the level of symbol layers (both for the "source" of masking - not only labels, but also as "destination" of masking) adds some complexity to the overall rendering process.

The rendering process is now completed by a second pass after everything has been rendered.

• During the first pass, sources of masks (labels or mask symbol layers) render their shape in a new "mask" image.
• Then after the first pass, new jobs of rendering are launched with some symbol layers disabled.
• Results from the second pass are composed with masks and results from the first pass

I tried my best to make the "second pass" rendering as independent as possible from the rest of the rendering process, but I would like to have some feedback about the overall approach described here.

Masks are painted in a separate image buffer. Only the alpha channel is needed here, so the color is ignored, but anything that has a varying alpha can be used. In particular, paint effects can be used to display a "partial mask".

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include fixes #11111 in the commit message next to the description
• Commits which add new features are tagged with [FEATURE] in the commit message
• Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
• I have read the QGIS Coding Standards and this PR complies with them
• This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit

[andreasneumann]

Nice to see progress - and thanks for sharing the examples. At https://github.com/mhugo/QGIS/blob/selective_masking/doc/design/selective_masking/p1/README.md you also need to enable the symbol levels on the road layer in order to first render all black lines and then all yellow lines on top of the black ones (good test to see if that still works after your changes).

[mhugo]

@andreasneumann Thanks for your feedback. I'll add a test with symbol levels enabled, you're right.

Actually, the part with which I am struggling is case 4 (and 5) in my examples.

So we have yellow-orange circles that must erase yellow lines of roads and labels that must erase black lines of roads. The problem is: what to do where label and circles' masks overlap ? The current result is not very intuitive. I guess we should have in that particular case both yellow and black lines erased on locations where the two masks overlap.
But .. this adds a lot of complexity in code and could also result in a very slow rendering.

I am reconsidering this particular use case here: what if we limit the masking so that the user could not define different masks that erase different "sets" of symbol layers on a given layer ? Or do you think this is a must-have ?

[andreasneumann]

@mhugo I think that normally both label and symbol masks would mask the same set of symbol layers.

Yes, the result in example 4 still looks strange. While the masks of the labels look good, the ones from the orange circle symbols look wrong. They are masking the yellow lines away, while they should mask the black lines and keep the yellow ones. In example 5 it is the opposite the symbols are masked fine, but the labels not. But that's probably the part you are still struggling with.

cc @nyalldawson - what do you think?

[andreasneumann]

@mhugo back to your question: I think that both labels and symbol would mask the same symbol layers of a given layer, not different ones. So we would need different masks (from different layers) that mask the same set of symbol layers in another layer. Do we understand each other?

[mhugo]

@andreasneumann So you confirm, examples 4 and 5 should not exist, right ? The user must be constrained so that these cases cannot exist ?

[mhugo]

On another topic: I've added a QgsSymbolLayerId class, but there is already a very close concept of "legend keys" in QgsFeatureRender that I could reuse.

[mhugo]

@andreasneumann Just to try to be clearer (?): you confirm we should have in example 4 labels and symbols that both mask, say, the yellow part of roads and in example 5, labels and symbols that both mask the black part of roads, but we cannot have labels that mask the yellow part and symbols that mask the black line at the same time ?

[andreasneumann]

@andreasneumann Just to try to be clearer (?): you confirm we should have in example 4 labels and symbols that both mask, say, the yellow part of roads and in example 5, labels and symbols that both mask the black part of roads, but we cannot have labels that mask the yellow part and symbols that mask the black line at the same time ?

Confirmed.

However, in example 3 I would like to see the improvement that the label masks can also mask away the black outline of the orange circle symbols (which is also a mask for the black lines in the road network) - do you think this is or will be possible?

[mhugo]

Confirmed.

@andreasneumann Good to know !

However, in example 3 I would like to see the improvement that the label masks can also mask away the black outline of the orange circle symbols (which is also a mask for the black lines in the road network) - do you think this is or will be possible?

Hmmm.
Currently the orange circles are made by only one symbol layer with a stroke color and a fill color. What you need could be achieved by splitting the circles in two symbol layers: one for the black outline and another one for the orange interior. Do you think it is acceptable ?

[andreasneumann]

Currently the orange circles are made by only one symbol layer with a stroke color and a fill color. What you need could be achieved by splitting the circles in two symbol layers: one for the black outline and another one for the orange interior. Do you think it is acceptable ?

Yes, that's certainly acceptable. This whole masking thing is quite advanced anyway. A user that uses this functionality is most likely also familiar with the concept of multiple symbol levels and how they work.

[nyalldawson]

@mhugo

Just FYI - i've a partial review I've been chipping away at over the last week. There's a lot to understand here and it's taking some time, especially given how complex the map rendering code already is. But don't misinterpret the lack of feedback as a lack of interest! (it's fantastic stuff)

[mhugo]

@nyalldawson Thanks for your comment. I am updating this PR with a complete implementation, including the GUI part (screenshots yet to come). The rendering process should now be a bit simplified.

[nyalldawson]

Looking great! I haven't reviewed map renderer changes yet sorry, but this is a review of the rest....

[nyalldawson]

My interpretation of reading this PR is that this symbol layer type will ONLY work in map renders. E.g. it shouldn't be an option in other contexts, such as labeling background marker symbols, or centroid fills used in layout polygon shape items. We need to add api to limit it being exposed as an option ONLY for vector layer renderers.

[nyalldawson]

Also - what happens with masks in legends?

[mhugo]

We need to add api to limit it being exposed as an option ONLY for vector layer renderers.

On GUI side, perhaps with something added to QgsSymbolWidgetContext ?

Also - what happens with masks in legends?

If no mask painter is defined, it is rendered as a regular symbol layer.

[nyalldawson]

If no mask painter is defined, it is rendered as a regular symbol layer.

Wouldn't this then show the big red buffer in the legend?

[mhugo]

Yes it will. Do you think it's a problem (apart from the color) ?

[nyalldawson]

Yes - I think it's an issue. What I would suggest is doing something like:

• adding a new flag to QgsRenderContext, something like " Is GUI preview only"
• only rendering the red mask in gui previews - for all other contexts where a mask isn't used (e.g. legends), just totally skip masks and don't render anything.

Then you could safely use point layers with masked symbols in legends and they'll look as expected.

[mhugo]

I've changed things so that only QgsSymbol::bigSymbolPreviewImage() will render the mask. QgsSymbol::asImage() and QgsSymbolLayerUtils::symbolLayerPreviewImage() do not.

[nyalldawson]

Hey @wonder-sk - could really use your opinion on the map renderer changes here. I haven't dug into them in depth yet, but I figure you would have a strong opinion on this!

[mhugo]

I've updated the PR description with some screenshots

[nirvn]

@mhugo , ok, those GIFs did it for me, my jaw officially dropped to the floor!

Fantastic work!

[andreasneumann]

@mhugo - great to see progress! Very much looking forward to test it soon.

One question I have is on the mask type behind text labels. Currently, the mask behind text is following the outline of the individual letters, resulting in curved shapes. Would it be possible to add an alternative "rectangular" mask type that would be a bounding box of the whole text - similar to the rectangle background shape one can choose behind text labels?

See this screenshot for label backgrounds:

So basically one could have two options:

• "buffer" following individual letters/glyphs (like you already support now)
• rectangular background (like in option 1 in the graphic below)

At a later stage one could also think about adding a third type, something like option 2 as a "rectangular buffer" option, but that follows "x-height" and "cap-heights", "baseline" and "tails", as defined here: https://www.fontshop.com/glossary/

[mhugo]

@andreasneumann

Would it be possible to add an alternative "rectangular" mask type that would be a bounding box of the whole text - similar to the rectangle background shape one can choose behind text labels?

Yes I think @haubourg would also like to have it :) I've run a bit short of time to implement it. I'll see what I can do to add it when other aspects discussed here are ok.

[mhugo]

@nirvn Thanks for your positive feedback :-)

[andreasneumann]

Yes I think @haubourg would also like to have it :) I've run a bit short of time to implement it. I'll see what I can do to add it when other aspects discussed here are ok.

Good - I understand that this may be "followup" work. Not a problem.

[mhugo]

@nyalldawson Thanks for your review, handling of data defined properties of labeling was harder than I thought :/

I still have to figure out how to restrict the use of mask symbol layer to labeling and add a test for hi dpi exports

[mhugo]

I have interpreted Martin's "Overall great job, +1 from me to merge" as an approval ... Anyway, tell me if you think something is still missing.

I'll do the rebase and have another look at the code state today

[nyalldawson]

@mhugo sorry, i was under the impression you still had changes you wanted to make, and I didn't want anyone merging as a result of the "merge after thaw" tag. +1 to merge from me too!

[luipir]

merge from me too

+1

[mhugo]

@nyalldawson @luipir Thanks.

Indeed, I had in mind Martin's last suggestions to refactor a bit the code so that the original rendering loop keeps its original form (by moving the second pass handling in QgsVectorLayerRenderer). I've spent a few hours on that on a branch, but it requires a little bit more extra time, which unfortunately I do not have.

I prefer merging now and keep a bit of extra time to answer to feedback and fix things.

Thank you very much all for the reviewing effort !

[mhugo]

Sorry, wrong git manipulation

[mhugo]

@3nids I confirm this is related. But I don't confirm the compilation error. Will have a second check ...

[3nids]

@mhugo sorry, mixed up things. Let me double check on my side, sorry for the noise

[andreasneumann]

I just compiled fine with selective masking.

[nyalldawson]

@mhugo
There's something a bit funky going on the gui. Checkout the screencast below:

I set the label properties to use a mask, and try to change the buffer size -- no effect. Then I go to the masked layer's properties and setup the masking and the results are shown correctly. I then go back to the first layer and try to alter the buffer size again, and nothing happens. Then going back to the masked layer, all the settings have been lost...

[nyalldawson]

I've also noticed that PDF outputs from layouts with these settings result in the whole map being flattened and force rasterized. Why is that? At worst, shouldn't only the mask shape be a raster?

[nirvn]

This has also likely led to another regression I just spotted: when rendering, the canvas will flash white prior to drawing the labels. The flashing lasts longer for more complex label scenario.

One simple polygon layer with a rule-based labelling setup blanks the canvas for ~30sec over here too, will provide a test case later today.

[nirvn]

OK, regression has to do with labels with a non-normal blending settings.

Here's a test project:
grid_slow_label.zip

This is a pretty bad UX regression.

[haubourg]

cc @mhugo did you get the latest messages here about regressions ?

[nirvn]

I prefer merging now and keep a bit of extra time to answer to feedback and fix things.

@mhugo , gentle ping on these post-merge comments. There are several regressions (as well as implementation issues and bottlenecks) raised here that definitively need addressing sooner than later.

In terms of regression, the canvas turning white after symbology is drawn and prior to labels being drawn has to be dealt with, it's a regression that makes it really unenjoyable to use QGIS master ATM.

The server regression is also quite serious (arguably more so than the highly visible UX regression raised above).

[mhugo]

@nirvn @nyalldawson Really sorry for my total absence of answer. These seem serious issues. I'll have a look asap.

[mhugo]

@nirvn Thanks for your testcase, I can reproduce easily. My console is flooded with messages like "QPainter not active"

[mhugo]

@nirvn The problem only appears when parallel rendering is enabled, like in #32791

[nirvn]

@mhugo , by parallel rendering, you mean the default QGIS desktop rendering behavior? 😄

I'll test out PRs as they come. I can't wait for a fix to canvas blank flashes.

[mhugo]

@nirvn Found something, I am on it.