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

Support fractional minzoom on layers #5039

Merged
merged 2 commits into from
Jul 31, 2017
Merged

Conversation

asheemmamoowala
Copy link
Contributor

@asheemmamoowala asheemmamoowala commented Jul 25, 2017

Addresses : #2929

GeoJSON and Vector Tile sources are using a floor'ed zoom value when loading new tiles. This blocks tile data loaded for layers with fractional minzoom as it doesn't pass the test in worker_tile.js

The solution requires two changes

  • Pass the current zoom level to the worker tile
  • Worker tile should parse tiles that are at the integer min zoom floor
benchmark master b528524 2929-fractional-minzoom 6d1c21b
map-load 180 ms 113 ms
style-load 107 ms 88 ms
buffer 1,057 ms 926 ms
fps 60 fps 60 fps
frame-duration 6.1 ms, 0% > 16ms 6.3 ms, 1% > 16ms
query-point 0.94 ms 0.98 ms
query-box 53.86 ms 49.46 ms
geojson-setdata-small 4 ms 3 ms
geojson-setdata-large 145 ms 137 ms

Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@asheemmamoowala asheemmamoowala changed the title [WIP] Support fractional minzoom on layers Support fractional minzoom on layers Jul 25, 2017
@@ -190,7 +190,7 @@ class GeoJSONSource extends Evented implements Source {
type: this.type,
uid: tile.uid,
coord: tile.coord,
zoom: tile.coord.z,
zoom: this.map.transform.zoom,
Copy link
Contributor

Choose a reason for hiding this comment

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

@asheemmamoowala I'd love to avoid another point of dependency of sources on the map object (see also #3350).

Would an alternative to using the current map's zoom level be to keep using tile.coord.z, but change the test to something like if (this.zoom < floor(layer.minzoom)) continue;?

If I'm understanding this right, we want:

layer minzoom tile zoom tile needed?
4 3 (and below) no
4 4 (and above) yes
4.5 4 (and above) yes
4.5 3 (and below) no

"test": {
"width": 512,
"height": 512
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this test a tad by:

  • width & height: 64
  • center 0,0
  • data: literal geojson point

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

LGTM! Approving, notwithstanding a couple of small notes that I should have caught last time.

@@ -0,0 +1,38 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoot, I should have thought of this before, but: could we move this test to render-tests/regressions/mapbox-gl-js#2929?

"circle-color": "#EC8D8D",
"circle-radius": 4
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have two more layers with minzoom 4.5 and 5, that we would expect not to appear? (could use circle-translate so each layer's dot would be in a distinct location)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good improvement to the test, Thanks !

@asheemmamoowala asheemmamoowala merged commit ce68009 into master Jul 31, 2017
@asheemmamoowala asheemmamoowala deleted the 2929-fractional-minzoom branch July 31, 2017 18:54
@kkaefer
Copy link
Contributor

kkaefer commented Aug 4, 2017

This needs implementation in Native.

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