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

Annotation manager moved to dart #779

Merged
merged 26 commits into from Jan 18, 2022
Merged

Annotation manager moved to dart #779

merged 26 commits into from Jan 18, 2022

Conversation

felix-ht
Copy link
Collaborator

@felix-ht felix-ht commented Nov 19, 2021

With the addition of layers, we now have have the option to implement the annotation manager code in dart. This significantly reduces the required plugin surface area. This PR does this migration while keeping the old api stable.

New Features

  • Fixes longstading performance issues on android caused by annotations manager
  • Full annotation manager implementation in dart
  • the api is stable - no changes required by the users
  • using Object oriented annotation managers in dart are now possible
  • adding multiple annotation managers of the same type is now possible (using the object oriented api)
  • fixes long standing issues (e.g.: Fills with patterns and colors are now possible at the same time)
  • full drag and drop support - also for normal feature layers
  • support for updating single features
  • disable interaction for certain geojson layers
  • unified code path for geojson layers and annotations
  • deleted 50% of the whole native code

@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 19, 2021 19:58 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 19, 2021 19:58 Inactive
@felix-ht
Copy link
Collaborator Author

@tobrun @shroff @AAverin

Some feedback would be fantastic 😃

@AAverin
Copy link
Contributor

AAverin commented Nov 19, 2021

Overall I like this direction, but I will need to test if all of the features are still working fine.
In my PR #776 I have just fixed moving lines programmatically (which I need for my project) and also did a big investigation on why dragging doesn't work, posted my findings here #749

Annotations on both Android and iOS are some experimental plugins that are not maintained for years. I don't even understand why something like https://github.com/mapbox/mapbox-annotation-extension is still used by Mapbox powering some crucial features drawing something on top of the map, even though it clearly says there that repo is not intended for production usage.

If we will be able to rebuild all of the annotation features using geoJSON it might get easier to support them.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Nov 19, 2021

Overall I like this direction, but I will need to test if all of the features are still working fine. In my PR #776 I have just fixed moving lines programmatically (which I need for my project) and also did a big investigation on why dragging doesn't work, posted my findings here #749

Annotations on both Android and iOS are some experimental plugins that are not maintained for years. I don't even understand why something like https://github.com/mapbox/mapbox-annotation-extension is still used by Mapbox powering some crucial features drawing something on top of the map, even though it clearly says there that repo is not intended for production usage.

If we will be able to rebuild all of the annotation features using geoJSON it might get easier to support them.

For the next release we could keep the old code in place as well. Bot methods can life fine next to each other.
The release after we can replace it with the new manager code. So we can iron out all the issues in the mean time.

@Chaoba
Copy link
Collaborator

Chaoba commented Nov 22, 2021

@felix-ht We can generate the codes for annotation managers.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Nov 22, 2021

@Chaoba I don't think we would gain to much by generating them. With the current implementation the code needed per manager is pretty limited.

For lines this is all:

class LineManager extends _AnnotationManager<Line> {
  LineManager(MapboxMapController controller, {void Function(Line)? onTap})
      : super(controller, onTap: onTap);
  @override
  LayerProperties get properties => const LineLayerProperties(
        lineOpacity: [Expressions.get, 'lineOpacity'],
        lineColor: [Expressions.get, 'lineColor'],
        lineWidth: [Expressions.get, 'lineWidth'],
        lineGapWidth: [Expressions.get, 'lineGapWidth'],
        lineOffset: [Expressions.get, 'lineOffset'],
        lineBlur: [Expressions.get, 'lineBlur'],
      );
}

@felix-ht felix-ht added the enhancement New feature or request label Nov 24, 2021
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 13:51 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 13:51 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 14:08 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 14:08 Failure
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 15:08 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 15:08 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:02 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:02 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:04 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:04 Inactive
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:26 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:26 Failure
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:28 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 18:28 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 19:41 Inactive
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN December 16, 2021 19:41 Failure
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 17:18 Inactive
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 17:18 Failure
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 18:40 Inactive
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 18:40 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 18:51 Failure
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 17, 2022 18:51 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 18, 2022 09:17 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 18, 2022 09:17 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 18, 2022 09:32 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN January 18, 2022 09:32 Inactive
@felix-ht
Copy link
Collaborator Author

@m0nac0 do you want to still take a look as well or are you fine with me merging it?

@m0nac0
Copy link
Collaborator

m0nac0 commented Jan 18, 2022

Feel free to merge!

Are you planning on keeping both the old and the new approach for the next release?

@lfofelipe
Copy link
Contributor

lfofelipe commented Jan 18, 2022

Asset Images added to the style are not showing on the ios platform.
Example - > Place Symbol -> "add (custom icon)"
ios: 15.2.1
model: iphone SE

@felix-ht
Copy link
Collaborator Author

felix-ht commented Jan 18, 2022

Asset Images added to the style are not showing on the ios platform. Example - > Place Symbol -> "add (custom icon)" ios: 15.2.1 model: iphone SE

I tested it and automatic loading of assets is currently not working for ios. They have to be added manually (as it is done with add(asset image)). We have to handle the callbacks properly for ios for this to work. But i would solve that in a future PR. This one is large enough as it is. This would also be good for normal feature layers.

@m0nac0 yes i would like to keep the old and the new api - we can deprecate it down the line tho. Than we could even move the annotation stuff to a separate package that just uses the apis exposed by this.

@felix-ht felix-ht merged commit 3aaa705 into master Jan 18, 2022
@felix-ht felix-ht deleted the annotation-in-dart branch January 18, 2022 15:40
@m0nac0
Copy link
Collaborator

m0nac0 commented Jan 18, 2022

Amazing work @felix-ht !

yes i would like to keep the old and the new api - we can deprecate it down the line tho.

Great, I'd suggest putting a prominent note in the changelog to tell users about the rewrite and invite them to test it. (But I'm sure you planned something like this anyways 😄 )

@AAverin
Copy link
Contributor

AAverin commented Mar 1, 2022

@felix-ht will give more details, but it seems to be a bug in the original Mapbox Android SDK that is no longer maintained

@easazade
Copy link

easazade commented Mar 1, 2022

Sorry I just realized I was using the wrong version which did not have these changes. I confirm that android performance problems are fixed after this merge though it introduces another issue which causes lines and markers that are drawn on map won't be cleared

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants