ait-energy / qgis-edge-bundling

GNU General Public License v2.0
63 stars 14 forks source link

Tips for 3.0 port #12

Closed nyalldawson closed 6 years ago

nyalldawson commented 6 years ago

Hi Anita,

I had a quick look over the 3.0 port to see if there was any improvements I could suggest, since a lot of people will use this as a model for their ports. Here's a couple of small suggestions:

Line 160:

   features = source.getFeatures(QgsFeatureRequest(), QgsProcessingFeatureSource.FlagSkipGeometryValidityChecks

The skip geometry validity check flag isn't required here - I think it's been copied from an algorithm which HAS to keep invalid geometries (e.g. a repair geometry alg). You probably want to remove the flag so that users are warned if their geometries are invalid.

Line 169:

    for current, feat in enumerate(features):

Might be nice to check if the feedback is cancelled in this loop to allow responsive cancellation on big layers. (Could also potentially add a progress report here too)

Line 179:

        print(labels)

I'd suggest changing this to feedback.pushDebugInfo(...). It'll be shown in the processing log (and in future release recorded to a semi permanent history log)

Line 200

I'd suggest a cancel check in this loop too (they are very cheap)

Otherwise a perfect 3.0 port!

Also keen to hear if you see memory leaks while running this alg? (Unrelated to this particular script, but a bigger issue in 3.0)

anitagraser commented 6 years ago

Thank you for checking my port and for the recommendations! I had a good template in Alex Bruy's plugin.

anitagraser commented 6 years ago

Currently, pressing run opens a big popup window that blocks everything until the algorithm finishes. Is that an issue with my code or with Processing in general?

nyalldawson commented 6 years ago

Currently, pressing run opens a big popup window that blocks everything until the algorithm finishes. Is that an issue with my code or with Processing in general?

Processing in general - it's a modal progress dialog for algs which can't safely run in background threads.( Not sure why it defaults to such a large size). But a fix is very close to allow background execution by default for Python algs...

nyalldawson commented 6 years ago

Try with a new master build... Your algorithm should operate in a background task now!

anitagraser commented 6 years ago

"Run in background" looks great in nightly!

On the other hand, I cannot cancel the algorithm. If I click cancel, it takes as long as if I let it run uninterrupted and then shows "Execution failed"

nyalldawson commented 6 years ago

@anitagraser could it be getting caught up in the call to force_directed_eb, (during which there's no cancel checks)? If so, you could pass the feedback object into that method and also add cancel checks within it.

anitagraser commented 6 years ago

Good catch, thank you!