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

[core] Implement "smart setStyle" #9256

Merged
merged 4 commits into from
Jun 15, 2017
Merged

[core] Implement "smart setStyle" #9256

merged 4 commits into from
Jun 15, 2017

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Jun 13, 2017

Implements automatic "smart setStyle" behavior for existing style-setting APIs: Map::setStyleURL and Map::setStyleJSON. Both of these APIs will now smoothly transition between the prior and new style (providing equivalent layers/sources/etc. use the same IDs).

gl

Besides the obvious behavioral change, several changes are worth calling out for consideration of their downstream impact:

  • Paint property mutations now transition by default over a 300ms duration. This can be overridden via Map::setTransitionOptions (already existed), or within the style via the "transition" property (support added in this PR). A 300ms transition was always the intended default behavior according to the style specification, and is what's implemented in gl-js, but for whatever reason was not the default for gl-native. This change is included because it makes smart setStyle look cool.
  • For Map::setStyleURL, the prior style remains in effect until the asynchronous network request for the new style has completed. Previously, the map would have an empty style in the interim. This caused flashing and flickering which, if not fixed, would be especially noticeable with smart setStyle.
  • The internal Style object held by Map has a lifetime coincident with the Map lifetime itself. This was the most straightforward implementation of the prior point, but I think it's also a reasonable change on the grounds that it eliminates corner cases from the API. Specifically:
    • A Map now actually has an (empty) style from the get go. Previously, runtime styling APIs were explicitly no-ops or errors prior to a style being loaded.
    • Map::setStyleURL/setStyleJSON update the existing internal style object in place, rather than creating a new one. There isn't currently a Map::getStyle method, so this is unlikely to be an externally visible change. If we add Map::setStyle(std::unique_ptr<Style>), it will replace the existing style object, rather than updating it in place.

Fixes #7893.

TODO:

  • Enable set-style-* integration tests and make sure they pass

@jfirebaugh jfirebaugh added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 13, 2017
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Jun 13, 2017
@brunoabinader
Copy link
Member

Beautiful @jfirebaugh!

@tmpsantos
Copy link
Contributor

RIP style classes

@jfirebaugh jfirebaugh removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 14, 2017
@jfirebaugh
Copy link
Contributor Author

Updated the description and this is ready for review.

@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2017

Paint property mutations now transition by default over a 300ms duration.

#9270 tracks documenting this default on iOS and macOS.

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.

within the style via the "transition" property (support added in this PR)

Does the root property affect the transition to or from the style it’s on? If the former, then is it the case that, by the time the developer has the opportunity to set this property, the transition will’ve already commenced? If the latter, this documentation would need to be updated.

@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2017

A Map now actually has an (empty) style from the get go. Previously, runtime styling APIs were explicitly no-ops or errors prior to a style being loaded.

-[MGLMapView styleURL] currently compensates by returning the “default style URL” (currently Streets v10). I wonder if we should account for this change by making the property nullable instead of null-resettable or (if we’re concerned about backwards-compatibility) return about:blank as the URL. The iOS and macOS SDKs automatically load Streets unless otherwise configured, so this edge case would only occur while loading the initial style or if Streets fails to load (due to a missing access token).

/cc @boundsj

@jfirebaugh
Copy link
Contributor Author

Does the root property affect the transition to or from the style it’s on?

The value from the new style takes precedence, so the former.

If the former, then is it the case that, by the time the developer has the opportunity to set this property, the transition will’ve already commenced?

No, the user has opportunities to override the style default. When using setStyleJSON, you can do:

map.setStyleJSON(newJSON);
map.setTransitionOptions({ Duration::zero() });

When using setStyleURL, you can call setTransitionOptions in an onDidFinishLoadingStyle handler.

@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2017

Sounds good. In that case, the MGLStyle.transition documentation should mention that it affects inter-style transitions as well.

A more natural way to expose transition options to the SDKs would involve something like -[MGLMapView setStyleURL:duration:delay:], since a transition is by definition distinct from the start and end states. With these changes, it would be feasible to implement such a method at the SDK level, but we might need to hold off until #6386 (comment) is completely unblocked.

Copy link
Contributor

@lbud lbud left a comment

Choose a reason for hiding this comment

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

🎉

@jfirebaugh jfirebaugh force-pushed the smart-set-style branch 2 times, most recently from f27cb58 to fa6b7ad Compare June 14, 2017 22:02
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.

Beautiful 🎉

@jfirebaugh jfirebaugh merged commit cdc5f8f into master Jun 15, 2017
@jfirebaugh jfirebaugh deleted the smart-set-style branch June 15, 2017 14:43
@nitrag
Copy link
Contributor

nitrag commented Sep 24, 2017

So to clarify, mapViewDidFinishLoadingMap won't fire after a style change? Only onDidFinishLoadingStyle?

Referencing discussion in: #6180

My current code:

func mapViewDidFinishLoadingMap(_ mapView: MGLMapView) {
        //Fired each time map is changed (changing styles)
        //
        // Set up Map Data/Styles only once!
        //
        guard let style = mapView.style else {
            print("Can't load styles yet! You shouldn't be seeing this!!")
            return
        }

        //add sources
        //add layers
        ....
}

@qrh672114236
Copy link

setStyleUrl

{y.mango_measure}[Setup]: loading style failed: Attempt to invoke virtual method 'java.lang.String okhttp3.HttpUrl.host()' on a null object reference

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement 'smart' setStyle using diff-and-patch technique
9 participants