remove FAT_TILE_INDEX #700
Merged
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
The current code incorrectly assumes that a user will set -DFAT_TILE_INDEX for zooms >= 15. This is a change in behaviour vs v2.4, where -DFAT_TILE_INDEX was only needed for zooms >= 16. This removes -DFAT_TILE_INDEX and changes the buggy behaviour into a runtime exception. Future commits will shift us to teaching tilemaker.cpp to clamp the zoom to z14 if needed.
This is a mechanical renaming, I haven't yet taught tilemaker.cpp how to clamp basezooms that are greater than 14. Another performance implication: clip_cache maxes out its clipping at z14, so we'll do some redundant clipping when the map's basezoom is 15+. It doesn't have to be this way -- I just didn't want to thread the real basezoom into TileDataSource just for this case. This could be restored later, although I imagine in practice, the performance benefit of caching past z14 is fairly modest.
Clamp to 14 for indexing, then reconstitute tiles/objects at higher zooms from z14 when writing.
Extract an interface from TileCoordinatesSet, and provide two
implementations:
- PreciseTileCoordinatesSet: meant for z0..z14, read-write, accurate
- LossyTileCoordinatesSet: meant for z15+, read-only, extrapolates
from a lower zoom, but with false positives
After this commit, things stored in the small object index (e.g. points
and lines/polygons that span few tiles) work, even at z15+.
Still to do: change the large object index to support z15+. This
probably requires a different test PBF than Liechenstein...
|
I think this is ready for review now. @magicianlim reports that it resolves their issue with z16. I also generated a subregion of Ontario using the OpenMapTiles profile, and compared it using the script at #664 (comment) It's almost identical. Out of ~4,000 tiles, a single tile (z11/568/746) differs. The tile has an extra wood polygon feature. They look visually the same, and this tile is on the boundary of the clipping region for the input, so I'm assuming this is just an edge case that can be safely ignored -- e.g. perhaps something that previously got unioned into a multipolygon is now a standalone polygon. Before: After: |
|
Thank you! Seems to work well. |
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.
This fixes #682. Opening as a draft, as I've only done minimal testing so far.
The gist is: TileDataSource now operates in terms of
indexZoom, notbaseZoom. It's the lesser of14or your configured base zoom.TileCoordinatesSethas been turned into an interface, with two implementations:PreciseTileCoordinatesSet, which is the old implementation, and used for z0-z14LossyTileCoordinatesSet, which is used for z15+. It wraps aPreciseTileCoordinatesSetto answer whether a tile might have objects at z15+. It gives false positives, but never false negatives. False positives are OK, as they get clipped out when writing anyway.Some smoke testing makes me think this is working. I'll ask the reporter to give it a whirl, too.