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

Enabling followUserLocation with other follow props on Camera is non-deterministic #530

Open
KiwiKilian opened this issue Dec 2, 2024 · 1 comment
Milestone

Comments

@KiwiKilian
Copy link
Collaborator

KiwiKilian commented Dec 2, 2024

The followUserLocation and other follow props of the Camera are currently non-deterministic and don't work as expected.

Reproduction is possible with the following example. It basically switches between normal map browsing into a "navigation mode", by following the user and applying pitch:

import MapLibreGL, {
  UserLocationRenderMode,
  UserTrackingMode,
} from "@maplibre/maplibre-react-native";
import { useState } from "react";
import { Button } from "react-native";

import maplibreIcon from "../../assets/images/maplibre.png";
import { OSM_RASTER_STYLE } from "../../constants/OSM_RASTER_STYLE";
import { sheet } from "../../styles/sheet";

export default function UserLocationForNavigation() {
  const [navigationActive, setNavigationActive] = useState(false);

  return (
    <>
      <Button
        title={`Navigation is ${navigationActive ? "active" : "inactive"}`}
        onPress={() => setNavigationActive((prevState) => !prevState)}
      />

      <MapLibreGL.MapView
        style={sheet.matchParent}
        styleJSON={JSON.stringify(OSM_RASTER_STYLE)}
        contentInset={navigationActive ? [200, 0, 0, 0] : undefined}
        // pitchEnabled={navigationActive}
      >
        {navigationActive ? (
          <MapLibreGL.UserLocation
            renderMode={
              navigationActive
                ? UserLocationRenderMode.Normal
                : UserLocationRenderMode.Native
            }
            showsUserHeadingIndicator
          >
            <MapLibreGL.SymbolLayer
              id="navigation-icon"
              style={{
                iconImage: maplibreIcon,
                iconPitchAlignment: "map",
                iconAllowOverlap: true,
              }}
            />
          </MapLibreGL.UserLocation>
        ) : null}

        <MapLibreGL.Camera
          followUserLocation={navigationActive}
          followUserMode={
            navigationActive
              ? UserTrackingMode.FollowWithHeading
              : UserTrackingMode.Follow
          }
          followZoomLevel={19}
          followPitch={navigationActive ? 60 : 0}
          onUserTrackingModeChange={(event) => {
            console.log(event.nativeEvent.payload);

            if (
              navigationActive &&
              !event.nativeEvent.payload.followUserLocation
            ) {
              setNavigationActive(false);
            }
          }}
        />
      </MapLibreGL.MapView>
    </>
  );
}

Expected behavior on both platforms when enabling navigation for the first and following times:

  • Zoom to user location
  • Change pitch
  • Follow user location with said pitch
Android iOS
Camera.Follow.Android.mp4
Camera.Follow.iOS.mp4

I suspect the migration to function components to be still faulty for the Camera component. Lets review the code at the latest point before migration to FC.

The component was never re-rendered, the props were only initially passed via render for the first ever render cycle. Afterwards it was blocked by:

shouldComponentUpdate(): boolean {
return false;
}

This is the render method which initially passes the native props:

render(): ReactElement {
const props = Object.assign({}, this.props);
const callbacks = {
onUserTrackingModeChange: props.onUserTrackingModeChange,
};
return (
<RCTMLNCamera
testID="Camera"
ref={this.cameraRef}
followUserLocation={this.props.followUserLocation}
followUserMode={this.props.followUserMode}
followPitch={this.props.followPitch}
followHeading={this.props.followHeading}
followZoomLevel={this.props.followZoomLevel}
stop={this._createStopConfig(props)}
maxZoomLevel={this.props.maxZoomLevel}
minZoomLevel={this.props.minZoomLevel}
maxBounds={this._getMaxBounds()}
defaultStop={this._createDefaultCamera()}
{...callbacks}
/>
);
}
}

Now looking into how the props were updated within the Class Component we see this UNSAFE_componentWillReceiveProps. It was the reason, why the migration was done as this is deprecated.

UNSAFE_componentWillReceiveProps(nextProps: CameraProps): void {
this._handleCameraChange(this.props, nextProps);
}

And this called the _handleCameraChange function:

_handleCameraChange(
currentCamera: CameraProps,
nextCamera: CameraProps,
): void {
const c = currentCamera;
const n = nextCamera;
if (!n.allowUpdates) {
return;
}
const hasCameraChanged = this._hasCameraChanged(c, n);
if (!hasCameraChanged) {
return;
}
if (c.followUserLocation && !n.followUserLocation) {
this.cameraRef.current?.setNativeProps({followUserLocation: false});
return;
}
if (!c.followUserLocation && n.followUserLocation) {
this.cameraRef.current?.setNativeProps({followUserLocation: true});
}
if (n.followUserLocation) {
this.cameraRef.current?.setNativeProps({
followUserMode: n.followUserMode,
followPitch: n.followPitch || n.pitch,
followHeading: n.followHeading || n.heading,
followZoomLevel: n.followZoomLevel || n.zoomLevel,
});
return;
}
if (n.maxBounds) {
this.cameraRef.current?.setNativeProps({
maxBounds: this._getMaxBounds(),
});
}
if (n.minZoomLevel) {
this.cameraRef.current?.setNativeProps({
minZoomLevel: this.props.minZoomLevel,
});
}
if (n.maxZoomLevel) {
this.cameraRef.current?.setNativeProps({
maxZoomLevel: this.props.maxZoomLevel,
});
}
const cameraConfig: CameraStop = {
bounds: undefined,
centerCoordinate: undefined,
padding: n.padding,
zoomLevel: n.zoomLevel,
pitch: n.pitch,
heading: n.heading,
animationMode: n.animationMode,
animationDuration: n.animationDuration,
};
const boundsChanged = this._hasBoundsChanged(c.bounds, n.bounds);
const centerCoordinateChanged = this._hasCenterCoordinateChanged(
c.centerCoordinate,
n.centerCoordinate,
);
const paddingChanged = this._hasPaddingChanged(c.padding, n.padding);
const zoomChanged = this._hasNumberChanged(c.zoomLevel, n.zoomLevel);
const pitchChanged = this._hasNumberChanged(c.pitch, n.pitch);
const headingChanged = this._hasNumberChanged(c.heading, n.heading);
let shouldUpdate = false;
if (n.bounds && boundsChanged) {
cameraConfig.bounds = n.bounds;
shouldUpdate = true;
} else if (n.centerCoordinate && centerCoordinateChanged) {
cameraConfig.centerCoordinate = n.centerCoordinate;
shouldUpdate = true;
}
if (paddingChanged || zoomChanged || pitchChanged || headingChanged) {
shouldUpdate = true;
}
if (shouldUpdate) {
this._setCamera(cameraConfig);
}
}

This one calls setNativeProps in specific order. E.g. it sets followUserLocation first, then other follow props and only then restrictions/controlled camera position. This might be the culprit for the now rather unclear behavior. It looks like it's native props are updated in a random order, when just rendering the NativeComponent and passing the props.

To be honest I'm not 100 % sure how to fix this. I couldn't find any docs around native props order during updating. I have some ideas:

  • Try to match the behavior of the old Class Component with the Functional Component
    • I would guess this means running through useEffect and maybe storing old props within a ref
    • Is this the approach you tried here @caspg?
      useEffect(() => {
      if (!props.allowUpdates) {
      return;
      }
      cameraRef.current?.setNativeProps({
      followUserLocation: props.followUserLocation,
      });
      }, [cameraRef.current, props.followUserLocation]);
    • Maybe we have to take the best of both worlds, my refactoring and your useEffects but without the NativeComponent getting the props passed?
  • Batching Native Props (like putting it into one object?)
    • Could create even more problems
    • Especially when there are partial updates
  • How does this change when migrating to the new architecture?
@KiwiKilian KiwiKilian added the bug label Dec 2, 2024
@KiwiKilian KiwiKilian added this to the 10.0.0 milestone Dec 2, 2024
@KiwiKilian KiwiKilian removed the bug label Dec 2, 2024
@KiwiKilian KiwiKilian changed the title Enabling followUserLocation with other follow props is non-deterministic Enabling followUserLocation with other follow props on Camera is non-deterministic Dec 2, 2024
@caspg
Copy link
Contributor

caspg commented Dec 2, 2024

I was trying what was done in class component but I didn't suspect that order of passing native props matter. It's quite strange 🤔

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

No branches or pull requests

2 participants