nitaliano / react-native-mapbox-gl

A Mapbox GL react native module for creating custom maps
Other
2.16k stars 697 forks source link

[7.0.0] API Design #1238

Open nitaliano opened 6 years ago

nitaliano commented 6 years ago

For anyone that would like to leave feedback on what they would like to see in our upcoming major release please leave your feedback here.

edolix commented 6 years ago

Wow, looking forward to see this release 👍 I've found a strange bug described in #937 .

The upcoming release deprecates the PointAnnotation? I've read something related around issues or gitter channel. Thanks!

keenubee commented 6 years ago

Do you plan to integrate routing?

nitaliano commented 6 years ago

@edolix PointAnnotation is going to be removed, AnnotationViews no longer exist in the next version of the Android SDK. There are plans to drop an Annotation component that wraps sources/layers and gives a similar API to PointAnnotation

nitaliano commented 6 years ago

@keenubee when you ask about routing are you talking about the nav SDK? That would be a separate react native SDK that we are debating if we should release or not

kristfal commented 6 years ago

@nitaliano

First of all, thanks a lot for your work on this wrapper. It's really good! Onto some wishes:

1) Camera API

Current state: To fully use the camera API, a mix between setCamera and props on the <MapView> is required.

Ex: During user location tracking, the android app takes into account zoomLevel on the <MapView>, meaning that any zoomLevel applied via setCamera doesn't propagate to the user location tracking state. There are also quite a few other issues related to the camera that do exist currently.

Proposal: 1) Go all in on a declarative prop-based camera API. 2) Ensure that any combination of prop changes doesn't cause race conditions / ignored updates

Here is a proposal for camera related props:

  animated: {Function | Bool} – can either be set to true/false, or passed a function that takes two arguments: (currentCamera, nextCamera) => which in turn should return Boolean to indicate camera transition should be animated
  animationDuration: {Function | Number} – if animated is true, sets the duration of the animation. Can also be passed function that takes two arguments: (currentCamera, nextCamera) => which in turn should return a Number to indicate transition duration
  animationType: {Function | String} – If string, one of 'FlyTo' or 'JumpTo'. Can also be passed function that takes two arguments: (currentCamera, nextCamera) => which in turn should return a String to indicate transition type
  center: {Array} – camera center 
  zoom: {Number} – camera zoom
  bearing: {Number} – camera bearing
  pitch: {Number} – camera pitch
  bounds: {Array} – camera bounds. Setting this after setting center or zoom will override center and zoom and vice versa
  contentInset: {Array} – content inset of camera

With these props we've managed to make the camera fully declarative so setCamera, fitBounds, zoomTo and flyTo can be removed.

2) User location marker and camera API

Current state:

The user location is currently tied closely to the camera API, and there are quite a lot of significant differences between the iOS and Android implementation. The current implementation also leaves little room for flexibility in terms of selecting what marker to show and what camera state to be in.

Proposal: 1) Go all in on a declarative prop-based location API 2) Split up camera logic, camera follow logic and user location marker logic into separate props 3) Write all user location marker and camera logic in Javascript to make iOS and Android completely equal (some logic in iOS can't be fully handled on Android at the moment, so I think its easier for devs to not have any subtile platform differences here) 4) Always expose location, compass orientation (bearing) and vertical orientation (track) in the onUserLocationUpdate regardless of tracking mode or if showUserLocation is set to false

Proposal for user location marker related props:

  showUserLocation: {Boolean}
  userLocationMarkerType: {String} – one of: 'marker', 'markerWithViewOrientation', 'markerWithArrow', 'navigationArrow'. May also be able to pass in any valid sprite in the loaded sprite sheet or imageSource.
  showUserLocationAccuracy: {Boolean}

Proposal for user location camera related props:

  cameraFollowUserLocation: {Boolean} – if camera should follow the user's location
  cameraFollowUserOrientation: {String} – One of: 'none', 'heading', 'track'. Heading is compass and track is vertical speed orientation.
  cameraFollowUserLocationStopTrigger: {String} one of: 'anyGesture', 'panGesture', 'programmatic' and 'none'. Decides when the camera should stop following the user's location. 'anyGuesture' and 'panGuesture' will also stop on 'programmatic'.
  cameraFollowVerticalAlignment: {Number} – Number from 0 to 1 that declares the vertical fraction of the camera location. 0 being fully at top, 1 fully at bottom.
  cameraFollowHorizontalAlignment: {Number} – Number from 0 to 1 that declares the horizontal fraction of the camera location. 0 being fully to the left, 1 fully to the right.
  cameraFollowPitch: {Boolean} – If camera should have a set pitch during follow mode (declared separately)
  cameraFollowZoom: {Boolean} – If camera should have a set zoom level during follow mode (declared separately)
  cameraFollowPitchValue: {Number} – Pitch of camera during camera follow mode. 
  cameraFollowZoomValue: {Number} – Zoom of camera during camera follow mode. 

If cameraFollowUserLocation is set to false, the camera will return to whatever state is set by the regular camera props. If cameraFollowUserOrientation is set to none after being sett to one of the other alternatives, camera orientation will return to whatever state is set by regular camera orientation prop. The same logic applies for cameraFollowPitch and cameraFollowZoom.

Feel free to add / edit this list. Treat it as a lightweight proposal based on our experiences and not something very well defined.

howardsj9 commented 6 years ago

Hi nitaliano thank you for starting this thread. Is a fix to the issue where custom icons(images) on the symbol layer blink/reload when zooming being addressed in the next release?

https://github.com/mapbox/react-native-mapbox-gl/issues/1223

Thank you very much!

sandroferrari commented 6 years ago

Hi everyone !

I'd like to take this opportunity to thank you @nitaliano and all of active contributors for your huge fantastic work, it's wonderfull !

Otherwise, I think the navigation SDK on react native would be a great idea because it would be the only one to propose a full navigation for react.

Thanks !

ferdicus commented 6 years ago

In addition it would be great to have the source/ layers play nice with animated values. I would love to pipe in animated values into a shape and have it move smoothly from one coordinate to the next based on the animation timing.

<Mapbox.Animated.ShapeSource id="positionSource" shape={<point with animated lng/ lat values>}>  
  <Mapbox.Animated.SymbolLayer id="position" style={styles.icon}/>
</Mapbox.Animated.ShapeSource>
nitaliano commented 6 years ago

@ferdicus that is something that will happen, I would like to build that into an Annotation component.

@kristfal love the ideas, this is something we will for sure explore and deserves it's own ticket to talk about implementation details.

hampelm commented 6 years ago

Thanks @nitaliano for all your hard work on this!

Layers not updating when URLs or IDs change (#1169) is a big sharp edge for us; would love that to be more react-y in a future release.

Would be nice to have long-press on feature layers (#1127). The generic version of this would be "all layer support all standard interaction types" but I haven't encountered any other situations where that's not the case.

ferdicus commented 6 years ago

Please overhaul the documentation if you find the time. Things to consider:

To further elaborate on the last point: Mapbox.geoUtils.makePoint - would be nice if something like this is mentioned in the new docs Mapbox.setTelemetryEnabled - also this

Thanks :)

ericpalakovichcarr commented 6 years ago

Will you be adding support for expressions? (i.e. https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions)

nitaliano commented 6 years ago

@bigsassy yes expression support will be added. I already have it working on iOS and currently working on the Android side

olliekav commented 6 years ago

Thanks everyone for their work on this library, it's saved out project!

As mentioned by @kristfal decoupling the user location stuff would be great. @nitaliano I think you mentioned it on another issue about adding MapboxGL.locationManger and MapboxGL.UserLocation. I would love to move away from using the RN navigator.geolocation which seems really flakey and let this library handle all that.

We can't use showUserLocation right now as there is no way to click through the marker or add a custom one, so having to add a custom icon using navigator.geolocation which is painful.

amitassaraf commented 6 years ago

Great wrapper. Great library. My most requested thing would be a more explicit camera control so I can always know in the JS side the camera properties and only send them one way to the map. Meaning I know when the user touched the map and I know when the user zoomed the map and I don't have to wait for Native callbacks to arrive as it slows down updating the data that I am displaying on the map which translates to a bad user experience.

nitaliano commented 6 years ago

setNativeProps support will be added to 7.0.0, this is something I should have added long ago

ericpalakovichcarr commented 6 years ago

Hey, @nitaliano. Do you have a broad estimate of when a 7.0.0 release would drop? Days? Weeks? Months? Too soon to tell? Thanks 👍

nitaliano commented 6 years ago

next month(July). I already have expressions working on both Android and iOS, now it's just adding the other features and fixing bugs

nitaliano commented 6 years ago

I roughly have a locationManager working, and am working on creating the UserLocation component

screenshot 2018-06-13 15 54 53

olliekav commented 6 years ago

@nitaliano This looks great, so the userLocation would act similar to a layer in terms of props (onPress, layerIndex, style etc).

nitaliano commented 6 years ago

Yup, and it would accept layers as children if you would want to customize it

nitaliano commented 6 years ago

This is the API I have setup for the locationManager

MapboxGL.locationManager.getLastKnownLocation()
MapboxGL.locationManager.addListener(listener)
MapboxGL.locationManager.removeListener(listener)
MapboxGL.locationManager.removeAllListeners()
MapboxGL.locationManager.start()
MapboxGL.locationManager.pause()
MapboxGL.locationManager.dispose()
nitaliano commented 6 years ago

@kristfal I'm going to start taking a crack at making the camera declarative, I'll post back here with an API(which will be based around what you proposed) and we can get feedback on it.

kristfal commented 6 years ago

@nitaliano cool! We’ve made our own declarative wrapper in line with the suggestion above. It does really make things easy.

We did add one extra aspect tho: All location/zoom camera props also have an initial prop, eg: initialCenter and initialZoom. The reason for this being that in some cases, we want to set the camera initial state and not have to declare updated props on camera change.

nitaliano commented 6 years ago

@kristfal if you could share any code snippets or gists of what you and your team have done I would love to look it over and port over what makes sense to the SDK.

One thing I also want to make sure of is that we think of how this API will impact a navigation camera experience and interact with a potential RN Nav SDK.

savv commented 6 years ago
kristfal commented 6 years ago

@nitaliano for the declarative camera API, you can have a look at:

https://gist.github.com/kristfal/9b1af1a381c1d57fe8dab1993cb1ad62

I included some of the helpers to give you an idea of how they work, however the gist isn't functional as it imports some stuff I've left out. In general, its a very light wrapper around what is.

We ended up with a camera API like this:

{
  bearing: {Number},
  bounds: [[{Number}, {Number}], [{Number}, {Number}]],
  center: [{Number}, {Number}],
  pitch: {Number},
  isInstant: {Boolean},
  bearing: {Number},
  zoom: {Number},
}

The same camera object could also be passed to the prop initialCamera. This camera API should probably be extended with at least an animationDuration key. There are some issues/bugs with this approach tho:

1) Camera pitch doesn't work properly when applied via setCamera (iOS only issue) 2) fitBounds lack options for changing pitch and bearing

We're getting user location from a separate react component, and user location layer is handled in separate map layer components.

We also ended up calculating the camera for user location track modes in a redux selector (straightforward except for vertical calculating offset). This responsibility should be moved into the react wrapper in your case. Let me know if you want some snippets on how we handle this.

nitaliano commented 6 years ago

@kristfal thanks for the snippet!

This is what I'm loosely prototyping right now, to see what quirks I run into cross platform screenshot 2018-06-18 13 53 46

All logic get's moved up to the JS layer, and the native layer will essential just be parsing a json config and applying the properties.

It is nice having a locationManager now, because I can just do and not have to worry about any other location needs

locationManager.addListener(listener)
mattijsf commented 6 years ago

@nitaliano That camera API looks very nice! I'm not sure if you've seen it but I experienced some performance issues using initial camera bounds with the existing API's. Therefore I created this PR https://github.com/mapbox/react-native-mapbox-gl/pull/1255. It would be great if this new Camera API would provide similar initial load performance :)

I think it will since it's layout (component) hierarchy is available upon creation of the MapView itself.

Edit: About the cross-platform quirks you're mentioning, I did ran into this one: https://github.com/mapbox/react-native-mapbox-gl/pull/1255/files#diff-645e56b5e26606b8e46afa55273c4b22R356

mattijsf commented 6 years ago

It would be nice to get out of the box support for converting \ (MarkerView) to SymbolLayer since this provides a huge performance gain on Android:

We mocked up an approach using react-gateway / react-native-view-shot which seems to work pretty well but an out-of-the-mapbox solution would be much preferred 👍

felixgourdeau commented 6 years ago

Any ETA for the 7.0.0 release?

nitaliano commented 6 years ago

Development on this has slowed down, since another project is competing with my time. I will get around to doing this release in a week or two. Majority of this release is done, it's just adding extra features that the community has asked for. Thanks for your patience everyone!

isabelle-butterfield commented 6 years ago

Any update on the 7.0.0 release? Thanks for your hard work!!

brandonreavis commented 6 years ago

@nitaliano Is there a branch with the progress towards the 7.0.0 release somewhere? Or is the release imminent?

I’m working with a style that uses expressions which requires the v4.0.0 iOS SDK, and based on the comments it seems like you have all of the major difficulties with pulling that in already resolved. If the development branch with its loose ends could made public that would help us out a bunch.

Thanks for all the awesome work on this!

woodpav commented 6 years ago

Animated MapView region

ferdicus commented 6 years ago

@nitaliano, could you give us another life sign? At this point I'm just worried about your well being :)

Cheers

nitaliano commented 6 years ago

I'm alive 😄I have time to step away from my other projects for the next couple of days to focus on this one.

I'll still be more absent 😢 from this then I would like but we're taking some steps to get help from the OS community when I am more absent. I will try to do weekly checkins with this repo so we can keep it moving. Sorry for the pain of having this so out of date

ferdicus commented 6 years ago

Good to hear! :)

What happened to the plan of adding more bodies to this project? If I remember correctly you mentioned that another dev was supposed to work with you on this?

Maybe I'm misinterpreting, however this sounds more like mapbox isn't considering this a first class sdk citizen anymore...

Also: You know, that people are going to ask for an updated ETA of 7.0, now that you replied 😏

Finally, it can't be mentioned enough: thanks for all the great work! 👏

nitaliano commented 6 years ago

That was the plan, but things changed internally and for the last couple of months I haven't been assigned to React Native. I wish I could give a better answer than this but this is where we are at.

tsemerad commented 6 years ago

@nitaliano You mentioned a couple months ago that you have expression support working on your work towards a 7.0 release. Echoing @brandonreavis, is there a branch with the progress towards the 7.0 release somewhere?

My project requires using v4.x of the iOS SDK. I've started updating this library to use NSExpressions as required by the new SDK, but while I've gotten some of it to work, I still have a long way to go. I'm pretty sure I'm duplicating what you've already set up. If I could see your work-in-progress 7.0, that would help tremendously.

Thanks again for your work on this!

nitaliano commented 6 years ago

I updated everyone in the Gitter I will be dropping a branch and everyone will have access to it shortly

ericpalakovichcarr commented 6 years ago

Hey @nitaliano. Did that branch ever drop? And also, is there anything we can help with to get this release over the finish line (e.g. write up documentation for new features)?

nitaliano commented 6 years ago

Posted my branch https://github.com/mapbox/react-native-mapbox-gl/pull/1377 it's a huge api change, I wouldn't try to use this in any projects until we get this merged into master

TheCodeDestroyer commented 6 years ago

I know this might be asking too much with your available time, but could you squeeze in custom layers?

zugaldia commented 6 years ago

Hey all - it's been a while since we posted an update so I just wanted to let you all know that though we're moving slower than we wanted, we're still focused on a v7 release which we're targeting for December. Thank you all for your continued support and patience, I'll report back as soon as I have any more information to share.

ferdicus commented 6 years ago

That's great to hear @zugaldia. Is @nitaliano still assigned to this project, or is it the two of you or are you taking over as @nitaliano has to take care of other responsibilities?

Thanks in advance

zugaldia commented 5 years ago

@ferdicus v7 work is lead by @nitaliano :)

atomheartother commented 5 years ago

@nitaliano Is v7 still planned for December? We're starting work on a project using this component and of course if you're close to a new release we might want to work on the newer api...

olivierstern commented 5 years ago

@nitaliano Will V7 update native SDK for iOS Android to the latest versions? ;)

zugaldia commented 5 years ago

hey all - as we wrap up the year I just wanted to give a quick update on the status of v7: our original plans have changed slightly but we're on track for a release on mid-January. we'll give further updates with the new year, meanwhile thank you all for your continued support and happy holidays!