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

[core] Annotations refactor #2420

Merged
merged 9 commits into from
Sep 28, 2015
Merged

[core] Annotations refactor #2420

merged 9 commits into from
Sep 28, 2015

Conversation

jfirebaugh
Copy link
Contributor

Significant refactoring of annotations. Annotation-related code that was in MapContext and StyleParser is now in AnnotationManager.

Includes additional tests for line and fill annotations.

Cumulates in rewriting the annotation invalidation strategy:

  • First, move style mutation code out of StyleParser and into AnnotationManager, coalescing it with the mutation code for shape layers.
  • Second, allow AnnotationManager to keep track of stale tiles entirely internally; avoid passing sets of TileIDs around.
  • Third, correct the logic for invalidating the shape source. Since AnnotationManager does not track shape invalidations on a tile-by-tile basis, don't try to invalidate the shape source tile-by-tile.

Fixes #1675 - @picciano I used code derived from your test app to confirm. If you have time to confirm on your app, that would be awesome.
Probably fixes #2322 - @friedbunny can you confirm?
Probably fixes #2095 - @erf can you confirm?
Might partially fix #1488 - I will add a test and confirm.

@picciano
Copy link

@jfirebaugh I will do that first thing tomorrow. thanks

@jfirebaugh
Copy link
Contributor Author

Hmm, definitely seeing some issues with this after testing some more scenarios. Stand by.

@erf
Copy link
Contributor

erf commented Sep 25, 2015

Running the app fails on the latest 'master' or 'annotations' branch, i am told 'couldn't find "libmapbox-gl.so"', even if i have built it using 'make android' and i can see it in the 'jniLibs/armeabi-v7a' folder. I went back to 4b58760 and then it worked ok. I'm running the latest cyanogen mod on a sony xperia z1.

09-25 10:36:45.520    1535-1535/? E/AndroidRuntime﹕ FATAL EXCEPTION: main
    Process: com.mapbox.mapboxgl.testapp, PID: 1535
    java.lang.RuntimeException: Unable to start activity ComponentInfo{com.mapbox.mapboxgl.testapp/com.mapbox.mapboxgl.testapp.MainActivity}: android.view.InflateException: Binary XML file line #33: Error inflating class com.mapbox.mapboxgl.views.MapView
            at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2357)
            at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2419)
            at android.app.ActivityThread.access$900(ActivityThread.java:154)
            at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1321)
            at android.os.Handler.dispatchMessage(Handler.java:102)
            at android.os.Looper.loop(Looper.java:135)
            at android.app.ActivityThread.main(ActivityThread.java:5293)
            at java.lang.reflect.Method.invoke(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:372)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)
     Caused by: android.view.InflateException: Binary XML file line #33: Error inflating class com.mapbox.mapboxgl.views.MapView
            at android.view.LayoutInflater.createView(LayoutInflater.java:633)
            at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:743)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:806)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:504)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:414)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:365)
            at android.support.v7.app.AppCompatDelegateImplV7.setContentView(AppCompatDelegateImplV7.java:255)
            at android.support.v7.app.AppCompatActivity.setContentView(AppCompatActivity.java:109)
            at com.mapbox.mapboxgl.testapp.MainActivity.onCreate(MainActivity.java:84)
            at android.app.Activity.performCreate(Activity.java:5990)
            at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1106)
            at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2310)
            at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2419)
            at android.app.ActivityThread.access$900(ActivityThread.java:154)
            at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1321)
            at android.os.Handler.dispatchMessage(Handler.java:102)
            at android.os.Looper.loop(Looper.java:135)
            at android.app.ActivityThread.main(ActivityThread.java:5293)
            at java.lang.reflect.Method.invoke(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:372)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)
     Caused by: java.lang.reflect.InvocationTargetException
            at java.lang.reflect.Constructor.newInstance(Native Method)
            at java.lang.reflect.Constructor.newInstance(Constructor.java:288)
            at android.view.LayoutInflater.createView(LayoutInflater.java:607)
            at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:743)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:806)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:504)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:414)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:365)
            at android.support.v7.app.AppCompatDelegateImplV7.setContentView(AppCompatDelegateImplV7.java:255)
            at android.support.v7.app.AppCompatActivity.setContentView(AppCompatActivity.java:109)
            at com.mapbox.mapboxgl.testapp.MainActivity.onCreate(MainActivity.java:84)
            at android.app.Activity.performCreate(Activity.java:5990)
            at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1106)
            at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2310)
            at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2419)
            at android.app.ActivityThread.access$900(ActivityThread.java:154)
            at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1321)
            at android.os.Handler.dispatchMessage(Handler.java:102)
            at android.os.Looper.loop(Looper.java:135)
            at android.app.ActivityThread.main(ActivityThread.java:5293)
            at java.lang.reflect.Method.invoke(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:372)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)
     Caused by: java.lang.UnsatisfiedLinkError: dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/com.mapbox.mapboxgl.testapp-1/base.apk"],nativeLibraryDirectories=[/vendor/lib, /system/lib]]] couldn't find "libmapbox-gl.so"
            at java.lang.Runtime.loadLibrary(Runtime.java:366)
            at java.lang.System.loadLibrary(System.java:988)
            at com.mapbox.mapboxgl.views.NativeMapView.(NativeMapView.java:37)
            at com.mapbox.mapboxgl.views.MapView.initialize(MapView.java:364)
            at com.mapbox.mapboxgl.views.MapView.(MapView.java:318)
            at java.lang.reflect.Constructor.newInstance(Native Method)
            at java.lang.reflect.Constructor.newInstance(Constructor.java:288)
            at android.view.LayoutInflater.createView(LayoutInflater.java:607)
            at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:743)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:806)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.rInflate(LayoutInflater.java:809)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:504)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:414)
            at android.view.LayoutInflater.inflate(LayoutInflater.java:365)
            at android.support.v7.app.AppCompatDelegateImplV7.setContentView(AppCompatDelegateImplV7.java:255)
            at android.support.v7.app.AppCompatActivity.setContentView(AppCompatActivity.java:109)
            at com.mapbox.mapboxgl.testapp.MainActivity.onCreate(MainActivity.java:84)
            at android.app.Activity.performCreate(Activity.java:5990)
            at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1106)
            at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2310)
            at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2419)
            at android.app.ActivityThread.access$900(ActivityThread.java:154)
            at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1321)
            at android.os.Handler.dispatchMessage(Handler.java:102)
            at android.os.Looper.loop(Looper.java:135)
            at android.app.ActivityThread.main(ActivityThread.java:5293)
            at java.lang.reflect.Method.invoke(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:372)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)

@jfirebaugh
Copy link
Contributor Author

Ok, I don't think that Android issue is caused by these changes.

I fixed a bug with style cascading that was causing shapes to appear black. Should be good to test now.

This does not fix #1488 -- points are still invisible and there are messages like "Can't find sprite named 'com.mapbox.sprites.51'" in the output when you switch a style after adding point annotations.

@friedbunny
Copy link
Contributor

As of e9ece31 this does fix #2322. 👍

@mikemorris mikemorris mentioned this pull request Sep 25, 2015
Merged
@erf
Copy link
Contributor

erf commented Sep 25, 2015

I build and run ok on latest master 81fde42

incanus
incanus reviewed Sep 28, 2015
View reviewed changes
src/mbgl/map/map_context.cpp
@@ -420,7 +313,6 @@ void MapContext::onResourceLoadingFailed(std::exception_ptr error) {
}

void MapContext::onSpriteStoreLoaded() {
updateAnnotationTilesIfNeeded();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't necessarily need onSpriteStoreLoaded() anymore. This was added in 0d899df to address #1675, but this PR obsoletes that.

@incanus
Copy link
Contributor

incanus commented Sep 28, 2015

This does not fix #1488 -- points are still invisible and there are messages like "Can't find sprite named 'com.mapbox.sprites.51'" in the output when you switch a style after adding point annotations.

Is it possible to just send another Update::Annotations after style changes somehow to fix this?

@incanus
Copy link
Contributor

incanus commented Sep 28, 2015

Seeing odd staleness with the built-in 1,000 points test:

anigif-1443465578

@incanus
Copy link
Contributor

incanus commented Sep 28, 2015

Same for 100 points, actually.

@jfirebaugh
Copy link
Contributor Author

Is it possible to just send another Update::Annotations after style changes somehow to fix this?

No, fixing this requires some bigger changes: keeping a record of which sprite images were added via Map::setSprite and then re-add them to the new style. Or, better, removing Map::setSprite and associating the image directly with the annotation object.

@jfirebaugh
Ensure that LiveTileData can be reparsed
028abaa 
Annotation tiles may become partially parsed just like regular tiles,
for example if a point annotation is added to the map before the style's
sprite has been loaded. In such cases, they need to be reparsed or the
annotation will not be rendered. Previously, the code path for reparsing
would be short-circuited by a dynamic_cast<VectorTileData*> followed by
a null check. This commit removes that case and adds (back) a virtual
reparse method to the TileData interface.
@jfirebaugh
Add some shape annotation tests
931c56a
@jfirebaugh
Inline AnnotationManager::addTileFeature
dd0e6e0
@jfirebaugh
Move entire loop body to helper function
bc9aaa5
@jfirebaugh
Move Annotation class to cpp file
588fed9
@jfirebaugh
map/annotation.* ⇢ annotation/annotation_manager.*
0c4c7ed
@jfirebaugh
Move updateAnnotationTiles[IfNeeded] to AnnotationManager
24a5834
@jfirebaugh
Rewrite annotation invalidation strategy
af392b9 
First, move style mutation code out of StyleParser and into AnnotationManager,
coalescing it with the mutation code for shape layers.

Second, allow AnnotationManager to keep track of stale tiles entirely
internally. There's no reason to pass sets of TileIDs around.

Third, correct the logic for invalidating the shape source. Since
AnnotationManager does not track shape invalidations on a tile-by-tile basis,
don't try to invalidate the shape source tile-by-tile.

Fixes #1675
Fixes #2322
Fixes #2095
@jfirebaugh
Copy link
Contributor Author

Seeing odd staleness with the built-in 1,000 points test

Found, fixed, added a test.

jfirebaugh added a commit that referenced this pull request Sep 28, 2015
@jfirebaugh
Merge pull request #2420 from mapbox/annotations
6d357eb 
[core] Annotations refactor
@jfirebaugh jfirebaugh merged commit 6d357eb into master Sep 28, 2015
@jfirebaugh jfirebaugh deleted the annotations branch September 28, 2015 21:39
@picciano
Copy link

@jfirebaugh This is working in the TallyGo app now. Thanks much.

@incanus
Copy link
Contributor

incanus commented Sep 28, 2015

Found, fixed, added a test.

Where at @jfirebaugh? I'm only seeing 0156ac7 as recent and not seeing how this works.

But I am seeing the fix!

@jfirebaugh
Copy link
Contributor Author

I messed up splitting Source::invalidateTiles into two methods, and one of them invalidated tile_data but not tiles. https://github.com/mapbox/mapbox-gl-native/blob/6d357eb0c87897ef99926a4b2b12d0ae3354e7d8/test/api/annotations.cpp#L84-84 will catch that.

@jfirebaugh jfirebaugh mentioned this pull request Oct 1, 2015
Closed
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
@jfirebaugh
Merge pull request mapbox#2420 from mapbox/annotations
bf669ad 
[core] Annotations refactor
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
5 participants
@jfirebaugh @picciano @erf @friedbunny @incanus