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

Add option to prefetch low-resolution tiles #14031

Merged
merged 5 commits into from
Mar 6, 2019
Merged

Conversation

m-stephen
Copy link
Contributor

Closes: #9438

We got a loading issue report from our customer.
I've added a boolean-typed prefetchesTiles property to MGLMapView to fix this issue.

Could you help to review these codes? @1ec5 @friedbunny

@m-stephen m-stephen requested review from a team, 1ec5 and friedbunny March 5, 2019 03:59
@m-stephen m-stephen self-assigned this Mar 5, 2019
@m-stephen m-stephen added this to the release-l milestone Mar 5, 2019
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Suggested changelog entry:

  • Added an MGLMapView.prefetchesTiles property that you can disable if you don’t want to prefetch simplified tiles as a performance optimization. (#14031)

@tmpsantos, are you comfortable with the approach of making this a Boolean option, as I proposed in #9438 (comment), or do you think developers need the flexibility of specifying a zoom level to prefetch?

else
{
//Reset to default value.
_mbglMap->setPrefetchZoomDelta(4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #9438 (comment), use the constant DEFAULT_PREFETCH_ZOOM_DELTA instead of hard-coding the literal number 4. Also, the if/else statement can be written more succinctly as a ternary expression: setPrefetchZoomDelta(prefetchesTiles ? DEFAULT_PREFETCH_ZOOM_DELTA : 0).

platform/ios/src/MGLMapView.mm Show resolved Hide resolved

Loads tiles at a lower zoom-level to pre-render a low resolution preview while more detailed tiles are loaded.

Default is YES.
Copy link
Contributor

Choose a reason for hiding this comment

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

The developer may be unfamiliar with the concept of prefetching simplified tiles, so here’s an attempt at making the mechanism easy to understand:

When this property is set to YES, the map view prefetches loads tiles designed for a low zoom level and displays them until receiving more detailed tiles for the current zoom level. The prefetched tiles typically contain simplified versions of each shape, improving the map view’s perceived performance. The default value of this property is YES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. That's a better one. I'll change to that.

@m-stephen m-stephen requested a review from 1ec5 March 5, 2019 06:45
@m-stephen
Copy link
Contributor Author

m-stephen commented Mar 5, 2019

Suggested changelog entry:

  • Added an MGLMapView.prefetchesTiles property that you can disable if you don’t want to prefetch simplified tiles as a performance optimization. (#14031)

@tmpsantos, are you comfortable with the approach of making this a Boolean option, as I proposed in #9438 (comment), or do you think developers need the flexibility of specifying a zoom level to prefetch?

FYI, this has been implemented only with a boolean value on Android side. @1ec5

@tmpsantos
Copy link
Contributor

tmpsantos commented Mar 5, 2019

@tmpsantos, are you comfortable with the approach of making this a Boolean option, as I proposed in #9438 (comment), or do you think developers need the flexibility of specifying a zoom level to prefetch?

I'm ok with the boolean option for the sake of consistence with Android.

@@ -5,6 +5,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
## master

* Client-side text rendering of CJK ideographs is now enabled by default. ([#13988](https://github.com/mapbox/mapbox-gl-native/pull/13988))
* Added an MGLMapView.prefetchesTiles property that you can disable if you don’t want to prefetch simplified tiles as a performance optimization. ([#14031](https://github.com/mapbox/mapbox-gl-native/pull/14031))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the backticks help jazzy automatically link to the property’s documentation:

Suggested change
* Added an MGLMapView.prefetchesTiles property that you can disable if you don’t want to prefetch simplified tiles as a performance optimization. ([#14031](https://github.com/mapbox/mapbox-gl-native/pull/14031))
* Added an `MGLMapView.prefetchesTiles` property that you can disable if you don’t want to prefetch simplified tiles as a performance optimization. ([#14031](https://github.com/mapbox/mapbox-gl-native/pull/14031))

@1ec5 1ec5 added the iOS Mapbox Maps SDK for iOS label Mar 6, 2019
@m-stephen m-stephen merged commit 383fde6 into master Mar 6, 2019
@m-stephen m-stephen deleted the stephen-pretch-tiles branch March 6, 2019 04:02
@friedbunny friedbunny added the performance Speed, stability, CPU usage, memory usage, or power usage label Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to prefetch low-resolution tiles
4 participants