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

Configurable tile sizes with a metatile #342

Merged
merged 4 commits into from
Jul 13, 2018

Conversation

zerebubuth
Copy link
Member

The previous behaviour to generate all the tiles of different sizes within a metatile. That worked great while we were generating 512px and 256px tiles in a size 2 metatile. It kinda worked while we were generating 1024px, 512px and 256px tiles in a size 4 metatile. However, moving to a size 8 metatile means we're duplicating information across 4 different sizes of tile - two of which we don't use ever, and the last one we rarely use.

This change introduces configurable tile sizes within a metatile. For example, it's possible to specify just 256, just 512 or both. This will cut down on extra, unwanted data in metatiles.

There are some special cases to this:

At maximum zoom

For maximum nominal zoom tiles, smaller tiles are always included even if not configured. This means that if the maximum nominal zoom is 16 and the configured tile-sizes: [512], the metatile containing nominal zoom 16 (e.g: at zoom 13 for 8x8 metatiles) will contain both 512px and 256px tiles. This allows tapalcatl to treat the smaller tiles as "overzooms" for the larger tiles.

At minimum zoom

For zoom zero tiles, unused "slots" in the metatile for larger tiles will be used by lower zoom tiles. For example, if the only configured tile size is 512px and the metatile size is 8x8, then we would expect the 0/0/0 metatile to contain the 4x4 2/x/y 512px tiles only. However, this means we don't have any tiles for requests at zoom levels 0 and 1. So we also render the 512px tiles at lower nominal zooms to provide those.

Normally, a metatile would only contain tiles all at the same nominal zoom. This change breaks that, but means we don't have to add negative zoom tiles to the already complicated mental model of zooms, metatiles, nominal zooms, etc...

This last change fixes an issue with not generating zoom 0: #341.

@zerebubuth zerebubuth requested review from rmarianski and iandees July 4, 2018 17:45
@nvkelso
Copy link
Member

nvkelso commented Jul 4, 2018 via email


def process(coord, metatile_zoom, fetch_fn, layer_data, post_process_data,
formats, buffer_cfg, output_calc_mapping, max_zoom,
cfg_tile_sizes):
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, we might want to consider making the api/entry point be the processor instance itself in the future that clients called. I think it would be extremely rare that in a course of a python process running we'd need to support processing across different zoom configurations and the abstraction you made here seems like a cleaner interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went backwards and forwards on the interface. Overall, I think a single function call is cleaner, but I had trouble separating out the exceptions in the fetch phase from the processing phase. That's a helpful distinction, so I thought it would be good to keep.

Anyone know how to cleanly support try:/except: changing or wrapping the exception while keeping the stack trace intact? I feel like this is something I used to know, but have forgotten 😕

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just meant that we can keep the single function call, just have most of the "config" state in the closure/instance.

@rmarianski rmarianski merged commit b672f74 into master Jul 13, 2018
@rmarianski rmarianski deleted the zerebubuth/configure-tile-sizes branch July 13, 2018 18:41
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