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

wants to merge 16 commits into from
+61 −30

Conversation

naftalibeder
Copy link
Collaborator

@naftalibeder naftalibeder commented Aug 29, 2024 •
edited
Loading

Description

Fixes camera bounds-with-padding bug in v11 described in mapbox/mapbox-maps-ios#2134.

Checklist

  • I've read CONTRIBUTING.md
  • I updated the doc/other generated code with running yarn generate in the root folder
  • I have tested the new feature on /example app.
    • In V11 mode/ios
    • In New Architecture mode/ios
    • In V11 mode/android
    • In New Architecture mode/android
  • I added/updated a sample - if a new feature was implemented (/example)

@naftalibeder naftalibeder temporarily deployed to CI with Mapbox Tokens August 29, 2024 21:13 — with GitHub Actions Inactive
@naftalibeder naftalibeder temporarily deployed to CI with Mapbox Tokens August 29, 2024 21:13 — with GitHub Actions Inactive
@naftalibeder naftalibeder temporarily deployed to CI with Mapbox Tokens August 29, 2024 21:13 — with GitHub Actions Inactive
@naftalibeder naftalibeder temporarily deployed to CI with Mapbox Tokens August 29, 2024 21:13 — with GitHub Actions Inactive
@naftalibeder naftalibeder temporarily deployed to CI with Mapbox Tokens August 29, 2024 21:13 — with GitHub Actions Inactive
@naftalibeder
Copy link
Collaborator Author

@mfazekas What would be required to get this merged?

mfazekas
mfazekas approved these changes Sep 12, 2024
View reviewed changes
ios/RNMBX/RNMBXCamera.swift Show resolved Hide resolved
ios/RNMBX/RNMBXCamera.swift Outdated
@@ -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?

@naftalibeder
Copy link
Collaborator Author

@mfazekas what would be involved in getting this merged? Happy to make changes, but it seems fairly straightforward - please let me know.

Gnative
Gnative reviewed Oct 16, 2024
View reviewed changes
ios/RNMBX/RNMBXCamera.swift
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.

mfazekas
mfazekas reviewed Oct 27, 2024
View reviewed changes
ios/RNMBX/RNMBXCamera.swift
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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gnative Gnative Gnative left review comments

@mfazekas mfazekas mfazekas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@naftalibeder @mfazekas @Gnative