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

Map.flyTo padding not being cleared -> fitBounds is summing it up with it's own padding argument. #11831

Closed
sienki-jenki opened this issue Apr 27, 2022 · 20 comments
Labels

Comments

@sienki-jenki
Copy link

mapbox-gl-js version: 2.8.2 but also present in 2.7.1

browser: Google Chrome 100.0.4896.127 64-bit

Steps to Trigger Behavior

  1. Call function map.FlyTo with padding ->
 map.flyTo({
   center: coordinates[0],
   padding: {
      top: 50,
      bottom: 0,
      left: 0,
      right: 0
   }
})
  1. Call function map.fitBounds ->
map.fitBounds(bounds, {
 padding: 0
});

Link to Demonstration

https://codepen.io/sienki-jenki/pen/xxpvPza

Mapbox.flyTo.fitBounds.padding.bug.-.27.April.2022.mp4

When you open site, calling fitBounds will work correctly, however if you call flyTo, padding is getting saved in some state and is never cleared, meaning fitBounds is somehow summing up padding passed to flyTo with it's own passed paddings as argument. As you can see on video, 50px of top padding is added to fitBounds even when 0 is an argument.

Expected Behavior

Map fits linestring to view without padding, because padding: 0.

Actual Behavior

Map firts linestring to view with sum of paddings, meaning paddings passed to flyTo function are added to paddings of fitBounds.

@lchop
Copy link

lchop commented May 23, 2022

+1

@Kambyses
Copy link

Kambyses commented May 27, 2022

Yes, i ran into same problem, and did a quick fix myself. Note that i assume that map is initialized with default fitBoundsOptions.padding and padding is alway given in form of { top, left, right, bottom }

class Map extends mapboxgl.Map {
  
  _cameraForBoxAndBearing ( p0, p1, bearing, options ) {
    if ( options && options.padding && this.options && this.options.fitBoundsOptions && this.options.fitBoundsOptions.padding && mapboxgl.version === "2.8.2" ) {
      const defaultPadding    = this.options.fitBoundsOptions.padding;
      const edgePadding       = this.transform.padding;
      options.padding.top     = defaultPadding.top - edgePadding.top + options.padding.top;
      options.padding.left    = defaultPadding.left - edgePadding.left + options.padding.left;
      options.padding.right   = defaultPadding.right - edgePadding.right + options.padding.right;
      options.padding.bottom  = defaultPadding.bottom - edgePadding.bottom + options.padding.bottom;
    }
    return super._cameraForBoxAndBearing( p0, p1, bearing, options );
  }

}

@Soneliem
Copy link

Soneliem commented Nov 8, 2022

We have also had this issue but in reverse. Using flyTo and then fitBounds also creates different issues related to the same bug.

CodePen with example

  • On a landscape view where there is enough horizontal space on the right, the padding gets summed. (600px + 600px)

  • On a portrait view (our use case) where the 1200px width does not exist, MapBox refuses to fit bounds with the following error: Map cannot fit within canvas with the given bounds, padding, and/or offset.

Our temporary solution is to wait for the camera to stop moving usingisMoving and then fitBounds

@mathhulk
Copy link

mathhulk commented Jan 15, 2023

Both map.fitBounds and map.fitScreenCoordinates utilize map._cameraForBounds (or map.cameraForBounds) to account for padding via the zoom and center options and adjust the specified EasingOptions accordingly.

They then utilize map._fitInternal which removes the padding from the specified EasingOptions because "calculatedOptions already accounts for padding by setting zoom and center accordingly" and then executes either map.easeTo or map.flyTo.

delete options.padding;

However, map.easeTo and map.flyTo both set padding based on the padding specified in EasingOptions, which we know has been removed by map._fitInternal, and any existing padding. In this case, using map.fitBounds after map.flyTo will result in the padding set by map.flyTo to remain until map.flyTo, map.jumpTo, or map.easeTo are called.

A temporary solution would be to call map.setPadding and correct for the discrepancy yourself. I might try to tackle this myself and open a pull request, but I can't guarantee anything.

@mathhulk
Copy link

Both map.fitBounds and map.fitScreenCoordinates utilize map._cameraForBounds (or map.cameraForBounds) to account for padding via the zoom and center options and adjust the specified EasingOptions accordingly.

They then utilize map._fitInternal which removes the padding from the specified EasingOptions because "calculatedOptions already accounts for padding by setting zoom and center accordingly" and then executes either map.easeTo or map.flyTo.

delete options.padding;

However, map.easeTo and map.flyTo both set padding based on the padding specified in EasingOptions, which we know has been removed by map._fitInternal, and any existing padding. In this case, using map.fitBounds after map.flyTo will result in the padding set by map.flyTo to remain until map.flyTo, map.jumpTo, or map.easeTo are called.

A temporary solution would be to call map.setPadding and correct for the discrepancy yourself. I might try to tackle this myself and open a pull request, but I can't guarantee anything myself.

Although confusing, this seems to be the intended behavior according to a unit test. I think this should either be documented or the behavior should be normalized.

t.test('padding does not get propagated to transform.padding', (t) => {

@mathhulk
Copy link

Both map.fitBounds and map.fitScreenCoordinates utilize map._cameraForBounds (or map.cameraForBounds) to account for padding via the zoom and center options and adjust the specified EasingOptions accordingly.

They then utilize map._fitInternal which removes the padding from the specified EasingOptions because "calculatedOptions already accounts for padding by setting zoom and center accordingly" and then executes either map.easeTo or map.flyTo.

delete options.padding;

However, map.easeTo and map.flyTo both set padding based on the padding specified in EasingOptions, which we know has been removed by map._fitInternal, and any existing padding. In this case, using map.fitBounds after map.flyTo will result in the padding set by map.flyTo to remain until map.flyTo, map.jumpTo, or map.easeTo are called.

A temporary solution would be to call map.setPadding and correct for the discrepancy yourself. I might try to tackle this myself and open a pull request, but I can't guarantee anything.

Another solution: replace the line I linked with the following.

if (options.padding) extend(this.transform.padding, options.padding);

I tried to implement this myself, but I couldn't get testing to work and I don't have the time to spend troubleshooting.

@michaelklopf
Copy link

I don't know if the question is related, but we have the problem that calling fitBounds on our map with new Mapbox.LngLatBounds does not adapt the map to the new bounds. The center of the map is not changed to the new center computed by map.cameraForBounds.

Instead the center of the map after the _fitInternal call is still the default center we set when setting up the map.

@andrewharvey
Copy link
Collaborator

The map has a global padding which has get/set methods map.getPadding, map.setPadding and for debugging map.showPadding = true.

If you pass a padding option to easeTo and flyTo, they set the global padding.

If you call fitBounds without a padding option, it will respect the global bounds.

If you call fitBounds with a padding option, it will respect the global bounds and in addition apply your padding options.

Neither of these calls to fitBounds will set the global bounds though.

In my view, it's unexpected that easeTo/flyTo set the global bounds as a side affect. I feel the global bounds should only be set on map.setPadding which is respected by easeTo/flyTo/fitBounds and any padding options you pass to those methods is applied in addition to the global padding.

Given the current behaviour would have worked it's way into applications, any changes would be breaking changes imo, but would the confusion.

@npruzaniec
Copy link

npruzaniec commented Aug 23, 2023

I am not able to get the padding to be respected with fitBounds after using flyTo and I have confirmed the global padding is still set.

On first load padding looks good.
Screenshot 2023-08-23 at 3 37 51 PM

Then, after using flyTo then resetting to the full map with fitBounds (exact same coordinates I loaded the map with), the padding is not respected.
Screenshot 2023-08-23 at 3 37 33 PM

You can see the padding is still set with the showPadding debug lines showing.

I have tried every version of setting and resetting the padding at different points in the process but have no luck with fitBounds on the subsequent runs.

@kamil-sienkiewicz-asi
Copy link
Contributor

bump 😢

@npruzaniec
Copy link

Same...

1 similar comment
@12finger
Copy link

Same...

@kamil-sienkiewicz-asi
Copy link
Contributor

@stepankuzmin Could you elaborate why this issue is closed? It got fixed in recent releases? Could you link PR/Release notes for that? 🙏

@stepankuzmin
Copy link
Contributor

Hi @kamil-sienkiewicz-asi,

Yes, this was fixed, and GitHub automatically closed the issue. It will be available in the next v3.2.0 release.

@timcreatedit
Copy link

Is this actually fixed? I can't get flyTo to work with padding properly at all anymore. The padding doesn't animate and doesn't seem to go to the right values.

@stepankuzmin
Copy link
Contributor

stepankuzmin commented Mar 7, 2024

Hi, @timcreatedit. Could you please make a repro? I've tried the original repo with v3.2.0, which seems to be working.

The changelog for v3.2.0 is available here https://github.com/mapbox/mapbox-gl-js/releases/tag/v3.2.0

@kamil-sienkiewicz-asi
Copy link
Contributor

@stepankuzmin @timcreatedit is right. Right now first invocation map.flyTo ignores padding.

See: https://codepen.io/kamil-sienkiewicz-asi/pen/BaEZmam

Click "Fly" wait for map till complete stop and then click Fly again. Second time padding is respected.

@kamil-sienkiewicz-asi
Copy link
Contributor

Actually it seems like map.easeTo is right now saving padding and then fitBounds is using that :| Same case as with flyTo

@ricardoweiss
Copy link

Yes, also here happening that the flyTo is not respecting the padding property anymore, this issue has been happening for more than a month

@mourner
Copy link
Member

mourner commented Apr 23, 2024

@ricardoweiss yep, tracking this in #13152...

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

No branches or pull requests