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

Upgrading from v8.2.1 -> 8.4.0 Broke Updating Camera Props #1621

Closed
dorthwein opened this issue Nov 6, 2021 · 31 comments · Fixed by #1631
Closed

Upgrading from v8.2.1 -> 8.4.0 Broke Updating Camera Props #1621

dorthwein opened this issue Nov 6, 2021 · 31 comments · Fixed by #1631

Comments

@dorthwein
Copy link
Contributor

dorthwein commented Nov 6, 2021

Describe the bug
After upgrading from v8.2.1 -> v8.4.0 update camera props stoped working.

In iOS updating zoom and heading no longer work after initial load (follow modes do still work however).

In Android, no camera prop updates work after initial load.

To Reproduce
Create MapView map camera tied to a redux store for zoom & heading props. Outside of the component, have a button to adjust props. Zoom/Heading fail to update when external button is pressed updating camera props.

Could be resolved by https://github.com/react-native-mapbox-gl/maps/pull/1619 - but unclear.

Expected behavior
Map View Zoom & Heading update as expected on prop change to Camera component after initial load.

Actual behavior
Map View Zoom & Heading, and in Android, follow modes etc..., are reflected in Map View after updating the Map Camera.

Screenshots

Versions (please complete the following information):

  • Platform: Android, iOS
  • Platform OS: iOS 15.0
  • Device: iPhone 12
  • Emulator/ Simulator: yes (also failed on real device)
  • Dev OS: OSX 11.6
  • react-native-mapbox-gl: 8.4.0
  • Mapbox GL version: 6.4.1
  • React Native Version 0.66.2
@naftalibeder
Copy link
Collaborator

@dorthwein Sorry you're having this issue!

Setting up a new example with Redux integrated is kind of a lot of overhead just to verify. Are you able to run this example, and if so does the zoom change work?

If it does, then I would suggest the problem is with your test repo. If it doesn't, would you be able to attach a video showing what it does?

@dorthwein
Copy link
Contributor Author

@naftalibeder here is an example of follow working & not working trying to set following the user. No redux.

Test on real Android Device
react-native-maps: 8.4.0
react-native: 0.66.2

Be sure to initialize MapBox access token somewhere.

import React, { useState } from "react"
import { TouchableOpacity, View, Text } from "react-native"
import MapboxGL from "@react-native-mapbox-gl/maps"

export const PlaygroundScene = () => {
  /*
   * Toggle follow user locaiton on/off
   */
  const [follow, setFollow] = useState(false)
  const toggleFollow = () => {
    setFollow(!follow)
  }

  /*
   * Toggle between working and not working examples
   */
  const [example, setExample] = useState("notWorking")
  const toggleExample = () => {
    if (example == "notWorking") setExample("working")
    else if (example == "working") setExample("notWorking")
  }

  return (
    <View style={{ flex: 1 }}>
      <MapboxGL.MapView styleURL={MapboxGL.StyleURL.Satellite} style={{ flex: 1 }}>
        {/*
         * Un mounting and re-mounting the camera seems to work and
         * prompted follow to work as expected.
         */}
        {follow && example == "working" ? (
          <MapboxGL.Camera
            followUserLocation={follow}
            followUserMode={"course"}
            zoomLevel={16}
            key="working"
          />
        ) : (
          <View />
        )}

        {/*
         * Simply updating the followUserLocation prop, does not
         * trigger following, however it previously did.
         *
         * Note: When toggling between working & not working,
         * be sure to set the example, then do multiple attempts at
         * toggling follow on & off.  Otherwise it will just behave
         * like the working example due to re-mounting.
         */}
        {example == "notWorking" ? (
          <MapboxGL.Camera
            followUserLocation={follow}
            followUserMode={"course"}
            zoomLevel={16}
            key="not-working"
          />
        ) : (
          <View />
        )}
        <MapboxGL.UserLocation
          animated={true}
          renderMode="native"
          androidRenderMode={"gps"}
          key="mapUserLocationComponent"
        />
      </MapboxGL.MapView>
      <Controls toggleFollow={toggleFollow} toggleExample={toggleExample} />
      <Stats follow={follow} example={example} />
    </View>
  )
}
const Stats = ({ follow, example }) => {
  return (
    <View>
      <Text style={{ color: "#FFF" }}>Is Following: {follow ? "YES" : "NO"}</Text>
      <Text style={{ color: "#FFF" }}>Example: {example}</Text>
    </View>
  )
}

const Controls = ({ toggleFollow, toggleExample }) => {
  return (
    <View style={{ flexDirection: "row", alignItems: "center", padding: 8 }}>
      <Button label={"Toggle Follow"} onPress={toggleFollow} />

      <Button label={"Toggle Example"} onPress={toggleExample} />
    </View>
  )
}

const Button = ({ label, onPress }) => {
  return (
    <TouchableOpacity
      onPress={onPress}
      style={{ padding: 8, backgroundColor: "blue", borderRadius: 8, margin: 4 }}>
      <Text style={{ color: "#FFF" }}>{label}</Text>
    </TouchableOpacity>
  )
}

@dorthwein
Copy link
Contributor Author

dorthwein commented Nov 9, 2021

Some additional information, if we make the following change on the camera component from false, too true, things seem to work as intended.

I'm sure there is a good reason why we don't do this though...any thoughts?

shouldComponentUpdate() {
    return false;
  }

to

shouldComponentUpdate() {
    return true;
  }

Breakdown seems to be in this.refs.camera.setNativeProps()

Also is there a reason we don't update this component to be a functional component instead?

@naftalibeder
Copy link
Collaborator

shouldComponentUpdate

Setting this to true causes the component to update anytime a prop changes, which in particular creates issues with comparing objects and functions. Updating constantly can cause significant performance degradation.

update this component to be a functional component instead

No reason not to do that, just the possible introduction of bugs, and developer effort. I would bet if you got a PR tested with that conversion, @ferdicus would be happy :)

But as for the issue you’re having, I’m going to look into that this afternoon. I have a hunch.

@ferdicus
Copy link
Member

Also is there a reason we don't update this component to be a functional component instead?

basically what @naftalibeder said:

  • who got time?
  • who got tests?
  • who tests backwards compatibility?
  • who checks native bridge files?
  • so many things to do, so little time.... oops dead already ☠️

PRs definitely welcome though :)
Make sure to check out the Discussion for additional maintainers 👋🏿

Cheers 🍻

@ferdicus
Copy link
Member

But as for the issue you’re having, I’m going to look into that this afternoon. I have a hunch.

🙇🏿

@dorthwein
Copy link
Contributor Author

dorthwein commented Nov 10, 2021

Also is there a reason we don't update this component to be a functional component instead?

basically what @naftalibeder said:

  • who got time?
  • who got tests?
  • who tests backwards compatibility?
  • who checks native bridge files?
  • so many things to do, so little time.... oops dead already ☠️

PRs definitely welcome though :) Make sure to check out the Discussion for additional maintainers 👋🏿

Cheers 🍻

100% understand. I'll try and slot some time around upgrading components/contributing where I can! ~ DFO ~

@dorthwein
Copy link
Contributor Author

@naftalibeder - If you can point me in a direction, I can try and take a look at figuring out what could have changed btw.

@naftalibeder
Copy link
Collaborator

@dorthwein Sorry I didn't respond earlier, I think I may have resolved it in #1631 . Would you want to try pointing here

"@react-native-mapbox-gl/maps": "https://github.com/TruckMap/maps.git#fix/camera",

and see if that works?

@dorthwein
Copy link
Contributor Author

dorthwein commented Nov 12, 2021

@naftalibeder I must be tripping because I could have swore I responded here....

Anyway - I tried your fixes, zoomTo is working if followUserLocation={true} however if it is false, it does not work. We have a specific use case relying on this thats basically a +/- zoom buttons.

Also, setting followUserLocation={true} and setting followUserMode="course" does not cause the map to follow the user's location while driving :(.

@naftalibeder
Copy link
Collaborator

@dorthwein You're not crazy - you responded here. :)

Let me check into it.

@naftalibeder
Copy link
Collaborator

naftalibeder commented Nov 12, 2021

@dorthwein

zoomTo is working if followUserLocation={true} however if it is false, it does not work.

Are you sure you don't have that backwards? I see zoomTo only working if followUserLocation === false. if it's true, this logic is triggered, and the zoom level does not update.

This is because _followZoomLevel is never actually set - the function setFollowZoomLevel is never called. This seems to be a bug, but it's a part of the code base that I haven't touched, so I don't think my PR could have broken it.

Check out the Fit example again, where I've now added a toggle for followUserLocation. Let me know your thoughts.

@dorthwein
Copy link
Contributor Author

@naftalibeder - I'll try and take a look at it shortly - side note - do y'all use use followUserLocation or just update the camera?

@naftalibeder
Copy link
Collaborator

We just set bounds and centerCoordinate with a variable duration, which is probably why I hadn’t noticed any issue.

@dorthwein
Copy link
Contributor Author

dorthwein commented Nov 15, 2021

@naftalibeder yeah I can confirm still not working although it takes some toggling and messing around with. Interestingly when using Lokito to mock locations on the examples, it doesn't update the user location, however Lokito does update the location in google maps, our project etc...

Updating the key (which is effectively a remount) is a functional work around for us.

@naftalibeder what are you using to mock location/moving?

@naftalibeder
Copy link
Collaborator

@dorthwein Let me double-check on Android - admittedly I do more testing on iOS.

@Alaa-Ben
Copy link

Alaa-Ben commented Nov 17, 2021

Same issue on IOS only when upgrading from 8.3.0 to 8.4.0 :'(
All good on Android

@WilliamF63
Copy link

Yes on Android no problem but on Ios when i use FlyTo to change my camera position, the zoomlevel passed zoomLevel Max.

I tried to change flyTo at moveTo but it's same issue.

@dorthwein
Copy link
Contributor Author

@naftalibeder - I'm confirming on iOS if followerUserLocation: false, adjusting zoom level on iOS & Android does not work, even if we change the key.

Some where some seriously breaking changes got introduced when adjusting camera props.

@naftalibeder
Copy link
Collaborator

@dorthwein Can you try the latest commit on this #1631? See this comment.

@dorthwein
Copy link
Contributor Author

Confirming this https://github.com/react-native-mapbox-gl/maps/pull/1631 fixes the zoom & follow issues on iOS & Android

Great job @naftalibeder!

@narcismihaitech
Copy link

Thank you for the fix. When could we expect a release with this?

@ferdicus
Copy link
Member

@narcismihaitech, today

@ferdicus
Copy link
Member

ferdicus commented Nov 26, 2021

release 8.5.0 is published - it includes the fix for this issue

@danidaryaweesh
Copy link

I'm having this issue on 8.5.0. Updating zoomLevel props and nothing happens to the zoom after first component render. Is anyone else experiencing this?

@Cmoen11
Copy link

Cmoen11 commented Apr 1, 2022

yes, still problems with switching followUserMode with state.... Forcing me to use the key prop

<MapboxGL.Camera
					ref={cameraRef}
					key={`${navigationSettingsState.mode}-${mapProperties.zoom}`} // must be set to force re-render when changing the camera (only on android it appears.)
					followZoomLevel={mapProperties.zoom}
					followUserLocation={navigationSettingsState.followUserLocation}
					followUserMode={navigationSettingsState.followUserMode}
					followPitch={navigationSettingsState.pitch}
					allowUpdates
				/>

and when changing followZoomLevel, it causes buggy experiences

@danidaryaweesh
Copy link

@Cmoen11 Are you using the latest release version 8.5.0 ?

@Cmoen11
Copy link

Cmoen11 commented Apr 5, 2022

@danidaryaweesh Yes, i'm using "@react-native-mapbox-gl/maps": "^8.5.0",

@danidaryaweesh
Copy link

@Cmoen11 For me it is working on Android without having to use the key prop, although a bit buggy and not as smooth as iOS.

@Cmoen11
Copy link

Cmoen11 commented Apr 20, 2022

@danidaryaweesh Could you share your react code for the map?

@danidaryaweesh
Copy link

danidaryaweesh commented Apr 20, 2022

@Cmoen11 We are using this library as a base for turn by turn navigation.

<MapboxGL.MapView
                    styleURL={'mapbox://styles/....'}
                    ref={r => {
                        _mapRef = r;
                    }}
                    onLayout={e =>
                        setUserPositionInViewDisplacement(
                            e.nativeEvent.layout.height / 2,
                        )
                    }
                    contentInset={[userPositionInViewDisplacement, 0, 0, 0]}
                    onRegionWillChange={onMapLocationUpdate}
                    style={styles.map}
                    logoEnabled={false}
                    compassEnabled={false}
                    attributionEnabled={false}>
                    <MapboxGL.Camera
                        allowUpdates={true}
                        animationDuration={1500}
                        animationMode={'flyTo'}
                        maxZoomLevel={21}
                        followZoomLevel={
                            !cameraProps.zoomLevel
                                ? defaultCameraProps.zoomLevel
                                : cameraProps.zoomLevel
                        }
                        defaultSettings={{
                            centerCoordinate: [18.0521569, 59.3274506],
                            zoomLevel: defaultCameraProps.zoomLevel,
                            pitch: defaultCameraProps.pitch,
                        }}
                        followPitch={cameraProps.pitch}
                        {...cameraProps}
                        followUserLocation={localFollowUserLocation}
                        followUserMode={localTrackingMode}
                        userTrackingMode={
                            MapboxGL.UserTrackingModes.FollowWithCourse
                        }
                        centerCoordinate={
                            !test &&
                            !localFollowUserLocation &&
                            currentLocation
                                ? currentLocation
                                : undefined
                        }
                    />
....
</MapboxGL.MapView>

Updating zoom level in this way:

setCameraProps({ zoomLevel: Platform.OS === 'ios' ? 21 : 17, pitch: defaultCameraProps.pitch, });

And default camera props is:
const defaultCameraProps = { zoomLevel: Platform.OS === 'ios' ? 20.35 : 16, pitch: 89, };

Hope that this helps.

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

Successfully merging a pull request may close this issue.

8 participants