Add hidden QSettings switch to use a QgsGeometry/GEOS backend for label masking #57623
Conversation
|
I'm -1 on this. If we really do think that replacing mask computation is better than basic one, I would rather deprecate the old one and switch completely to geometry one than add a new setting and code complexity/branches all over the place. No user has the needed knowledge to be able to choose a mask backend. Regarding selective masking and vector ouput, I would very much prefer to slow down things a bit and be sure we can fix the main generation issue before adding too much code. |
Agreed, that's why this is an opt in, hidden setting for now. It was intentional to choose a hidden approach because we should definitely NOT be requiring users to pick a mask backend for themselves.
I respectfully disagree. Masking is basically unusable in many situations right now, and dramatic changes will be necessary to bring it to a stable state where it's reliable and working for all. My intention here was to limit the user impact by introducing this as an opt in setting so that users can test and give feedback on the new backend without the risk of making things worse. We can easily deprecate and remove the old code when we're confident that the new approach is better for everyone. Also regarding code complexity -- the new paint engine has been designed with multiple future use cases in mind, including the ability to render text/labels using standard qgis polygon fill symbols, and the ability to render coloured "halos" around marker symbols. Both of these are large feature gaps we have compared with ESRI software and are something I plan on adding during the 3.40 cycle. So the new class is not solely for masking use, but for other future cartographic purposes too. |
But does the solution you propose fix the major issue when QGIS crashes when exporting a PDF layout with too many masks? I have the feeling we are rushing towards a specific direction without being sure that it fixes our very major issue. Sincerely, the overall looks good and I'm looking forward to see where it goes. I'm just not not very fond of adding new code that user could test with an hidden settings. Could we not just test ourselves with the few scenario that already crashes the application and if it works, then deprecating the old code? |
…el masking If the 'map/maskBackend' QSettings key is set to 'geometry', then QgsGeometryPaintEngine will be used for calculating clip paths for label masking instead of QPainterPath The intention here is that we gain improved flexibility to optimise the creation and logic behind clipping path generation, vs the limited API we get from QPainterPath.
Use GEOS to do the path stroking
It does fix the crash -- you can try https://github.com/nyalldawson/QGIS/commits/simplify_mash/ if you'd like to verify. Additionally, that rough simplification change (which I intend to introduce and refine in a follow up pr) drops the exported PDF file size from over 200mb to around 20mb, and it loads in PDF viewers MUCH quicker (and also can be opened in Inkscape without an endless hang). So yes, it definitely does make concrete steps towards fixing the issue! 👍 |
|
@troopa81 tests are all passing now (with the exception of the clang tidy false positive), am i good to merge now? |
I'm gonna make a proper review of it |
There was a problem hiding this comment.
The code looks good, just 2 minor comments.
I tested it with #50734 project and it seems to be working. Indeed the produced file is 10 times smaller with the proposed approach 👍
Is there a good reason to add a setting and not just get rid of the old method ?
...ng/layout_export_random_generator_fill/variant1/layout_export_random_generator_fill_mask.png
Outdated
Show resolved
Hide resolved
|
It looks like that mask resolution has been severely lowered. With painterpath mask With geometry mask Is it just an issue? Do we keep the same 10x size reduction factor if we match the same resolution than now? |
I agree that's not great. I'll fix before opening the simplification pr! |
If the 'map/maskBackend' QSettings key is set to 'geometry', then QgsGeometryPaintEngine will be used for calculating clip paths
for label masking instead of QPainterPath
The intention here is that we gain improved flexibility to optimise the creation and logic behind clipping path generation, vs the
limited API we get from QPainterPath.
For now, the default remains with the existing QPainterPath approach.