Check and fix algorithms improvement #61531

Djedouas · 2025-04-17T09:14:53Z

Description

Following #59637 to improve the processing checks and fixes already here.

avoid using new keyword to avoid memory leaks
avoid using unnecessary pointers
remove auto keyword
better formatting
better algorithm descriptions

github-actions · 2025-04-17T10:26:29Z

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit c2544ea)

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit c2544ea)

DelazJ

Hi @Djedouas
Thanks for these updates. I was about to open issue reports for these and saw your PR, hence I'm asking here.

src/analysis/processing/qgsalgorithmcheckgeometryangle.cpp

DelazJ · 2025-04-17T11:02:02Z

src/analysis/processing/qgsalgorithmcheckgeometryangle.cpp

+  ) );
+
+  std::unique_ptr<QgsProcessingParameterNumber> tolerance = std::make_unique<QgsProcessingParameterNumber>(
+    QStringLiteral( "TOLERANCE" ), QObject::tr( "Tolerance" ), Qgis::ProcessingNumberParameterType::Integer, 8, false, 1, 13


As mentioned in qgis/QGIS-Documentation#9773, afaiu I find this tolerance parameter (8 meaning 10^-8) non-obvious, and its min/max values counter-intuitive. A parameter help is at least required for end users, but imho, we should move to something simpler as a plain double value with unit selector, as commonly used for this kind of parameter.

Yes but it's to keep consistency with Geometry Checker plugin:

output.mp4

btw, Geometry Checker accepts only an integer https://github.com/qgis/QGIS/blob/master/src/analysis/vector/geometry_checker/qgsgeometrycheckcontext.cpp#L20

Yes but it's to keep consistency with Geometry Checker plugin:

Probably, but 1E-8 as shown in the plugin looks much more meaningful than 8. I don't know how strong is/should be the link between the two tools and I'm not speaking in terms of code, rather as a Processing end user who would expect some obviousness in the options and harmonization (and that we take advantage from improved parameter types in Processing)...

Yes I agree that from an end user perspective, it could be confusing. However, we are tied with how the geometry checker is written right now.

We can't use the distance widget parameter because it would be nonsense and difficult to restrict the user to specific logarithmic values like "10, 1, 0.1, 0.001, 0.0001, etc.".

Our options are

to create another parameter type, think of its UI, and that handles the differences we have between geographic and metric distances

to change how the geometry checker tolerance is handled

I prefer to go with option 2. but in any case, it will be a work that would be done in a dedicated PR.

Right now, we could improve the documentation, or add a description of the tolerance parameter in every processing description.

I would have said

Right now, we could improve the documentation, orAND add a description of the tolerance parameter in every processing description.

They should have a start of answer in the GUI imho.

Added a short text in every processing short help.

The "Tolerance" advanced parameter defines the numerical precision of geometric operations, given as an integer n, meaning that any difference smaller than 10⁻ⁿ (in map units) is considered zero.

Given that this option is in advanced panel, I doubt people will modify it and don't think it deserves a mention in the algorithm main help. It should instead be as a tooltip over the parameter only, i.e.:

tolerance->setHelp( QObject::tr( "The \"Tolerance\" defines the numerical precision of geometric operations, " "given as an integer n, meaning that any difference smaller than 10⁻ⁿ (in map units) is considered zero." );

Djedouas · 2025-04-18T09:39:30Z

Hi @DelazJ

What do you think of changing the "Check geometry (<name>)" processing name to simply "<name>" ?

It seems heavy right now:

DelazJ · 2025-04-18T15:54:21Z

What do you think of changing the "Check geometry (<name>)" processing name to simply <name>" ?

I agree that it looks crowded. I'm not very sure that an alg named "Angle" or "Contained" would be obvious (well, it is still displayed under the check group).
Maybe "Check <name>"? though I'm still not sure if "Check contained" is a quite expressive name...

PS: I'm not sure that the alg name is supposed to be in title case... ("Check Geometry (Dangle) --> Check geometry (dangle)")
PS2: most of the time, the alg outputs are named using the alg name, i.e., Buffer --> Buffered, while here they all output "layer output" and "Error layer". It could be nice to have part of the alg name in the output(s)
PS3: Thanks for the switch to optional output.

Djedouas · 2025-04-24T08:52:56Z

Maybe "Check "? though I'm still not sure if "Check contained" is a quite expressive name...
PS: I'm not sure that the alg name is supposed to be in title case... ("Check Geometry (Dangle) --> Check geometry (dangle)")

@DelazJ In fact, I think that we need to keep the long algorithm name, because when used inside a processing model we must see a full name. I really don't have an idea about title case or not for the name, that's why there is a mix... What do you think about this?

PS2: most of the time, the alg outputs are named using the alg name, i.e., Buffer --> Buffered, while here they all output "layer output" and "Error layer". It could be nice to have part of the alg name in the output(s)

That's a good point. We could add the check name like the algorithm name:

Errors layer (check dangle)
Output layer (check dangle)

DelazJ · 2025-04-28T05:44:28Z

In fact, I think that we need to keep the long algorithm name, because when used inside a processing model we must see a full name.

@Djedouas Sorry I do not get the point with the modeler.
IMHO No need to repeat the group name in the alg, which is already displayed at the top of the alg dialog.

The name should really be shortened for the reason you mentioned earlier, and maybe revisited to greatly reflect what the alg does. Do we really need the "check..." every time? What about e.g.:

Check geometry (Angle) --> Find(/Detect) small(er?) angles
Fix geometry (Angle) --> Delete small(er?) angles

I really don't have an idea about title case or not for the name, that's why there is a mix... What do you think about this?

Actually, they should not be in title case. If you walk through the Processing toolbox you'll notice that only the first letter is in upper case, unless there is an acronym. This is the rule to follow.

That's a good point. We could add the check name like the algorithm name:

Errors layer (check dangle)
Output layer (check dangle)

I'm not really fan; still long and not inline with existing practice. What about e.g.:

Dangle errors / Errors (dangle)
Dangle / Dangle locations -- the global target of this output is not always really clear to me
Dangle fixed / Undangled (assuming it is English)

Sorry to add: There is also that great function to display algs short help when you hover over them in the toolbox. That may be helpful. But I realize there are quite a few algorithms using it. 😢

If usual Processing algs reviewers could chime in...

Djedouas · 2025-05-06T14:59:06Z

@DelazJ I will start renaming the algorithms with clearer names, like "Detect small angles", and output layer, like "Small angle errors".

Sorry to add: There is also that great function to display algs short help when you hover over them in the toolbox. That may be helpful. But I realize there are quite a few algorithms using it. 😢

Do you mean implementing the QgsProcessingAlgorithm::shortDescription() method?

Djedouas · 2025-05-09T11:25:37Z

@DelazJ I renamed the algorithms and their output layers.

Check geometry algorithm names:

Fix geometry algorithm names:

Tooltip and short help explaining where the input error layers should come from:

It is not that easy, and some names will be kind of long, but more clear overall.

We can keep improving naming later if needed.

DelazJ · 2025-05-09T20:33:09Z

@DelazJ I renamed the algorithms and their output layers.

And you added the short description. ❤️
I didn't review in detail but from a user perspective, I really like the way this evolves. Thanks for your efforts @Djedouas
Looking at the above screenshots, I see a "Lines Intersections" alg, and we already had a "Line intersections" alg. Could we rename this into "Detect lines intersections"? And maybe rename the older into "Create line intersections"?

Djedouas · 2025-05-12T08:08:53Z

@DelazJ I tried not to add "Detect" to any new processing name, as it would be added to every name in the Check geometry section...

I prefer renaming the older from "Line intersections" to "Intersection (lines)" because there already is an "Intersection (multiple)" algorithm

Before

After

lbartoletti · 2025-05-13T05:07:02Z

Is it ok @DelazJ ? Would like to merge before the freeze (and finish this QEP :) ).

DelazJ · 2025-05-13T10:07:40Z

Is it ok @DelazJ ?

@lbartoletti I don't have resources to review this PR so as said above I'm fine with what I've seen so far. I think tweaks with wording will come when using or from documentation repo.

With regard to renaming the algorithm, I'm not very keen. I find it confusing/misleading to try to mimic the (multiple) formatting:

"Intersection" and "Intersection (multiple)" are two close algs that take common portions of features across layers, and they accept point, polygon and line layers. And they are coherent with the couples "Union - Union (multiple)" and "Difference - difference (multiple)".
the existing "Line intersections" alg does something quite different, creating point where features cross. With the renaming (and placement), people can think it does the same as above but only for lines. What about "Point at lines intersection" (not sure where the plural is supposed to be)?

lbartoletti · 2025-05-13T12:29:27Z

@lbartoletti I don't have resources to review this PR

@DelazJ don't worry, it's not about the review of this PR (I can guarantee the code is ok), but more for wording/user point of view :)

Djedouas · 2025-05-13T14:39:16Z

@DelazJ Yes I see your point.

So I renamed the "Lines intersections" into "Lines intersecting each other". So that it's like the other new algorithm named "Lines intersecting other layer", and somewhat a lot more different than the existing processing.

And we let the tooltips, the category, and the help do the rest of the job to correctly differentiate the two.

Does it seem ok for you?

- avoid using new keyword to avoid memory leaks - remove auto keyword - better formatting - better algorithm description

QList<QgsGeometryCheckError *> contains pointers to non-Qt objects. When the list is deleted, all pointers inside the list are deleted, but every data that each pointer points to is not automatically freed.

…tput

… with multipoint layers

DelazJ · 2025-05-13T14:44:34Z

Yes, thanks @Djedouas

lbartoletti · 2025-05-13T16:57:12Z

Many thanks @DelazJ for the review!

Djedouas added Processing Relating to QGIS Processing framework or individual Processing algorithms Geometry checker labels Apr 17, 2025

github-actions bot added this to the 3.44.0 milestone Apr 17, 2025

DelazJ reviewed Apr 17, 2025

View reviewed changes

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 45ff9b4 to 2bc04ce Compare April 18, 2025 08:35

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 2bc04ce to ed61539 Compare April 18, 2025 10:15

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 38b4cd4 to 89eb999 Compare April 24, 2025 08:33

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 89eb999 to 35d4332 Compare May 6, 2025 14:22

Djedouas closed this May 7, 2025

Djedouas reopened this May 7, 2025

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 9ae0bf8 to 4aa4402 Compare May 9, 2025 07:47

lbartoletti requested a review from DelazJ May 12, 2025 16:41

lbartoletti self-assigned this May 12, 2025

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 4e9bc8e to 8b7acc2 Compare May 13, 2025 14:34

Djedouas added 3 commits May 13, 2025 16:40

Improve angle check processing algorithm

6e93632

- avoid using new keyword to avoid memory leaks - remove auto keyword - better formatting - better algorithm description

Improve area check processing algorithm

00f646a

- avoid using new keyword to avoid memory leaks - remove auto keyword - better formatting - better algorithm description

Improve hole check processing algorithm

21ec406

- avoid using new keyword to avoid memory leaks - remove auto keyword - better formatting - better algorithm description

Djedouas added 25 commits May 13, 2025 16:40

Make duplicate check processing output layer optional

c4cc8f8

Make duplicate nodes check processing output layer optional

73042e6

Make follow boundaries check processing output layer optional

620c4f1

Make gap check processing output layer optional

3380239

Make hole check processing output layer optional

448a498

Make line intersection check processing output layer optional

3eee462

Make line layer intersection check processing output layer optional

e43388b

Make missing vertex check processing output layer optional

c781cc5

Make multipart check processing output layer optional

2e4b516

Make overlap check processing output layer optional

23e1f12

Make point covered by line check processing output layer optional

88b5246

Make point in polygon check processing output layer optional

c51f1b5

Make segment length check processing output layer optional

bd1a61d

Make self contact check processing output layer optional

5c7c25e

Make self intersection check processing output layer optional

1255f8d

Make sliver polygon check processing output layer optional

549f7c9

fix(algorithmcheckgeometry): add const for geometry error loop

3c1ca5e

fix(algorithmfixgeometry): add verification of feedback canceled

85e5db3

fix(algorithmcheckgeometry): avoid memory leaks

a666768

QList<QgsGeometryCheckError *> contains pointers to non-Qt objects. When the list is deleted, all pointers inside the list are deleted, but every data that each pointer points to is not automatically freed.

fix(algorithmgeometry): add tolerance help tooltip

2dc9990

fix(qgsalgorithmcheckgeometrypointcoveredbyline): remove duplicate ou…

8b01c57

…tput

fix(qgsalgorithmcheckgeometryangle): remove duplicate output

7b5dc0a

fix(qgsalgorithmcheckgeometrypointinpolygon): remove duplicate output

1908c45

chore(qgsalgorithmcheckgeometrymultimart): add test and compatibility…

e7fd5e5

… with multipoint layers

chore(check-fix-algorithms): improve names and help strings

c2544ea

Djedouas force-pushed the check_and_fix_algorithms_improvement branch from 8b7acc2 to c2544ea Compare May 13, 2025 14:40

lbartoletti approved these changes May 13, 2025

View reviewed changes

lbartoletti merged commit 2d49d1e into qgis:master May 13, 2025
31 checks passed

Uh oh!

Check and fix algorithms improvement #61531

Check and fix algorithms improvement #61531

Uh oh!

Conversation

Djedouas commented Apr 17, 2025

Description

Uh oh!

github-actions bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🪟 Windows Qt6 builds

🪟 Windows builds

Uh oh!

DelazJ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DelazJ Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

lbartoletti Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

lbartoletti Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

DelazJ Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Djedouas Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DelazJ Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Djedouas Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

DelazJ Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Djedouas commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DelazJ commented Apr 18, 2025

Uh oh!

Djedouas commented Apr 24, 2025

Uh oh!

DelazJ commented Apr 28, 2025

Uh oh!

Djedouas commented May 6, 2025

Uh oh!

Djedouas commented May 9, 2025

Uh oh!

DelazJ commented May 9, 2025

Uh oh!

Djedouas commented May 12, 2025

Uh oh!

lbartoletti commented May 13, 2025

Uh oh!

DelazJ commented May 13, 2025

Uh oh!

lbartoletti commented May 13, 2025

Uh oh!

Djedouas commented May 13, 2025

Uh oh!

DelazJ commented May 13, 2025

Uh oh!

lbartoletti commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 17, 2025 •

edited

Loading

Djedouas Apr 18, 2025 •

edited

Loading

DelazJ Apr 18, 2025 •

edited

Loading

Djedouas commented Apr 18, 2025 •

edited

Loading