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

Add support for (geojson) layers #723

Merged
merged 33 commits into from
Nov 12, 2021
Merged

Add support for (geojson) layers #723

merged 33 commits into from
Nov 12, 2021

Conversation

felix-ht
Copy link
Collaborator

@felix-ht felix-ht commented Oct 25, 2021

Adding full support for feature layers for ios, android and web. Click detection for these layers are supported as well. Code generation takes care of creating most of the code. Thanks to @maximumbuster for getting this started

Limitations

Notes

This pull request is based on code from #272 & #326

@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 25, 2021 19:22 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 25, 2021 19:22 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 25, 2021 19:32 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 25, 2021 19:32 Failure
@felix-ht felix-ht requested a review from m0nac0 October 25, 2021 19:32
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 26, 2021 15:15 Failure
@felix-ht felix-ht had a problem deploying to ANDROID_CI_DOWNLOADS_TOKEN October 26, 2021 15:15 Failure
* fixed issue with ios and array literals
* added belowId options to addLayer functions
* added set setGeoJsonSource to set new data on the fly
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 4, 2021 20:23 Inactive
@felix-ht felix-ht temporarily deployed to ANDROID_CI_DOWNLOADS_TOKEN November 4, 2021 20:23 Inactive
@felix-ht
Copy link
Collaborator Author

felix-ht commented Nov 4, 2021

Note that belowLayerId in addFill{Type} is currently ignored for the web version - this will be addressed with andrea689/mapbox-gl-dart#15

@AAverin
Copy link
Contributor

AAverin commented Nov 5, 2021

Man, tell me you didn't type every single of those 10k lines and managed to auto-generate all the boilerplate somehow!
Great job.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Nov 9, 2021

@AAverin most of it is generated and 50% is the style.json file 😄

@tobrun it would be really great if you could take a look before this get's merged

@felix-ht felix-ht mentioned this pull request Nov 10, 2021
@m0nac0
Copy link
Collaborator

m0nac0 commented Nov 10, 2021

@felix-ht It would be great if we could also support the clustering attributes for geojson sources (https://docs.mapbox.com/mapbox-gl-js/style-spec/sources/#geojson-cluster). That would finally enable clustering in this plugin, a quite frequently requested feature.

But I totally understand if you don't want to add even more changes to this PR, we can also just open an issue for that after this PR has landed.

@felix-ht
Copy link
Collaborator Author

felix-ht commented Nov 10, 2021

@m0nac0 i took a look and adding clustering should be pretty straightforward after this. It's also useful for my own usecase.

We would have to add some new code gen to create the ClusterProperties object, and parse it one the native side. This is trivial for web but certainly somewhat harder for native.

However i would like to push this to a future PR so we can get this one merged sooner. The api would stay stable as we would simply add some named parameters to addGeojsonSource

Another thing i want to add in the future is the ability to only update a specific feature in a source without replacing the whole collection. But i would also push this to some other future PR.

On a side note: i finished integrating this PR/branch into my own code base - an everything seems to work fine across all platforms. (Including fairly complicated data driven styles)

@tobrun
Copy link
Collaborator

tobrun commented Nov 12, 2021

@felix-ht amazing work and also thanks for @maximumbuster for kicking this off.
This goes above and beyond on what I was hoping to see supported for this wrapper SDK.
I was only planning to include this with moving towards #140 and #566 and using existing code generators for android/ios

@lukemadera
Copy link

Thanks for this awesome support and work @felix-ht Question - you noted it worked across multiple platforms but I noticed the code for addGeoJsonSource throws an error for non web? Or does this work on iOS & Android too? https://github.com/flutter-mapbox-gl/maps/blob/master/mapbox_gl_platform_interface/lib/src/mapbox_gl_platform_interface.dart#L280

Also, can a source be removed? I'm getting an error on rebuild when the data changes due to adding the same geojson source twice (it is already on the map). Should be an easy fix of checking if it is there first, but I get an error The method 'getSource' isn't defined for the class 'MapboxMap?'. Similar with remove source, which is needed for deleting sources. Does this functionality exist or does it need to be added? Any reason addSource and removeSource aren't exposed / available?

@m0nac0
Copy link
Collaborator

m0nac0 commented Jan 9, 2022

@lukemadera Regarding your first question: the implementation for iOS and Android lives in https://github.com/flutter-mapbox-gl/maps/blob/master/mapbox_gl_platform_interface/lib/src/method_channel_mapbox_gl.dart (it mostly just calls the relevant platform channel methods).

addGeoJsonSource and removeSource are available, support for other types of sources is being worked on in #797. @felix-ht Please correct me if I'm wrong.

@lukemadera
Copy link

Thanks @m0nac0 ! I am using addGeoJsonSource but removeSource is not working for me - could you share or point me to an example of how to use it?

@m0nac0
Copy link
Collaborator

m0nac0 commented Jan 10, 2022

Sorry, I haven't used the new GeoJson sources myself.

@felix-ht
Copy link
Collaborator Author

@lukemadera if you have issues please create a new ticket - this is not the place for discussions. But as far as i know removeSource should work fine. (You have to remove any layer attached to the source first)

@lukemadera
Copy link

As noted above @felix-ht I get an error: The method 'removeSource' isn't defined for the class 'MapboxMap?' Have you used removeSource? What property is it exposed on?

@felix-ht
Copy link
Collaborator Author

you have to use the MapboxMapController not the MapboxMap

@lukemadera
Copy link

Ah nice, progress! Thanks @felix-ht Getting an error though; I opened an issue: #850

m0nac0 added a commit to maplibre/flutter-maplibre-gl that referenced this pull request May 14, 2022
* Cherry-picked: Add support for (geojson) layers (flutter-mapbox-gl/maps#723)
* CI: also run pub get for scripts

Co-authored-by: Felix Horvat <felix.horvat@ocell.aero>
Co-authored-by: Max Buster <max@outway.io>
mario-jerkovic pushed a commit to mario-jerkovic/flutter_mapbox_gl that referenced this pull request Sep 24, 2022
* Added source and layers

* updates

* Changes

* added symbol layer to ios

* example code gen with mustache

* added swift and full java generation

* cleaned generator code
added dart template

* improved generation

* added doc string generation

* added web bindings

* working android implementation

* working web version

* basic working ios version

* working ios version

* added color tools

* added layer_properties

* fixed doc issue

* added click support for ios, android, and web

* getting access_token from env
minor cleanup

* fixed lint issues

* removed access token changes

* removed package config

* code review changes

* more code review changes

* deprecated addLayer
fixed issue with remove source

* added colon to Sdk Support

* removed token

* renamed removeImageSource to removeSource

* added missing result.sucess

* fixed issue with onFillTapped and stacked layers

* added setSource and add below layer capability
* fixed issue with ios and array literals
* added belowId options to addLayer functions
* added set setGeoJsonSource to set new data on the fly

* added doc strings

* fixed issues with android in the example

Co-authored-by: Max Buster <max@outway.io>

(cherry picked from commit 3c0ae33)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants