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

Distance is faster than finding shared edge #39997

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

m-kuhn
Copy link
Member

@m-kuhn m-kuhn commented Nov 12, 2020

Description

Followup #39961
This was still quite slow, this delays the expensive shared edge length test to when a fix requiring it is applied to make the checks themselves run fast.

CC @olivierdalang do you see an issue with this?

@github-actions github-actions bot added this to the 3.18.0 milestone Nov 12, 2020
@olivierdalang
Copy link
Contributor

Yes it makes sense... I'd also filter out the list of neighbours for the snapping that happens just after (lines 374).

Still, this all seems a bit risky, as it's also used in other methods like isEqual, or involvedFeatures, which I'm not sure how they are used ? Isn't there some room for improvement in QgsGeometryCheckerUtils::sharedEdgeLength itself ? Then, I haven't tested how bad perfs is currently, so can't really tell if it's worth the risk or not :-)

@m-kuhn
Copy link
Member Author

m-kuhn commented Nov 12, 2020

isEqual() is a utility function, important is that it returns stable results (which it still does), involvedFeatures is used to zoom to context (visually), so not something that leads to data corruption.

The speed impact is considerable, 4.5s vs 18s on a test data set here ~4 times.

I am sure that sharedEdgeLength could be improved, it's a myriad of nested loops, I tried to optimize it, unsuccessfully so far. So for the moment, this gives the best bang for the buck.

@nyalldawson
Copy link
Collaborator

@m-kuhn there's a geos method for extracting shared edges - maybe that is faster?

@m-kuhn
Copy link
Member Author

m-kuhn commented Nov 12, 2020

I could very well imagine that it's faster, but probably still slower than distance.
Still I'd like to continue to merge this and keep other improvements separate.

@nyalldawson
Copy link
Collaborator

Fair enough!

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.

3 participants