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

Add source property to limit parent's tile overscale factor #16347

Merged
merged 4 commits into from
Apr 1, 2020
Merged

Add source property to limit parent's tile overscale factor #16347

merged 4 commits into from
Apr 1, 2020

Conversation

alexshalamov
Copy link
Contributor

The new property sets a limit for how much parent tile can be overscaled.

When a set of tiles for a current zoom level is being rendered and some of the ideal tiles that cover the screen are not yet loaded, parent tile could be used instead. This might introduce unwanted rendering side-effects, especially for raster tiles that are overscaled multiple times.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • tagged @mapbox/maps-android @mapbox/maps-ios @mapbox/core-sdk if this PR adds or updates a public API
  • tagged @mapbox/gl-js if this PR includes shader changes or needs a js port
  • apply needs changelog label if a changelog is needed (remove label when added)

/cc @mapbox/maps-android @mapbox/maps-ios @mapbox/core-sdk
/cc @asheemmamoowala

@astojilj
Copy link
Contributor

Is it related to JS's maxOverzooming and `minOverzooming?

@alexshalamov
Copy link
Contributor Author

Is it related to JS's maxOverzooming and `minOverzooming?

@astojilj I have to check JS implementation a bit more. Looks similar to maxOverzooming but on a single source level.

@alexshalamov alexshalamov force-pushed the alex_max_overscale_factor branch 2 times, most recently from 3cf0c92 to 362afb3 Compare March 29, 2020 19:17
@alexshalamov alexshalamov marked this pull request as ready for review March 30, 2020 08:05
@alexshalamov
Copy link
Contributor Author

@tobrun could you take a look at android changes. Draft java bindings are in mapbox/mapbox-gl-native-android#299

pozdnyakov
pozdnyakov approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

lgtm%comment

include/mbgl/style/source.hpp Show resolved Hide resolved
@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 31, 2020
chloekraw
chloekraw reviewed Mar 31, 2020
View reviewed changes
Copy link
Contributor

@chloekraw chloekraw left a comment

Choose a reason for hiding this comment

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

@alexshalamov thank you!

src/mbgl/renderer/tile_pyramid.cpp Outdated Show resolved Hide resolved
src/mbgl/renderer/tile_pyramid.hpp Outdated Show resolved Hide resolved
include/mbgl/style/source.hpp Show resolved Hide resolved
@chloekraw chloekraw mentioned this pull request Mar 31, 2020
Open
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Mar 31, 2020
@alexshalamov alexshalamov force-pushed the alex_max_overscale_factor branch 2 times, most recently from fbc24c2 to 70c6722 Compare March 31, 2020 07:33
@astojilj
Copy link
Contributor

What happens with panZoom tiles in tilePyramid when maxOverscaleFactor is < sourcePrefetchZoomDelta?

@alexshalamov
Copy link
Contributor Author

alexshalamov commented Mar 31, 2020 •
edited
Loading

What happens with panZoom tiles in tilePyramid when maxOverscaleFactor is < sourcePrefetchZoomDelta?

@astojilj In that case maxOverscaleFactor would act as a threshold for sourcePrefetchZoomDelta. Do you think we should still pre-fetch tiles that are not supposed to be rendered?

@astojilj
Copy link
Contributor

What happens with panZoom tiles in tilePyramid when maxOverscaleFactor is < sourcePrefetchZoomDelta?

@astojilj In that case maxOverscaleFactor would act as a threshold for sourcePrefetchZoomDelta. Do you think we should still pre-fetch tiles that are not supposed to be rendered?

I don't think we should prefetch them. We should also notify users setting sourcePrefetchZoomDelta that prefetch is not active, e.g. assert or warning.

@asheemmamoowala
Copy link
Contributor

@alexshalamov Is there a case to be made to put this in the style specification since it is a per source property that can be used with sparse tilesets?

@alexshalamov alexshalamov force-pushed the alex_max_overscale_factor branch 2 times, most recently from 2112b8e to 87d0286 Compare April 1, 2020 08:42
@alexshalamov
Copy link
Contributor Author

@asheemmamoowala I think so, it may be useful for gl-js use-cases, for example when there is a raster tile in source cache and we quickly zoom-in and if connectivity is poor, highly overscaled tile would be shown to a user. As @astojilj noted above gl-js has some sort of thresholds already.

@astojilj added warning 9d593f2#diff-583602ba6bed5602d365e3b1055ecdd5R12

@alexshalamov alexshalamov merged commit 4bdb3ce into master Apr 1, 2020
@alexshalamov alexshalamov deleted the alex_max_overscale_factor branch April 1, 2020 20:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chloekraw chloekraw chloekraw left review comments

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@tobrun tobrun Awaiting requested review from tobrun

@astojilj astojilj Awaiting requested review from astojilj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexshalamov @astojilj @asheemmamoowala @pozdnyakov @chloekraw