Support edgePadding with camera, set initial frame to fullscreen #1057

mfazekas · 2020-10-05T09:16:43Z

Test code:

import React from 'react';
import {View, Button} from 'react-native';
import MapboxGL from '@react-native-mapbox-gl/maps';

const latitude = 40.723279;
const longitude = -73.970895;

const boundsEdge = 0.002;

/*
The error is:
Mapbox error [event]:General [code]:-1 [message]:Unable to calculate
appropriate zoom level for bounds. Vertical or horizontal padding is
greater than map's height or width.
Tested on:
- iOS Simulator, iPhone 11
- iOS Simulator, iPhone SE
- Android Emulator, Pixel 3 (API 30)
*/

// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding1 = {
  paddingTop: 0,
  paddingBottom: 0,
};

// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding2 = {
  paddingTop: 0,
  paddingBottom: 63,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding3 = {
  paddingTop: 0,
  paddingBottom: 64,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding4 = {
  paddingTop: 1,
  paddingBottom: 63,
};

// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding5 = {
  paddingTop: 31,
  paddingBottom: 32,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding6 = {
  paddingTop: 32,
  paddingBottom: 32,
};

const initialPadding = padding3;

class MapView extends React.Component {
  static propTypes = {
  };

  state = {
    padding: initialPadding,
  }

  constructor(props) {
    super(props);
  }

  renderContents = () => {
    const point = {
      "type": "Point",
      "coordinates": [longitude, latitude]
    };

    return (
      <MapboxGL.ShapeSource
        id={'source'}
        shape={point}
      >
        <MapboxGL.CircleLayer
          id={'circle-big'}
          style={{
            circleRadius: 20,
            circleColor: 'white',
            circleStrokeColor: 'black',
            circleStrokeWidth: 1,
            iconAllowOverlap: true,
            circlePitchScale: 'viewport',
          }}
        />
      </MapboxGL.ShapeSource>
    );
  };

  render() {
    let {padding} = this.state;
    return (
      <View style={{flex: 1}}>
        <View>
          <Button title="padding1" onPress={() => this.setState({padding: padding1})} />
          <Button title="padding2" onPress={() => this.setState({padding: padding2})} />
          <Button title="padding3" onPress={() => this.setState({padding: padding3})} />
          <Button title="padding4" onPress={() => this.setState({padding: padding4})} />
        </View>
        <MapboxGL.MapView
          ref={(c) => (this._map = c)}
          onPress={this.onPress}
          style={{flex: 1}}
        >
          <MapboxGL.Camera
            zoomLevel={9}
            bounds={{
              ne: [longitude - boundsEdge, latitude + boundsEdge],
              sw: [longitude + boundsEdge, latitude - boundsEdge],
              ...padding,
            }}
          />

          {this.renderContents()}
        </MapboxGL.MapView>
      </View>
      
    );
  }
}

export default MapView;

mfazekas · 2020-10-06T18:41:28Z

Updated testcase:

import React from 'react';
import {View, Button} from 'react-native';
import MapboxGL from '@react-native-mapbox-gl/maps';

const latitude = 40.723279;
const longitude = -73.970895;

const boundsEdge = 0.002;

const aLine = {
  type: 'LineString',
  coordinates: [
    [longitude-boundsEdge, latitude-boundsEdge],
    [longitude-boundsEdge, latitude+boundsEdge],
    [longitude+boundsEdge, latitude+boundsEdge],
    [longitude+boundsEdge, latitude-boundsEdge],
    [longitude-boundsEdge, latitude-boundsEdge],
  ],
};

/*
The error is:
Mapbox error [event]:General [code]:-1 [message]:Unable to calculate
appropriate zoom level for bounds. Vertical or horizontal padding is
greater than map's height or width.
Tested on:
- iOS Simulator, iPhone 11
- iOS Simulator, iPhone SE
- Android Emulator, Pixel 3 (API 30)
*/


// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding1 = {
  paddingTop: 0,
  paddingBottom: 0,
};

// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding2 = {
  paddingTop: 0,
  paddingBottom: 663,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding3 = {
  paddingTop: 0,
  paddingBottom: 664,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding4 = {
  paddingTop: 1,
  paddingBottom: 663,
};

// iOS: No error, but padding has no effect.
// Android: No error, and padding works.
const padding5 = {
  paddingTop: 31,
  paddingBottom: 32,
};

// iOS: Error, and padding has no effect.
// Android: No error, and padding works.
const padding6 = {
  paddingTop: 32,
  paddingBottom: 32,
};

const initialPadding = padding3;

class MapView extends React.Component {
  static propTypes = {
  };

  state = {
    padding: initialPadding,
  }

  constructor(props) {
    super(props);
  }

  renderContents = () => {
    const point = {
      "type": "Point",
      "coordinates": [longitude, latitude]
    };

    return (
      <MapboxGL.ShapeSource
        id={'source'}
        shape={point}
      >
        <MapboxGL.CircleLayer
          id={'circle-big'}
          style={{
            circleRadius: 20,
            circleColor: 'white',
            circleStrokeColor: 'black',
            circleStrokeWidth: 1,
            iconAllowOverlap: true,
            circlePitchScale: 'viewport',
          }}
        />
      </MapboxGL.ShapeSource>
    );
  };

  render() {
    let {padding} = this.state;
    return (
      <View style={{flex: 1}}>
        <View style={{flexDirection: 'row'}}>
          <Button title="p1" onPress={() => this.setState({padding: padding1})} />
          <Button title="p2" onPress={() => this.setState({padding: padding2})} />
          <Button title="p3" onPress={() => this.setState({padding: padding3})} />
          <Button title="p4" onPress={() => this.setState({padding: padding4})} />
        </View>
        <MapboxGL.MapView
          ref={(c) => (this._map = c)}
          onPress={this.onPress}
          style={{flex: 1}}
        >
          {/* zoomLevel={9} */}
            
          <MapboxGL.Camera
            bounds={{
              ne: [longitude - boundsEdge, latitude + boundsEdge],
              sw: [longitude + boundsEdge, latitude - boundsEdge],
              ...padding,
            }}
          />

          {this.renderContents()}
          <MapboxGL.ShapeSource id="idStreetLayer" shape={aLine}>
            <MapboxGL.LineLayer id="idStreetLayer" />
          </MapboxGL.ShapeSource>
        </MapboxGL.MapView>
      </View>
      
    );
  }
}

export default MapView;

naftalibeder

This is working great, thank you! The padding works perfectly, and I left a note about the one issue I was having.

naftalibeder · 2020-10-08T20:34:40Z

ios/RCTMGL/CameraUpdateItem.m

+- (UIEdgeInsets)_clippedPadding:(UIEdgeInsets)padding forView:(RCTMGLMapView*)mapView
+{
+    UIEdgeInsets result = padding;
+    if ((result.top + result.bottom) > mapView.frame.size.height) {
+        double extra =  mapView.frame.size.height / 4.0 + (result.top + result.bottom) - mapView.frame.size.height;
+        if (result.top == 0.0) {
+            result.bottom -= extra;
+        } else if (result.bottom == 0.0) {
+            result.top -= extra;
+        } else {
+            result.top -= extra/2.0;
+            result.bottom -= extra/2.0;
+        }
+    }
+    if ((result.left + result.right) > mapView.frame.size.width) {
+        double extra = mapView.frame.size.width / 4.0 + (result.left + result.right) - mapView.frame.size.width;
+        if (result.left == 0.0) {
+            result.right -= extra;
+        } else if (result.right == 0.0) {
+            result.left -= extra;
+        } else {
+            result.left -= extra/2.0;
+            result.right -= extra/2.0;
+        }
+    }
+    return result;
+}
+


@mfazekas I was still getting the overflow error, and I did a little investigation - it looks like the error occurs if the cumulative padding along one axis is equal to the map dimension (as well as if it exceeds it).

Here's the code I got working correctly - I believe it's a little simpler, but feel free to integrate it however you like.

- (UIEdgeInsets)_clippedPadding:(UIEdgeInsets)padding forView:(RCTMGLMapView*)mapView { UIEdgeInsets result = padding; if (result.top + result.bottom >= mapView.frame.size.height) { double overflow = result.top + result.bottom - mapView.frame.size.height; result.top -= overflow / 2.0 + 1; result.bottom -= overflow / 2.0 + 1; } if (result.left + result.right >= mapView.frame.size.width) { double overflow = result.left + result.right - mapView.frame.size.width; result.left -= overflow / 2.0 + 1; result.right -= overflow / 2.0 + 1; } return result; }

@naftalibeder I'm ambivalent about this. It's hard to correct padding in a sensible way once they are over the size of bounds.
Maybe it's correct for the framework to just complain.

I think this correction is something the app is also capable of doing. Eg get the diemnsion of map before setting up padding, and correct padding, if it's too large.

@mfazekas I'd like to try to make the case for it, if you don't mind.

Other layout examples I can think of tend to fail silently, and just try to intelligently guess what the best outcome would be. For example, 'illegal' CSS configurations still allow a page to render, and even ScrollView edge insets that sum to greater than the view dimension in iOS only throw a warning.

And regarding the app handling this, I think a declarative UI is a worse place to need to do it. Callbacks are asynchronous so it requires handling a number of edge cases that this library would not need to handle. I did attempt to do this, and it required a surprising amount of code just to avoid this crash, and I still wasn't confident that I had prevented it completely. The clipped method above, by contrast, actually guarantees the error will never be thrown.

I'm certainly open to working out a slightly different way for the overflow inset to distribute across the two edges, but I feel pretty strongly the app should not crash because of out-of-bounds layout configuration. Thoughts?

ferdicus

sorry, I'm a bit out of my element here - would have to trust you and @naftalibeder on this one 👍

As I understood it you're adjusting the padding just for iOS - this isn't an issue on Android (did we already adjust for this there)?
Sometimes I wonder why things like this aren't unified by the upstream SDKs 😅

naftalibeder · 2020-10-15T19:25:21Z

@ferdicus That's correct, this would only affect iOS. I actually just opened up a PR that fixes the same issue on Android, which you can see linked above.

mfazekas · 2020-10-16T14:45:55Z

@naftalibeder i've update padding code, pls review:

- (UIEdgeInsets)_clippedPadding:(UIEdgeInsets)padding forView:(RCTMGLMapView*)mapView
{
    UIEdgeInsets result = padding;
    if ((result.top + result.bottom) >= mapView.frame.size.height) {
        double totalPadding = result.top + result.bottom;
        double extra = totalPadding - mapView.frame.size.height + 2.0;
        result.top -= (result.top * extra) / totalPadding;
        result.bottom -= (result.bottom * extra) / totalPadding;
    }
    if ((result.left + result.right) >= mapView.frame.size.width) {
        double totalPadding = result.left + result.right;
        double extra = totalPadding - mapView.frame.size.width + 2.0;
        result.left -= (result.left * extra) / totalPadding;
        result.right -= (result.right * extra) / totalPadding;
    }
    return result;
}

The only way it's improvement on yours is that it will not make paddings negative.
With an 500 height and paddingTop:2 paddingBottom:600 this correct to something like paddingTop:1.9 paddingBottom: 496.1 while yours would correct to paddingTop: -52 paddingBottom: 550

naftalibeder · 2020-10-16T15:34:11Z

Nice, good catch!

Why add 2.0? Is it to have a full extra point on each edge? (Not a problem, just wondering if it's aesthetic or necessary, since these are double values.)
Since the lines like (result.top * extra) / totalPadding are really about multiplying a padding element by a ratio, I might consider being more explicit about that (see below).
Personally I would prefer something like paddingNew, which only gets mutated and doesn't get referenced, to prevent accidentally introducing side effects.

- (UIEdgeInsets)_clippedPadding:(UIEdgeInsets)padding forView:(RCTMGLMapView*)mapView
{
    UIEdgeInsets paddingNew = padding;
    if ((padding.top + padding.bottom) >= mapView.frame.size.height) {
        double totalPadding = padding.top + padding.bottom;
        double extra = totalPadding - mapView.frame.size.height + 2.0;
        paddingNew.top = padding.top * (1 - extra / totalPadding);
        paddingNew.bottom = padding.bottom * (1 - extra / totalPadding);
    }
    if ((padding.left + padding.right) >= mapView.frame.size.width) {
        double totalPadding = padding.left + padding.right;
        double extra = totalPadding - mapView.frame.size.width + 2.0;
        paddingNew.left = padding.left * (1 - extra / totalPadding);
        paddingNew.right = padding.right * (1 - extra / totalPadding);
    }
    return paddingNew;
}

These tweaks are up to you though. if it works, it works!

mfazekas · 2020-10-16T16:27:24Z

Why add 2.0? Is it to have a full extra point on each edge? (Not a problem, just wondering if it's aesthetic or necessary, since these are double values.)

First we need some minimal delta because of floating point math. Then mapbox alert might use int conversion, not sure but using some extra 0.1 was not enough. Using 1.0 seems to worked so changed to that.

Since the lines like (result.top * extra) / totalPadding are really about multiplying a padding element by a ratio, I might consider being more explicit about that (see below).

Personally I would prefer something like paddingNew, which only gets mutated and doesn't get referenced, to prevent accidentally introducing side effects.

Agree refactored code.

naftalibeder · 2020-10-16T16:31:05Z

Looks great! I'll bring this logic into the PR I opened for Android.

ferdicus · 2020-10-16T18:34:53Z

example/ios/RNMapboxGLExample.xcodeproj/project.pbxproj

@@ -757,6 +757,7 @@
 					"FB_SONARKIT_ENABLED=1",
 				);
 				INFOPLIST_FILE = RNMapboxGLExample/Info.plist;
+				IPHONEOS_DEPLOYMENT_TARGET = 10.0;


why is this in this PR?

Lazyness. Because travis builds are slow.

iOS in travis is broken i assume since the RN 63.3 upgrade, it was an attempt to fix.

:)

Does this also explain why there are no checks in the PRs, or is that unrelated?

@ferdicus i think that should improve now, with travis-ci.org => travis-ci.com migration

mfazekas changed the title ~~Support edgePadding with camera, set intiial frame to larger~~ Support edgePadding with camera, set initial frame to fullscreen Oct 5, 2020

mfazekas requested a review from ferdicus October 5, 2020 17:28

mfazekas mentioned this pull request Oct 6, 2020

Padding error on iOS: Unable to calculate appropriate zoom level for bounds #1032

Closed

naftalibeder reviewed Oct 8, 2020

View reviewed changes

ferdicus mentioned this pull request Oct 15, 2020

PaddingBottom on Camera fitBounds does not work anymore #841

Closed

ferdicus reviewed Oct 15, 2020

View reviewed changes

naftalibeder mentioned this pull request Oct 15, 2020

Prevent padding from exceeding bounds on Android #1072

Closed

mfazekas added 4 commits October 16, 2020 14:07

Support edgePadding with camera, set intiial frame to larger

29d5288

Use edgePadding instead of contentInset

2ab04ab

Clip padding if it's too large

b439d8a

Revised _clippedPadding logic

9ff2da7

mfazekas force-pushed the mfazekas/fix-boudnsPadding-ios branch from 123ad52 to 9ff2da7 Compare October 16, 2020 14:42

Fix isCoordValid

0158e09

mfazekas added 2 commits October 16, 2020 18:11

Padding code update

2c39f86

Up IPHONEOS_DEPLOYMENT_TARGET to 10.0

33c5fb1

mfazekas merged commit 9103f2d into master Oct 16, 2020

ferdicus reviewed Oct 16, 2020

View reviewed changes

ferdicus deleted the mfazekas/fix-boudnsPadding-ios branch August 20, 2021 10:18

Andarius mentioned this pull request Jun 30, 2022

feat: adding back bounds #2015

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support edgePadding with camera, set initial frame to fullscreen #1057

Support edgePadding with camera, set initial frame to fullscreen #1057

mfazekas commented Oct 5, 2020 •

edited

Loading

mfazekas commented Oct 6, 2020

naftalibeder left a comment

naftalibeder Oct 8, 2020 •

edited

Loading

mfazekas Oct 9, 2020

naftalibeder Oct 9, 2020

ferdicus left a comment •

edited

Loading

naftalibeder commented Oct 15, 2020

mfazekas commented Oct 16, 2020

naftalibeder commented Oct 16, 2020

mfazekas commented Oct 16, 2020

naftalibeder commented Oct 16, 2020

ferdicus Oct 16, 2020

mfazekas Oct 16, 2020

ferdicus Oct 16, 2020

mfazekas Oct 16, 2020

Support edgePadding with camera, set initial frame to fullscreen #1057

Support edgePadding with camera, set initial frame to fullscreen #1057

Conversation

mfazekas commented Oct 5, 2020 • edited Loading

mfazekas commented Oct 6, 2020

naftalibeder left a comment

Choose a reason for hiding this comment

naftalibeder Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

mfazekas Oct 9, 2020

Choose a reason for hiding this comment

naftalibeder Oct 9, 2020

Choose a reason for hiding this comment

ferdicus left a comment • edited Loading

Choose a reason for hiding this comment

naftalibeder commented Oct 15, 2020

mfazekas commented Oct 16, 2020

naftalibeder commented Oct 16, 2020

mfazekas commented Oct 16, 2020

naftalibeder commented Oct 16, 2020

ferdicus Oct 16, 2020

Choose a reason for hiding this comment

mfazekas Oct 16, 2020

Choose a reason for hiding this comment

ferdicus Oct 16, 2020

Choose a reason for hiding this comment

mfazekas Oct 16, 2020

Choose a reason for hiding this comment

mfazekas commented Oct 5, 2020 •

edited

Loading

naftalibeder Oct 8, 2020 •

edited

Loading

ferdicus left a comment •

edited

Loading