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

[core] [android] Update marker position and icon #3885

Closed

Conversation

socketbind
Copy link
Contributor

Hi,

This pull request enables updating a marker's position and icon in place without resorting to workarounds like removing and adding the marker. I have implemented this functionality because I needed it for an application that displays neighboring cards, traffic lights with animated marker icons.

As my C++ knowledge is a bit rusty the code might be a little bit crude. I would love to improve it to be more performant (it can get really slow with a lot of markers even on powerful devices) and make it so that it conforms better to your design principles, code quality.

Relates to #3775 (change marker icon without workarounds) but also adds option for changing position.

@tobrun
Copy link
Member

tobrun commented Feb 12, 2016

@socketbind
thank you reaching out and contributing!
I'm going review the java code and will add some inline comments.
I will try to track someone down to validate the C++ code.

@tobrun tobrun added feature Android Mapbox Maps SDK for Android labels Feb 12, 2016
@tobrun tobrun added this to the android-v4.0.0 milestone Feb 12, 2016
@socketbind
Copy link
Contributor Author

Hi @tobrun,
Thank you for the review. I have replaced HashMap in MapView with LongSparseArray per your suggestion and also made an attempt to make the C++ code a bit more performant by trying to reduce the number of operations required to update the marker (calling the appropriate AnnotationTileMonitor::update method in annotation_manager.cpp to update and coalescing the marker update calls into a single one when possible). It seems to be working properly but I'm unsure about its correctness (especially with regards to thread safety) because my knowledge is still very limited about the engine and C++14 itself. Could you please help me validate it?

I'm still trying to make things even faster as my application does a lot of marker updates in a short time but I think the code included here could be appropriate for basic functionality (given that it is actually correct and the design is acceptable).

@jfirebaugh
Copy link
Contributor

Thanks for the contribution @socketbind. I've merged the core C++ parts of this in #3933 (using your initial implementation rather than the more complex version in 23c357a5b210c8d4a45d32c947009ead1e18a057) and added a test. Can you rebase the Android changes and squash then down to a single commit for @tobrun to do a final review?

@socketbind socketbind force-pushed the update-marker-position-and-icon branch from 2088ac3 to 6f8e9b9 Compare February 14, 2016 20:52
@socketbind socketbind force-pushed the update-marker-position-and-icon branch from 6f8e9b9 to 2088ac3 Compare February 14, 2016 20:53
@socketbind socketbind force-pushed the update-marker-position-and-icon branch from 2088ac3 to 2620354 Compare February 14, 2016 21:25
@socketbind
Copy link
Contributor Author

Rebased changes as requested (messed up the first rebase on these commits, you might have received some strange emails, sorry about that).

I have also removed 23c357a as it turned out to be unreliable. Still, I would l love to improve performance somehow. I have been experimenting with batched updates (using a window of 100 ms on the Java side to save on JNI calls and applying changes first then issuing a single update call) but it only helped slightly, update(Update::Annotations) seems to be very costly for now.

@tobrun
Copy link
Member

tobrun commented Feb 15, 2016

@socketbind, np, I have fixed the rebase issue and pushed with #3943. For the Android code I'm also going to add some tests in that PR. Will also add an example activity demonstrating this awesome feature.

tobrun added a commit that referenced this pull request Feb 15, 2016
@tobrun
Copy link
Member

tobrun commented Feb 15, 2016

@socketbind b395302 landed with #3943. Thank you again for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants