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

Greatly optimise selective masking vector exports #57799

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

nyalldawson
Copy link
Collaborator

@nyalldawson nyalldawson commented Jun 19, 2024

Optimise the logic used when the new geometry backend for selective masking is in effect:

Whenever its SAFE, instead of calculating an "entire map" clipping path and then applying this for every feature being rendered,
we now defer the calculation of the clipping path until we are rendering individual features. Then, we create a clipping path
which contains ONLY the mask paths which are within the area being drawn over.

This avoids having the entire map clipping path being used for EVERY feature being rendered, which results in huge PDF/SVG
exports when masks are in effect, and instead results in clipping paths which are confined just to a sensible area
around each rendered feature.

In some complex test projects this reduces the PDF export size by a factor of 0.01, without any loss of quality or regressions in export time!! (Eg 60Mb -> 700kb). It also results in PDFs/SVGs which open much quicker in viewers and editors, and don't grind their operation to a halt.

Fixes #50734
Fixes #54788

@github-actions github-actions bot added this to the 3.38.0 milestone Jun 19, 2024
@nyalldawson
Copy link
Collaborator Author

@troopa81 here's the next optimisation for selective masking exports. I consider this one the "holy grail", which squashes the last of the remaining big limitations of the vectorised selective masking.

Copy link

github-actions bot commented Jun 19, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit c0d58b1)

Copy link
Contributor

@troopa81 troopa81 left a comment

Choose a reason for hiding this comment

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

I made a first quick review, I'll try to finish it in a few hours.

Having a tailored mask for each feature is interesting, but I think it brings a lot of complexity to the code (specially in QgsSymbol::renderFeature).

Like I said before, I'm wondering if we could not have improved Qt by sharing/referening clippingPath when writing PDF. This way, we would have had one global mask (maybe built and simplified using the geometry paint device) applied to all features. In the end, the crash appears when serializing, not painting.

Therefore, the more feature you have, the bigger the file will be because you multiply masks, while with a global mask your will size stay constant.

src/core/symbology/qgssymbollayerutils.h Show resolved Hide resolved
src/core/symbology/qgssymbollayerutils.cpp Show resolved Hide resolved
src/core/symbology/qgssymbol.h Outdated Show resolved Hide resolved
src/core/symbology/qgssymbollayer.cpp Show resolved Hide resolved
Allows us to know in startRender whether the symbol is actually
a subsymbol for a QgsSymbolLayer. Currently unused.
Optimise the logic used when the new geometry backend for
selective masking is in effect:

Whenever its SAFE, instead of calculating an "entire map" clipping
path and then applying this for every feature being rendered,
we now defer the calculation of the clipping path until we
are rendering individual features. Then, we create a clipping path
which contains ONLY the mask paths which are within the area
being drawn over.

This avoids having the entire map clipping path being used for
EVERY feature being rendered, which results in huge PDF/SVG
exports when masks are in effect, and instead results in
clipping paths which are confined just to a sensible area
around each rendered feature.

In some complex test projects this reduces the PDF export
size by a factor of 0.01!! (and results in PDFs/SVGs which
open much quicker in viewers and editors, and don't grind
their operation to a halt).
These are the symbol layer classes where there's no special logic
required relating to feature rendering and features are rendered
one-by-one, with no sub symbols.
@nyalldawson
Copy link
Collaborator Author

@troopa81

I made a first quick review, I'll try to finish it in a few hours.

Thanks!

Like I said before, I'm wondering if we could not have improved Qt by sharing/referening clippingPath when writing PDF. This way, we would have had one global mask (maybe built and simplified using the geometry paint device) applied to all features.

Therefore, the more feature you have, the bigger the file will be because you multiply masks, while with a global mask your will size stay constant.

That sounds good, and it would definitely be nice to see this happen in some future Qt release! 👍 I've now added a render context/layout flag which forces the "entire map" clipping paths to always be used. It's opt in, but if Qt does improve the situation then we'll easily be able to switch back by just enabling this flag. (At some stage I'd love to make some advanced dialog for configuring layout export presets, and then we could even expose this setting to users who want fine control over this, and all the other mask simplification parameters too).

Pragmatically though, there's no related Qt improvements in current Qt 6 versions and use of local clip masks fixes the bulk of the issues with exporting complex layouts, so we should make this default behaviour for now.

In the end, the crash appears when serializing, not painting.

It's caused during serializing the painter state, because the clip path is too complex for the structures Qt internally is using to do this. So we'd actually need two upstream improvements -- one would be the "write clip path once when exporting to SVG/PDF" improvement, and the other would be "adapt QPainter state storage to handle complex clip paths".

On the other hand, with the changes here I can confirm that use of local clip paths fixes all the crashing projects from the linked tickets with current Qt stable versions.

This is a new opt-in flag for map settings/render context/layouts.
If set, then when applying clipping paths for selective masking,
we always use global ("entire map") paths, instead of calculating
local clipping paths per rendered feature. This results in
considerably more complex vector exports in all current Qt versions,
but gives us a way to force this IF/when a future Qt version adds
optimisations which make global masks desirable.
@qgis qgis deleted a comment from github-actions bot Jun 20, 2024
@troopa81
Copy link
Contributor

Is it normal ?
layout_export_mask

@nyalldawson
Copy link
Collaborator Author

@troopa81

Is it normal

Yes, it's relating to that qt issue regarding paths bleeding out of the map frame's clip area. I'm fairly certain I can fix that now by doing the intersection of map item clip path and selective masking clip path via the geometry approach, but I'll do that in a different PR. And if it works I'll regenerate all the reference images so that none of them have the bleed anymore.

@nyalldawson nyalldawson enabled auto-merge (rebase) June 20, 2024 22:51
@nyalldawson nyalldawson merged commit 495091d into qgis:master Jun 21, 2024
30 checks passed
@nyalldawson nyalldawson deleted the thin_clip branch June 21, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants