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

Optimize MapCropper #5701

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Optimize MapCropper #5701

merged 5 commits into from
Aug 9, 2023

Conversation

bmarchant
Copy link
Contributor

Certain output file formats will crop data on output so the MapCropper isn't necessary during the read operation, this used to just be OGR formats and now includes GeoJSON output files.

The MapCropper object deletes ways that fall outside of the bounds one-by-one using the RemoveWayByEid map visitor. When each way is deleted the OsmMap object rebuilds the _index member which is a costly operation. Instead of doing that, now the OsmMap::bulkRemoveWays method deletes a list of ways all at once and then rebuilds the index afterwards only one time.

Closes #5693

Copy link
Contributor

@mschicker mschicker left a comment

Choose a reason for hiding this comment

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

This looks good, and like it will be a great speedup!

I think you should add a comment in OsmMap.cpp to the bulkRemoveWays function header that explains the removeFully parameter. From reading the function, it looks like this switch decides whether or not the given ways should be removed from the map AND relations, or just the map.

Copy link
Contributor

@mschicker mschicker left a comment

Choose a reason for hiding this comment

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

Tastefully done! Approved!

@bmarchant bmarchant merged commit 7fe97ae into master Aug 9, 2023
@bmarchant bmarchant deleted the optimize_map_cropping_5693 branch August 9, 2023 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize MapCropper
2 participants