[core] Asymmetric viewport when edge insets are specified #14664

astojilj · 2019-05-14T13:02:53Z

The change is implemented in TransformState::getProjMatrix, the rest of the code is making sure that existing API contracts stay and there are tests verifyingrendering and render query processing only items within screen and given tolerance around screen edges.

MapView: don't bake edge insets into relalculated camera center. Keep edge insets as property of camera in TransformState (similar to pitch, zoom, bearing) independent from specified camera center. Interpolate edge insets in animation.

iOS Demo app: "Turn On/Off Content Insets" pitch the camera and navigate to convenient location in Denver, where streets are parallel to cardinal directions, to illustrate viewport center offset when edge insets are set.

Tests:
ViewFrustumCulling: although Annotations are deprecated, queryRenderedFeatures related tests in Annotations would need to get ported and decided to add the edge insets related query tests next to them. Verify frustum culling (render+queryRenderedFeatures) With different camera and edge insets setups. TODO: port Annotations tests.

Transform.Padding: Verify that coordinates take proper place on screen after applying edge insets.

LocalGlyphRasterizer: verify text rendering when applying padding.

Related to #11882: both use projection matrix elements [8] and [9].

Alternative approach to this was to increase and offset map origin so that the screen would be a sub-rectangle in larger map viewport. This approach has a drawback of unecessary processing the items that are outside screen area.

Addreses: mapbox/mapbox-navigation-ios#1845

Fixes: #12728, #12107, https://github.com/mapbox/navigation-sdks/issues/120, #14362

Example1: Turn on/off content half-width left edge inset in horizontal mode. Two finger rotate at the end.

Example2: quarter-height top and quarter width right edge insets.

pozdnyakov

core LGTM

julianrex · 2019-05-16T14:56:36Z

@fabian-guerra @d-prukop would you mind looking at this from the SDKs' POV please?

astojilj · 2019-05-17T17:01:21Z

@fabian-guerra , @d-prukop, @frederoni
This fixes issues noticeable on iOS as e.g. iPhone X always has edge insets specified: top inset is larger in portrait mode.

The issue #14362 explains the problem well; user specify camera parameters, but because there is content inset, camera values that user reads back are not the same as specified (inset moves center).

I put focus on honouring existing SDK documentation and contracts and find that the code is in line with it. Render tests pass :)

However, I "might" have stretched the interpretation of the documentation. Please comment:

MGLMapCamera.centerCoordinate

Coordinate at the center of the map view.

It doesn't say center of screen and edge insets documentation further confirms it:

Edge Insets:

The distance from the edges of the map view’s frame to the edges of the map view’s logical viewport. When the value of this property is equal to UIEdgeInsetsZero, viewport properties such as centerCoordinate assume a viewport that matches the map view’s frame. Otherwise, those properties are inset, excluding part of the frame from the viewport. For instance, if the only the top edge is inset, the map center is effectively shifted downward.

This is how I interpreted terms map, map view and map view's frame:
It clearly states that the center of map is shifted. Center of map is the same as center of the map view.
Center of map view is then not the same as center of map's view frame (screen).

I wouldn't use both camera viewport center and center of view in documentation - only viewport should suffice and some of the complicated explanations, like the one above, could be trimmed.
Anyway, I need your opinion here. Thanks.

1ec5 · 2019-05-17T20:48:19Z

It doesn't say center of screen and edge insets documentation further confirms it:

Right, MGLMapCamera.centerCoordinate matches MGLMapView.centerCoordinate, which already accounts for any edge padding. If the developer needs the absolute center without accounting for edge padding, they need to pass UIView.center into -[MGLMapView convertPoint:toCoordinateFromView:].

fabian-guerra

Thank you for fixing this. I added some nits and a question. Also could you please add an entry in the changelogs.

platform/ios/app/MBXViewController.m

platform/ios/src/MGLMapView.mm

astojilj · 2019-05-23T13:24:25Z

@tobrun,
I have verified this on Android using Mapbox Android SDK TestApp's Map Padding activity: zoomed out to Denver (Denver streets parallel to cardinal directions), tilted the map and aligned to north. On image before (with master) it is obvious that projection center is in the middle of the screen while, with the patch, it is in the center of padded area.
Note that there was no Android specific code changed.

fabian-guerra

Just a minor blurb change. LGTM.

platform/ios/CHANGELOG.md

tobrun · 2019-05-28T09:48:43Z

re #14664 (comment), looks great @astojilj!

The change is implemented in TransformState::getProjMatrix, the rest of the code is making sure that existing API contracts stay and there are tests verifyingrendering and render query processing only items within screen and given tolerance around screen edges. MapView: don't bake edge insets into relalculated camera center. Keep edge insets as property of camera in TransformState (similar to pitch, zoom, bearing) independent from specified camera center. Interpolate edge insets in animation. iOS Demo app: "Turn On/Off Content Insets" pitch the camera and navigate to convenient location in Denver, where streets are parallel to cardinal directions, to illustrate viewport center offset when edge insets are set. Tests: ViewFrustumCulling: although Annotations are deprecated, queryRenderedFeatures related tests in Annotations would need to get ported and decided to add the edge insets related query tests next to them. Verify frustum culling (render+queryRenderedFeatures) With different camera and edge insets setups. TODO: port Annotations tests. Transform.Padding: Verify that coordinates take proper place on screen after applying edge insets. LocalGlyphRasterizer: verify text rendering when applying padding. Related to #11882: both use projection matrix elements [8] and [9]. Alternative approach to this was to increase and offset map origin so that the screen would be a sub-rectangle in larger map viewport. This approach has a drawback of unecessary processing the items that are outside screen area. Fixes #12107, #12728, navigation-sdks/issues/120

Check edge insets difference, in addition to isEqualToMapCamera in all the places in MGLMapView before map.easeTo/map.flyTo.

platform/ios/CHANGELOG.md

platform/macos/CHANGELOG.md

@1ec5

Allows changing camera when user duplicates content insets. Related to previous patch and comment: #14664 (review) Thanks @1ec5

To changelog: Fixed incorrect center coordinate after pinch regression caused by edge insets fix (#14664). While working on #14664, missed to understand the logic used in ``` CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate; mbgl::EdgeInsets padding { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x }; self.mbglMap.jumpTo(mbgl::CameraOptions() .withCenter(MGLLatLngFromLocationCoordinate2D(centerCoordinate)) .withPadding(padding)); ``` Replacing this code by moveBy achieves the required translation. Fixes: #14977, #15082

Viewport center offset usage was wrongly submitted in #14664. It was part of alternative approach that used enlarged viewport. Existing and added tests were not sufficient to spot the regression, since the collision check padding is usually larger than the center offset x and y. Annotation picking has tolerance of only 10 pixels but no annotation integration test was using content insets. Usage of offset is not needed because `posMatrix` in e.g. `CollisionIndex::projectPoint(const mat4& posMatrix, const Point<float>& point)` already incorporates center offset (projection matrix) and the code in current master was just offsetting all by the value. Modified [ios] MGLAnnotationViewIntegrationTests.testSelectingAnnotationWithCenterOffset to use different insets. It verifies the fix. Fixes [iOS] Annotations are not selectable (added via iosapp menu) #15106: In case of #15106, view's original content insets is {top:88, bottom:34}, causing that center offset is {x:0, y:27} and selection with tolerance of 10 wouldn't select annotation. After tapping the view, so that the header gets removed, view's content insets get changed to {top:44, bottom:34}, center offset is {x:0, y:5} and annotation selection would work, as described in #15106. Fixes: #15106

To changelog: Fixed incorrect center coordinate after pinch regression caused by edge insets fix (#14664). While working on #14664, missed to understand the logic used in ``` CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate; mbgl::EdgeInsets padding { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x }; self.mbglMap.jumpTo(mbgl::CameraOptions() .withCenter(MGLLatLngFromLocationCoordinate2D(centerCoordinate)) .withPadding(padding)); ``` Replacing this code by moveBy achieves the required translation. Fixes: #14977, #15082

Viewport center offset usage was wrongly submitted in #14664. It was part of alternative approach that used enlarged viewport. Existing and added tests were not sufficient to spot the regression, since the collision check padding is usually larger than the center offset x and y. Annotation picking has tolerance of only 10 pixels but no annotation integration test was using content insets. Usage of offset is not needed because `posMatrix` in e.g. `CollisionIndex::projectPoint(const mat4& posMatrix, const Point<float>& point)` already incorporates center offset (projection matrix) and the code in current master was just offsetting all by the value. Modified [ios] MGLAnnotationViewIntegrationTests.testSelectingAnnotationWithCenterOffset to use different insets. It verifies the fix. Fixes [iOS] Annotations are not selectable (added via iosapp menu) #15106: In case of #15106, view's original content insets is {top:88, bottom:34}, causing that center offset is {x:0, y:27} and selection with tolerance of 10 wouldn't select annotation. After tapping the view, so that the header gets removed, view's content insets get changed to {top:44, bottom:34}, center offset is {x:0, y:5} and annotation selection would work, as described in #15106. Fixes: #15106

To changelog: Fixed incorrect center coordinate after pinch regression caused by edge insets fix (#14664). While working on #14664, missed to understand the logic used in ``` CLLocationCoordinate2D centerCoordinate = _previousPinchCenterCoordinate; mbgl::EdgeInsets padding { centerPoint.y, centerPoint.x, self.size.height - centerPoint.y, self.size.width - centerPoint.x }; self.mbglMap.jumpTo(mbgl::CameraOptions() .withCenter(MGLLatLngFromLocationCoordinate2D(centerCoordinate)) .withPadding(padding)); ``` Replacing this code by moveBy achieves the required translation. Fixes: #14977, #15082

Viewport center offset usage was wrongly submitted in #14664. It was part of alternative approach that used enlarged viewport. Existing and added tests were not sufficient to spot the regression, since the collision check padding is usually larger than the center offset x and y. Annotation picking has tolerance of only 10 pixels but no annotation integration test was using content insets. Usage of offset is not needed because `posMatrix` in e.g. `CollisionIndex::projectPoint(const mat4& posMatrix, const Point<float>& point)` already incorporates center offset (projection matrix) and the code in current master was just offsetting all by the value. Modified [ios] MGLAnnotationViewIntegrationTests.testSelectingAnnotationWithCenterOffset to use different insets. It verifies the fix. Fixes [iOS] Annotations are not selectable (added via iosapp menu) #15106: In case of #15106, view's original content insets is {top:88, bottom:34}, causing that center offset is {x:0, y:27} and selection with tolerance of 10 wouldn't select annotation. After tapping the view, so that the header gets removed, view's content insets get changed to {top:44, bottom:34}, center offset is {x:0, y:5} and annotation selection would work, as described in #15106. Fixes: #15106

Decouple dependency content insets -> anchor point -> padding/content insets broken after mapbox/mapbox-gl-native#14664 introduced animated interpolation for padding change. Remove the need to specify center for puck view - use the approach where content inset is keeping it centered. Take safeArea into account when calculating contentInsets. Addresses: mapbox/mapbox-gl-native#15232, mapbox/mapbox-gl-native#15233 Related to: #2165, Fixes: #2145

astojilj requested a review from a team May 14, 2019 13:02

astojilj self-assigned this May 14, 2019

astojilj requested review from brunoabinader, pozdnyakov, 1ec5 and kkaefer May 14, 2019 13:09

astojilj mentioned this pull request May 15, 2019

Updating and restoring camera pitch causes a shift north #14362

Closed

pozdnyakov approved these changes May 16, 2019

View reviewed changes

julianrex requested review from d-prukop and fabian-guerra and removed request for a team May 16, 2019 14:55

fabian-guerra reviewed May 22, 2019

View reviewed changes

astojilj mentioned this pull request May 23, 2019

MGLMapCamera(lookingAtCenter:, fromDistance:, pitch:, heading:) is not respecting content insets properly. #12728

Closed

4 tasks

frederoni mentioned this pull request May 23, 2019

Remove viewport workaround in CarPlay mapbox/mapbox-navigation-ios#2134

Merged

fabian-guerra approved these changes May 23, 2019

View reviewed changes

platform/ios/CHANGELOG.md Outdated Show resolved Hide resolved

astojilj force-pushed the astojilj-center-offset branch 2 times, most recently from 4e6cd24 to 7a9ce6c Compare May 28, 2019 09:29

astojilj added 2 commits May 28, 2019 14:05

Asymmetric viewport changelog entry

85f6649

Check edge insets difference, in addition to isEqualToMapCamera in all the places in MGLMapView before map.easeTo/map.flyTo.

astojilj force-pushed the astojilj-center-offset branch from f3145c1 to 85f6649 Compare May 28, 2019 11:05

astojilj merged commit e5431d0 into master May 28, 2019

kkaefer deleted the astojilj-center-offset branch May 28, 2019 12:13

1ec5 mentioned this pull request May 28, 2019

Improve layout in landscape orientation mapbox/mapbox-navigation-ios#1139

Closed

1ec5 reviewed May 28, 2019

View reviewed changes

platform/ios/CHANGELOG.md Show resolved Hide resolved

platform/macos/CHANGELOG.md Show resolved Hide resolved

1ec5 mentioned this pull request May 28, 2019

Add public axonometric (isometric, skew) option #11882

Open

1ec5 mentioned this pull request May 31, 2019

Depend on map SDK v5.0.x specifically mapbox/mapbox-navigation-ios#2143

Merged

astojilj added a commit that referenced this pull request Jun 3, 2019

[ios] Content insets border case fix and comment.

1637a4b

Allows changing camera when user duplicates content insets. Related to previous patch and comment: #14664 (review) Thanks @1ec5

astojilj mentioned this pull request Jun 7, 2019

Port asymmetric viewport PR#14664 from gl-native to gl-js mapbox/mapbox-gl-js#8328

Closed

This was referenced Jun 11, 2019

Use contentInset for visibleCoordinateBounds #14135

Closed

[ios] add content inset to camera edge padding #14081

Closed

astojilj mentioned this pull request Jul 10, 2019

[ios] Fix incorrect center coordinate after pinch gesture #15097

Merged

astojilj mentioned this pull request Jul 16, 2019

[core] Fix collision with content insets #15130

Merged

1ec5 mentioned this pull request Jul 18, 2019

Runaway tile downloads, symbol placement when map is tilted with top padding #15163

Open

This was referenced Jul 26, 2019

Content insets and edge padding should be additive, not mutually exclusive #15232

Closed

Edge padding should not be persisted as content insets #15233

Open

astojilj mentioned this pull request Aug 1, 2019

Applying tilting and padding on the location component problem . #15269

Closed

astojilj mentioned this pull request Aug 9, 2019

[core] center still offset incorrectly with content insets #15344

Closed

astojilj mentioned this pull request Aug 12, 2019

Simplify anchor point calculation and puck centering mapbox/mapbox-navigation-ios#2211

Merged

astojilj mentioned this pull request Aug 19, 2019

[android] Setting padding moves camera #15412

Closed

mfazekas mentioned this pull request Mar 12, 2020

Bounds padding not applied when using setCamera() on iOS on version 7.2.0 rnmapbox/maps#715

Closed

This was referenced Apr 29, 2020

Add a factor to limit tilt when offset value is important #16448

Closed

Behavior of Tilt with top padding mapbox/mapbox-gl-native-android#162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Asymmetric viewport when edge insets are specified #14664

[core] Asymmetric viewport when edge insets are specified #14664

astojilj commented May 14, 2019 •

edited

Loading

pozdnyakov left a comment

julianrex commented May 16, 2019

astojilj commented May 17, 2019

1ec5 commented May 17, 2019

fabian-guerra left a comment

astojilj commented May 23, 2019 •

edited

Loading

fabian-guerra left a comment

tobrun commented May 28, 2019

[core] Asymmetric viewport when edge insets are specified #14664

[core] Asymmetric viewport when edge insets are specified #14664

Conversation

astojilj commented May 14, 2019 • edited Loading

pozdnyakov left a comment

Choose a reason for hiding this comment

julianrex commented May 16, 2019

astojilj commented May 17, 2019

1ec5 commented May 17, 2019

fabian-guerra left a comment

Choose a reason for hiding this comment

astojilj commented May 23, 2019 • edited Loading

fabian-guerra left a comment

Choose a reason for hiding this comment

tobrun commented May 28, 2019

astojilj commented May 14, 2019 •

edited

Loading

astojilj commented May 23, 2019 •

edited

Loading