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

Persist routes across style changes #2262

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Persist routes across style changes #2262

merged 1 commit into from
Nov 21, 2019

Conversation

LukasPaczos
Copy link

This PR fixes #2039 by caching the currently drawn routes/waypoints and reusing those same geometries immediately when the new style is loaded.

ezgif com-video-to-gif (7)

As you can see, the transitions are smooth, besides the first style change after NavigationMapRoute creation. @pozdnyakov and I are looking into the root cause.

@LukasPaczos LukasPaczos added the ⚠️ DO NOT MERGE PR should not be merged! label Nov 14, 2019
@pozdnyakov
Copy link

The issue is caused with style changes handling logic in mapbox-gl-native: atm it is considering sources order of the style, meaning that if the sources order of the updated style differs from the current sources order some sources are removed and re-added => flickering is observed. The preliminary fix is here I'm going to make it more elegant and shortly make a PR to the mapbox-gl-native repo with the appropriate fix.

@LukasPaczos
Copy link
Author

This is ready for review, but we cannot merge until @pozdnyakov's fix is available in a Map SDK's patch.

@codecov-io
Copy link

Codecov Report

Merging #2262 into master will increase coverage by 0.05%.
The diff coverage is 57.89%.

@@             Coverage Diff              @@
##             master    #2262      +/-   ##
============================================
+ Coverage     32.92%   32.97%   +0.05%     
- Complexity      621      627       +6     
============================================
  Files           143      143              
  Lines          5492     5519      +27     
  Branches        417      420       +3     
============================================
+ Hits           1808     1820      +12     
- Misses         3488     3505      +17     
+ Partials        196      194       -2

@pozdnyakov
Copy link

The corresponding GL-Native issue is at https://github.com/mapbox/mapbox-gl-native-team/issues/108 and the fix is at mapbox/mapbox-gl-native#15941

@LukasPaczos LukasPaczos force-pushed the lp-route-blinking branch 2 times, most recently from 7774d1e to 6d65f05 Compare November 20, 2019 17:34
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

This is looking good, great work @LukasPaczos

Nice refactoring out of MapboxMap into Style 💯 ❤️

As soon as we get the Maps final release we can merge here 🚀

@Guardiola31337 Guardiola31337 added this to the v0.43.0 milestone Nov 20, 2019
…er style change

This prevents the route layers from blinking when restoring the collection.
@LukasPaczos LukasPaczos removed the ⚠️ DO NOT MERGE PR should not be merged! label Nov 20, 2019
@LukasPaczos
Copy link
Author

Bumped Maps version to v8.5.1.

@@ -37,6 +37,7 @@ allprojects {
jcenter()
maven { url 'https://plugins.gradle.org/m2' }
maven { url 'https://mapbox.bintray.com/mapbox' }
// maven { url 'https://oss.jfrog.org/artifactory/oss-snapshot-local/' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing: Should we remove?

Copy link
Author

Choose a reason for hiding this comment

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

I left it for ease of use in the future. So that we don't have to look up the reference every time 😅

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.

Remove the logic that hides / removes route layers when the style changes
5 participants