remove PossiblyKnownTagValue; fixup LayerAsCentroid; fixup way_keys interaction with LayerAsCentroid; fixup Intersects #641
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When I replaced systemed#604 with systemed#626, I botched extracting this part of the code. I had the trait, which taught kaguya how to serialize `PossiblyKnownTagValue`, but I missed updating the parameter type of `Attribute` to actually use it, so it was a no-op. This PR restores the behaviour of avoiding string copies, but now that we have protozero's data_view class, we can use that rather than our own weirdo struct.
Eep, two fixes here as well: - I had rejigged how the skipping of LayerAsCentroid's algorithm argument worked; this rejigging ultimately broke it entirely, as `i` would never get incremented. - If `way_keys` is provided, we are no longer guaranteed that we'll have stored the `label` node of the relation
cldellow
changed the title
Previousy, we were excluding the last row and column when searching the bitmap. Most obvious with nodes, as they'd always be skipped.
cldellow
changed the title
- apply off-by-one fix to indexing phase (not just querying phase) - make both phases robust against invalid input coordinates
|
Thank you! Merged into v3. |
cldellow
added a commit
to cldellow/tilemaker
that referenced
this pull request
Oct 1, 2024
In systemed#635, we introduced a way to avoid calling Boost's R-Tree for `Intersects` when we knew the underlying z14 tile had no shapes in it. This was a perf win (although not as big as was claimed in that PR -- there was a bug, fixed in systemed#641, which was the source of much of the speedup). This improves on that work for the case of large, irregularly shaped polygons. Currently, for each shape in the shapefile/GeoJSON file, we compute the set of z14 tiles in its bounding box, and set a bit in a bitset for each of those tiles. This is fast to compute, but lossy. For example, the bounding box of https://www.openstreetmap.org/relation/9848163#map=12/39.9616/-105.2197 covers much of the city of Boulder, even though the park itself is entirely outside of Boulder. Instead, this PR now uses 2 bits per z14 tile. The loading of shapes is as before: the first bit is set to 1 if the z14 tile is contained in a bounding box of a shape. The second bit is initialized to 0. At `Intersects` time, we'll lazily refine the index, but only for those tiles that are actually being queried. We'll actually check if the z14 tile intersects any of the shapes, and then either set the second bit to 1, or clear the first bit. This doubles the memory requirement for indexing from 32MB to 64MB per layer. I've left some counters in - I'm going to fiddle with some other ideas to see if there are other speedups. I'll remove the counters before making a PR.
cldellow
added a commit
to cldellow/tilemaker
that referenced
this pull request
Oct 1, 2024
In systemed#635, we introduced a way to avoid calling Boost's R-Tree for `Intersects` when we knew the underlying z14 tile had no shapes in it. This was a perf win (although not as big as was claimed in that PR -- there was a bug, fixed in systemed#641, which was the source of much of the speedup). This improves on that work for the case of large, irregularly shaped polygons. Currently, for each shape in the shapefile/GeoJSON file, we compute the set of z14 tiles in its bounding box, and set a bit in a bitset for each of those tiles. This is fast to compute, but lossy. For example, the bounding box of https://www.openstreetmap.org/relation/9848163#map=12/39.9616/-105.2197 covers much of the city of Boulder, even though the park itself is entirely outside of Boulder. Instead, this PR now uses 2 bits per z14 tile. The loading of shapes is as before: the first bit is set to 1 if the z14 tile is contained in a bounding box of a shape. The second bit is initialized to 0. At `Intersects` time, we'll lazily refine the index, but only for those tiles that are actually being queried. We'll actually check if the z14 tile intersects any of the shapes, and then either set the second bit to 1, or clear the first bit. This doubles the memory requirement for indexing from 32MB to 64MB per layer. I've left some counters in - I'm going to fiddle with some other ideas to see if there are other speedups. I'll remove the counters before making a PR. Related: the code previously treated the lats as non-projected values. This was wrong, but didn't affect correctness, since both the indexing and querying code made the same error. This commit changes it to correctly use latp.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When I replaced #604 with #626, I botched extracting this part of the code. I had the trait, which taught kaguya how to serialize
PossiblyKnownTagValue, but I missed updating the parameter type ofAttributeto actually use it, so it was a no-op.This PR restores the behaviour of avoiding string copies, but now that we have protozero's data_view class, we can use that rather than our own weirdo struct.
It also fixes an unrelated test build error in the way store test.
...and two issues to do with LayerAsCentroid's support for relation members.
...and an issue with the Intersects PR.