Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[android] Setting padding moves camera #15412

Closed
2Rec opened this issue Aug 19, 2019 · 8 comments · Fixed by #15437
Closed

[android] Setting padding moves camera #15412

2Rec opened this issue Aug 19, 2019 · 8 comments · Fixed by #15437
Labels
Android Mapbox Maps SDK for Android needs information

Comments

@2Rec
Copy link

2Rec commented Aug 19, 2019

In previous Mapbox versions setting padding doesn't couse camera movement.

Steps to reproduce

  1. map.setPadding(0,0,1000,0)

Expected behavior

No camera movement

Actual behavior

Camera flips by padding value

Configuration

Android versions: 8.2.1
Device models: HTC U11
Mapbox SDK versions: 8.2.1

@astojilj
Copy link
Contributor

Is the problem that camera padding is now animated movement?

Otherwise, please check issues #15269 and #15199 as this is related.

As mentioned in #15199. padding pushes center of content area to area defined by padding. 1000 is too large IMHO - as explained in #15411 (comment).

@LukasPaczos LukasPaczos added the Android Mapbox Maps SDK for Android label Aug 19, 2019
@2Rec
Copy link
Author

2Rec commented Aug 19, 2019

Let's assume it's correct padding. It's move center as desired to approximatly 78% of screen height. Problem is that it's scrolling map camera with the same value. Previous versions 6.x 7.x didn't work like that.

@astojilj
Copy link
Contributor

astojilj commented Aug 19, 2019

Let's assume it's correct padding. It's move center as desired to approximatly 78% of screen height. Problem is that it's scrolling map camera with the same value. Previous versions 6.x 7.x didn't work like that.

Could you please take screenshot or video of the wrong behavior - with a note about expected and actual behavior? It sounds contradictory that moving center is desired but map scrolling is not. Thanks.

Edit: in the original pull request #14664, you'd find animated gifs that might help clarifying the issue.

@2Rec
Copy link
Author

2Rec commented Aug 20, 2019

Screenshot_20190820-105439
Screenshot_20190820-105443

  1. before padding change
  2. after padding change
    There's no need to automatically move camera like in previous versions or at least give a choice

@astojilj
Copy link
Contributor

astojilj commented Aug 20, 2019

@2Rec
Thanks for sharing the screenshots.

before padding change
after padding change

It is not obvious to me what before/after padding change means - is it about 1. previous version vs current version of SDK, or 2. before calling app.setPadding(0,0,1000,0) and after calling ap.setPadding(0,0,1000,0).

if it is case 2, please share further information, e.g. what was padding before.

Let me analyze the images.

If you set padding (0,0,1000,0) and set camera to user location, it should be like on image 1.
Is this what you see and the expected result?

Let's compare it to screenshot of our Android example: #14664 (comment)

Center point set using this code is moved to the center of padded area.

If that is not what you see, let's try to debug the issue.

  • what code you use to set the user location?
  • are you offsetting camera center position in your code to get it to bottom?
  • could you debug if there is other code resetting padding to 0?

Second screenshot looks like as if padding is set to 0 or if there is code that is calculating center that is offset from user location and setting camera to it.

Please share the code snippets for setting camera and location and it should help us identify the issue.

@2Rec
Copy link
Author

2Rec commented Aug 20, 2019

ok, true first image is padding (0,0, value, 0), second is after clearing (0,0,0,0)
still if u have padding (0,0,0,0) and set to (0,0,value,0) it's the same behaviour

  • i can't share a code for user location and it's no need
  • no offset set and no other moveCamera etc. functions called

It's possible to make workaround by moving Camera to have it in the same place (before and after padding change) (but for what?)

@LukasPaczos
Copy link
Contributor

LukasPaczos commented Aug 20, 2019

Hey @2Rec! In previous versions of the SDK, going from padding (0, 0, value, 0) to (0, 0, 0, 0) would not adjust the camera offset until the next camera animation was invoked. Now, the camera immediately reflects the applied padding. If you do not wish to move the camera position back to the center of the screen, you shouldn't reset the padding to (0, 0, 0, 0). This change was introduced to fix #14985.

That said, I can see a scenario where this brings additional challenges, for example:

  • We're navigating with top padding applied (0, 0, value, 0), the user clicks a POI and camera should travel to the POI that's centered on the screen. With the current behavior, the camera would jump when padding (0, 0, 0, 0) is applied to achieve that.

Is this scenario similar to what you're struggling with?

I think to resolve this issue we should do the following steps:

  1. Revert Invalidate camera when setting a padding #14989, introduce a boolean flag that updates the viewport if required (like LocationComponent invalidation when padding changes #14985) and release a patch:
void NativeMapView::setContentPadding(JNIEnv&, float top, float left, float bottom, float right, bool update) {
    insets = {top, left, bottom, right};
    if (update) {
        // invalidate the camera position to consider the new padding
        map->jumpTo(map->getCameraOptions(insets));
    }
}

or just invalidate the camera internally and suggest that in examples when setting the padding.

  1. Expose padding field in the CameraPosition which would unblock above scenario because we could call mapboxMap.animateCamera(CameraUpdateFactory.newLatLngPadding(latLng, 0, 0, 0, 0)) to achieve a smooth padding transition.

  2. Possibly deprecate MapboxMap#setPadding in favor of applying the padding with camera movement only to limit entry points and make the API easier to use. For example, CameraUpdateFactory.newPadding(0, value, 0, 0). I believe this is also the case on iOS @julianrex, that we can only apply the padding using the camera options?

@2Rec
Copy link
Author

2Rec commented Aug 20, 2019

@LukasPaczos
Exactly, first point i like most, 2 and 3 are nice additional features.
Pozdro :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android needs information
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants