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

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

Closed

Conversation

philemonmerlet
Copy link
Contributor

With an important tilt, when using a top padding to offset the map center, like in GPS apps, currently the view depth becomes very far away, and the apps become very laggy (unusable on Android devices).
PR #15195 limits the depth, but this is far from enough.

As described in mapbox/mapbox-gl-native-android#162 , from the demo sample :

  • Add a button on activity_basic_simpe_mapview.xml
    <Button  
        android:id="@+id/simple_map_change_padding"  
        android:layout_width="wrap_content"  
        android:layout_height="wrap_content"  
        android:text="Change padding" />
  • Modify the SimpleMapActivity. At the end of the onCreate, add
    findViewById(R.id.simple_map_change_padding).setOnClickListener(v -> {  
        mapView.getMapAsync(mapboxMap -> {  
        mapboxMap.setCameraPosition(new CameraPosition.Builder().padding(0, (int) (mapView.getMeasuredHeight() * 0.7), 0, 0).build());
        });  
    });

As a result, the view depth is too far away :

Capture d’écran 2020-04-29 à 19 27 14

By adding a factor of 2.4 (value to be discussed) to the top offset when calculating the maximum tilt for a specified inset, the depth is less far away and the behavior is much more reasonable, allowing to use the app without lag while maintaining a view depth similar as when there is no padding, and also similar as what we had before the padding behavior was changed (post Android 7.4 release).
Capture d’écran 2020-04-29 à 19 34 43

Note that on this particular screen, the resulting max tilt is approximately 45 degrees, which could be set manually using the MapboxMapOptions. However, this means the value would be set even when padding values do not need it (what about landscape mode ?). Also I think the current behavior looks like a bug, and users should not have to dig into mapbox issues to find this workaround.

Regards !

@philemonmerlet philemonmerlet requested a review from a team as a code owner April 29, 2020 17:44
@philemonmerlet
Copy link
Contributor Author

Other related issue : mapbox/mapbox-gl-native-android#282

@@ -644,7 +644,8 @@ double Transform::getMaxPitchForEdgeInsets(const EdgeInsets& insets) const {
// We use half of fov, as it is field of view above perspective center.
// With inset, this angle changes and tangentOfFovAboveCenterAngle = (h/2 + centerOffsetY) / (height * 1.5).
// 1.03 is a bit extra added to prevent parallel ground to viewport clipping plane.
const double tangentOfFovAboveCenterAngle = 1.03 * (height / 2.0 + centerOffsetY) / (1.5 * height);
// 2.4 is an arbitrary factor to prevent too much depth when the top offset is important.
const double tangentOfFovAboveCenterAngle = 1.03 * (height / 2.0 + centerOffsetY * 2.4) / (1.5 * height);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change effectively moves the camera’s focal point closer to where it was prior to #14664. Unfortunately, just modifying tangentOfFovAboveCenterAngle probably causes the pitch and contentInsets (edgePadding) properties to become inaccurate or inconsistent. This would technically be a consideration for #15195 as well, but in that case it’s only an edge case with very large padding, whereas here it would affect the camera no matter the amount of padding.

I agree that the pitch should vary by zoom level (#6908), but ideally it would be limited by the visible distance rather than a screen-based factor, and the pitch should be explicitly limited, affecting the camera model, not just the internal transform.

In the meantime, the common workaround for mapbox/mapbox-gl-native-android#162 is to reduce the pitch to something like 45°. That certainly isn’t optimal, but at least the map is internally consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback ! I understand that there is no easy solution, and this change would involve regressions for other use cases.
A separate, explicit parameterfor offset or for max visible distance sounds like a good idea, I hope it will be available at some time.
Actually the padding is not so "very large", it is easily reached if we want to have the map centered around 1/4 of the screen height.
The problem I have with reducing the pitch to 45° is that it doesn't take real padding size into account, sometimes it is not necessary to reduce the pitch so much. I think I will keep my "fix" in our fork while there is no alternative.

philemonmerlet pushed a commit to Mappy/mapbox-gl-native that referenced this pull request May 4, 2020
@stale stale bot added the archived Archived because of inactivity label Jul 1, 2020
@stale
Copy link

stale bot commented Jul 2, 2020

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Jul 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived Archived because of inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants