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 automatic simplification to mask clipping paths when exporting layouts #57703

Merged
merged 15 commits into from
Jun 19, 2024

Conversation

nyalldawson
Copy link
Collaborator

When the (currently non-default) geometry backend is used for selective masking (see #57623), we now apply a conservative amount of simplification to the clipping paths when exporting layouts to pdf /svg.

This results in much smaller file sizes, and PDFs /SVGs which are much faster to load in viewers/editors.

Refs #50734
Refs #54788

(I think we could safely make the simplification factor more aggressive, but I've opted for an initially conservative amount while we discuss the rest of these changes)

@github-actions github-actions bot added this to the 3.38.0 milestone Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 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 1ff3477)

@qgis qgis deleted a comment from github-actions bot Jun 7, 2024
Add QgsAbstractGeometry::simplifyByDistance, which is a direct
port of GEOS Douglas Peucker algorithm.

This is a trivial algorithm to implement, and we benefit from
avoiding the conversion to/from GEOS geometries.
And API mechanism to propagate these from layout exports down
to the render context

Gives us a place to specify fine-tuned control over masking
settings for map renders
This results in much smaller output file sizes, and files which
load much quicker in PDF viewers/editors

Refs qgis#50734
Refs qgis#54788
multigeometries to separate geometries in the vector

This gives us more scope to optimise which geometries we want
to use when rendering objects within a particular part of the map
Move the geometry repair to when we add it to the render context,
instead of every time we retrieve it
By simplifying line strings PRIOR to buffering, we get a significant
performance bump, as we're not asking GEOS to buffer excessively
detailed linestrings and then immediately discarding most of the
vertices.
@nyalldawson
Copy link
Collaborator Author

@troopa81 can I get a review of this?

@troopa81
Copy link
Contributor

@troopa81 can I get a review of this?

Yes, I'll review it

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'm wondering if we should not treat mask like vector and reuse QgsLayoutRenderContext::simplifyMethod. Did you consider it?

Why simplifying both when painting and when merging clip path ?

src/core/geometry/qgslinestring.cpp Show resolved Hide resolved
src/core/qgsmaskrendersettings.h Show resolved Hide resolved
src/core/qgsrendercontext.cpp Show resolved Hide resolved
std::unique_ptr< QgsAbstractGeometry > buffered;
if ( mSimplifyTolerance > 0 )
{
// For performance, we apply a lower level of simplification to the line BEFORE doing the buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid that it doesn't improve this issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you retest please? There's no way the gridded/pixelated mask output could be exported by this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Way better

painterpath way
simplifybefore

geometry way
simplify

Can we make it a little less agressive by default to have a prettier renderering

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@troopa81

Thanks for the retest!

Can we make it a little less agressive by default to have a prettier renderering

Let's revisit that after #57799 -- with this PR alone the export file sizes are still very large

src/core/painting/qgsgeometrypaintdevice.h Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator Author

I'm wondering if we should not treat mask like vector and reuse QgsLayoutRenderContext::simplifyMethod. Did you consider it?

Yes, there's a couple of reasons why it needs to be different:

  • we want to use the different simplification algorithm for masks. As you pointed out in Add hidden QSettings switch to use a QgsGeometry/GEOS backend for label masking #57623 (comment) the snap to grid approach used for rendered geometries does not look good for masks. We use snap to grid for the rendered geometries to avoid holes appearing between features which were touching before simplification. This isn't a requirement for the masks, and we can use a simplification (Douglas peucker) which results in less vertices and less "pixelated" outputs.
  • we can get away with a much more aggressive tolerance for the mask simplification

Why simplifying both when painting and when merging clip path ?

The native simplification method is extremely fast. We want to simplify prior to the union or the GEOS union is very slow. But the output of the union can add additional vertices (when masks overlap and GEOS fully nodes them), which we can also strip away (with the cheap simplify call) and ensure smallest possible output file size.

@nyalldawson
Copy link
Collaborator Author

Thanks for the review @troopa81 ! It's much appreciated!

@nyalldawson nyalldawson merged commit 5d81083 into qgis:master Jun 19, 2024
30 checks passed
@nyalldawson nyalldawson deleted the simplify_mash branch June 19, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants