Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Rationalize Resource initialization #3740

Merged
merged 3 commits into from
Jan 30, 2016
Merged

Conversation

jfirebaugh
Copy link
Contributor

Extract URL construction code for reuse in #3715.

@jfirebaugh
Copy link
Contributor Author

This is a partial reroll of #3675 (the least risky parts).

Resource(Kind kind_, const std::string& url_)
struct TileData {
std::string urlTemplate;
float pixelRatio;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a float? Are we supporting assets that have a fractional pixel ratio?

Copy link
Member

Choose a reason for hiding this comment

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

I know we support fractional pixel ratios when rendering, but should we always round up to the nearest integer pixel ratio when requesting assets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit of a grey area because technically we don't support anything other than @2x, and that only for Mapbox sources. So if the parameter is to represent strictly what's supported now, it could be an integer or even bool retinaTiles.

In terms of offline, what I am planning to do is extract a tileCover function that takes a display pixel ratio, display bounding box, display zoom range, source type, source tile size, and SourceInfo, and returns a std::vector<Resource>, where the Resources are Resource::Kind::Tile with the appropriate TileData. (Or I might extract Resource::TileData to a non-nested class and return a vector of that.)

The tileCover function will:

  • Determine the appropriate pixel ratio for the tile requests, given the display pixel ratio and supported pixel ratios of the source (currently 1.0 and 2.0 only for Mapbox sources)
  • Determine the set of appropriate source zoom levels for the tile requests, given the display zoom range and taking into account
    • source min and max zoom
    • tile size normalization (i.e. requesting z1 256px tiles at display zoom level 0)
    • different raster/vector float z ⇢ integer z rounding behavior
  • Determine the appropriate set of tile [x, y]s for each zoom level

Source will use this function to calculate the covering tiles for the current bounding box and zoom range (a degenerate range where min=max). Offline will use this function to calculate the appropriate tiles to download given the region definition.

This function will be responsible for rounding to the nearest integer pixel when that's all the source supports. It should be the only place we need to change if we add support for additional pixel ratios in the future (@3x or even non-integer ratios).

The only other place where the type of TileData::pixelRatio will matter is in the database schema. There I think I'd like to keep it floating point, just for future flexibility, even though right now the only values will be 1.0 and 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm comfortable saying we'll never support non-integer multipliers for tile resolution. I'll make it an integer here and in the database schema.

@jfirebaugh jfirebaugh force-pushed the resource-initialization branch from d2d1cee to ed013ff Compare January 29, 2016 21:23
@jfirebaugh jfirebaugh force-pushed the resource-initialization branch from ed013ff to b41a73e Compare January 29, 2016 23:32
@jfirebaugh jfirebaugh merged commit 2920020 into master Jan 30, 2016
@jfirebaugh jfirebaugh deleted the resource-initialization branch January 30, 2016 01:34
ansis added a commit that referenced this pull request Dec 15, 2016
ported from -js: 0b5520fa5ab2a4659d80dcffa8b035a0d84fe1ca

This should fix the issue behind #2270 and remove the need for the hack
added in #3740.
ansis added a commit that referenced this pull request Dec 21, 2016
ported from -js: 0b5520fa5ab2a4659d80dcffa8b035a0d84fe1ca

This should fix the issue behind #2270 and remove the need for the hack
added in #3740.
jfirebaugh pushed a commit that referenced this pull request Dec 21, 2016
ported from -js: 0b5520fa5ab2a4659d80dcffa8b035a0d84fe1ca

This should fix the issue behind #2270 and remove the need for the hack
added in #3740.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants