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.

Sign up for GitHub

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

Jump to bottom

Fix camera method and address deprecations #3602

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Fix camera method and address deprecations #3602

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
89d57ba
Use v11 camera method when available
naftalibeder Aug 29, 2024
c470fa3
Address deprecations and warnings
naftalibeder Aug 29, 2024
23c237c
Fix deprecated camera method
naftalibeder Aug 29, 2024
1234883
Merge branch 'main' of github.com:rnmapbox/maps into fix/camera
naftalibeder Sep 3, 2024
c602be7
Merge branch 'main' of github.com:rnmapbox/maps into fix/camera
naftalibeder Sep 12, 2024
5823880
Consolidate switch inside of map getter
naftalibeder Sep 12, 2024
5290700
Get mapView directly
naftalibeder Sep 13, 2024
762c1e7
Avoid setting camera with empty config
naftalibeder Sep 16, 2024
42e6726
Use v11 camera method when available
naftalibeder Aug 29, 2024
8582e6b
Address deprecations and warnings
naftalibeder Aug 29, 2024
bc011ab
Fix deprecated camera method
naftalibeder Aug 29, 2024
715adf7
Consolidate switch inside of map getter
naftalibeder Sep 12, 2024
67183ff
Get mapView directly
naftalibeder Sep 13, 2024
2f46a22
Avoid setting camera with empty config
naftalibeder Sep 16, 2024
0aaa177
Merge branch 'main' of github.com:rnmapbox/maps into fix/camera
naftalibeder Sep 20, 2024
6c91f71
Merge branch 'fix/camera' of github.com:TruckMap/maps into fix/camera
naftalibeder Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions android/src/main/java/com/rnmapbox/rnmbx/components/camera/CameraStop.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@ package com.rnmapbox.rnmbx.components.camera

import android.animation.Animator
import android.content.Context
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toPointGeometry
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toLatLng
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toLatLngBounds
import com.rnmapbox.rnmbx.utils.LatLngBounds
import com.rnmapbox.rnmbx.components.mapview.RNMBXMapView
import com.mapbox.maps.CameraOptions
import com.facebook.react.bridge.ReadableMap
import com.mapbox.geojson.FeatureCollection
import com.mapbox.geojson.Point
import com.mapbox.maps.CameraOptions
import com.mapbox.maps.EdgeInsets
import com.mapbox.maps.ScreenCoordinate
import com.rnmapbox.rnmbx.components.camera.constants.CameraMode
import com.rnmapbox.rnmbx.components.mapview.RNMBXMapView
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toLatLng
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toLatLngBounds
import com.rnmapbox.rnmbx.utils.GeoJSONUtils.toPointGeometry
import com.rnmapbox.rnmbx.utils.LatLng
import com.rnmapbox.rnmbx.utils.LatLngBounds

class CameraStop {
private var mBearing: Double? = null
Expand Down Expand Up @@ -124,14 +126,22 @@ class CameraStop {
if (mLatLng != null) {
builder.center(mLatLng!!.point)
} else if (mBounds != null) {
val coordinates = listOf<Point>(
mBounds!!.toBounds().northeast,
mBounds!!.toBounds().southwest
)
val tilt = if (mTilt != null) mTilt!! else currentCamera.pitch
val bearing = if (mBearing != null) mBearing!! else currentCamera.bearing

val boundsCamera = map.cameraForCoordinateBounds(
mBounds!!.toBounds(),
val boundsCamera = map.cameraForCoordinates(
coordinates,
CameraOptions.Builder()
.pitch(tilt)
.bearing(bearing)
.build(),
cameraPaddingEdgeInsets,
bearing,
tilt
null,
ScreenCoordinate(0.0, 0.0)
)
builder.center(boundsCamera.center)
builder.anchor(boundsCamera.anchor)
Expand Down
67 changes: 48 additions & 19 deletions ios/RNMBX/RNMBXCamera.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,24 @@ struct CameraUpdateItem {
try center.validate()
}

switch mode {
case .flight:
map.mapView.camera.fly(to: camera, duration: duration)
case .ease:
map.mapView.camera.ease(to: camera, duration: duration ?? 0, curve: .easeInOut, completion: nil)
case .linear:
map.mapView.camera.ease(to: camera, duration: duration ?? 0, curve: .linear, completion: nil)
default:
map.mapboxMap.setCamera(to: camera)
}
switch mode {
naftalibeder marked this conversation as resolved.
Show resolved Hide resolved
case .flight:
map.withMapView { mapView in
mapView.camera.fly(to: camera, duration: duration)
}
case .ease:
map.withMapView { mapView in
mapView.camera.ease(to: camera, duration: duration ?? 0, curve: .easeInOut, completion: nil)
}
case .linear:
map.withMapView { mapView in
mapView.camera.ease(to: camera, duration: duration ?? 0, curve: .linear, completion: nil)
}
default:
map.withMapboxMap { mapboxMap in
mapboxMap.setCamera(to: camera)
}
}
}
}
}
Expand Down Expand Up @@ -87,8 +95,10 @@ open class RNMBXMapComponentBase : UIView, RNMBXMapComponent {
}

func withMapView(_ callback: @escaping (_ mapView: MapView) -> Void) {
withRNMBXMapView { mapView in
callback(mapView.mapView)
withRNMBXMapView { map in
map.withMapView { mapView in
callback(mapView)
}
}
}

Expand Down Expand Up @@ -447,17 +457,33 @@ open class RNMBXCamera : RNMBXMapComponentBase {

withMapView { map in
#if RNMBX_11
let bounds = [sw, ne]
do {
let bounds: [CLLocationCoordinate2D] = [sw, ne]
camera = try map.mapboxMap.camera(
for: bounds,
camera: .init(cameraState: .init(
center: .init(),
padding: .zero,
zoom: zoom ?? 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that I have more consistant results by using zoom: .zero. If zoom is set on this camera is seems to effect the positioning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfazekas @naftalibeder I'll aim to make a demo this afternoon of why i've highlighted this.. but when moving the camera with setCamera from a centerCoordinates position to setCamera with bounds it seems that zoom is applied to the final position. Adding this zoom: .zero looks to fix this problem thou I need to look at why.

bearing: heading ?? map.mapboxMap.cameraState.bearing,
pitch: pitch ?? map.mapboxMap.cameraState.pitch
)),
coordinatesPadding: padding,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we use coordinatePadding and not camera padding?

maxZoom: nil,
offset: nil
)
} catch {
Logger.log(level: .error, message: "RNMBXCamera.toUpdateItem: Failed to build camera configuration: \(error)")
}
#else
let bounds = CoordinateBounds(southwest: sw, northeast: ne)
#endif

camera = map.mapboxMap.camera(
for: bounds,
padding: padding,
bearing: heading ?? map.mapboxMap.cameraState.bearing,
pitch: pitch ?? map.mapboxMap.cameraState.pitch
)
#endif
}
} else {
camera = CameraOptions(
Expand Down Expand Up @@ -528,8 +554,11 @@ open class RNMBXCamera : RNMBXMapComponentBase {
return false
}

map.mapView.viewport.removeStatusObserver(self)
return super.removeFromMap(map, reason:reason)
withMapView { mapView in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this on remove. Maybe just mapView?.viewport

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the comment exactly - mapView doesn't need optional chaining if acquired in the withMapView callback. What's the concern?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First it should be map.withMapView { } we're in remove so we have a RNMBXMapView we're removing from, passed in as argument .

Second if mapView of RNMBXMapView is null then what was the satusObserver was attached to? Why do we need to remove?

So I think withMapView is unnecessary in this case. Likely not harmful but you have the check carefully as it could easily keep the camera object alive, until we'll have a mapView. Which doesn't sound right, but probably no leak here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong sense of the implications of the callback architecture (_mapCallbacks.append(callback), etc.).

We can do

map._mapView.viewport.removeStatusObserver(self)

but since _mapView is force-unwrapped, I'm not sure if it could be nil when acquired that way. It'd be the only place in the camera module where map.mapView is referenced that way. Do you prefer the map._mapView syntax?

mapView.viewport.removeStatusObserver(self)
}

return super.removeFromMap(map, reason: reason)
}
}

Expand Down Expand Up @@ -568,12 +597,12 @@ extension RNMBXCamera : ViewportStatusObserver {
return "compass"
case .course:
return "course"
case .some(let bearing):
case .some(_):
return "constant"
case .none:
return "normal"
}
} else if let state = state as? OverviewViewportState {
} else if let _ = state as? OverviewViewportState {
return "overview"
} else {
return "custom"
Expand Down
Loading