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

[WIP] Configurable degrees of freedom for annotation views #5245

Closed
wants to merge 5 commits into from

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jun 6, 2016

Replaced MGLAnnotationView’s flat property with a freeAxes property that allows 0–2 degrees of freedom (pitch and/or rotation) for various “billboard” effects. (Fixes #2528.) Currently, y-free annotation views are rendered incorrectly when rotated and tilted. Other than that issue and #5090, most of the transforms are working:

Rotated Tilted Neither free x free y free Both free
✖️ ✖️ none unrotated untilted x unrotated untilted y unrotated untilted xy unrotated untilted
✔️ ✖️ none rotated untilted x rotated untilted y rotated untilted xy rotated untilted
✖️ ✔️ none unrotated tilted x unrotated tilted y unrotated tilted xy unrotated tilted
✔️ ✔️ none rotated tilted x rotated tilted y rotated tilted xy rotated tilted

This PR also applies perspective to x-free annotation views. (#5090) The effect is equivalent to #5218. The fix will require exposing the coordinate-to-point transformation matrix in mbgl::TransformState so that the SDK can convert it into a CATransform3D.

At times, I’m seeing an orange screen of death when there are a thousand annotations and the views start getting recycled:

orange

To do:

  • Plumb projection matrix through to MGLAnnotationView
  • Fix orange screen bug seen in iosapp with 1,000 point annotations
  • Fix y-free annotation views when tilted and rotated

This PR previously contained the following changes that have been split out:

/cc @boundsj @friedbunny

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS annotations Annotations on iOS and macOS or markers on Android labels Jun 6, 2016
@1ec5 1ec5 added this to the ios-v3.3.0 milestone Jun 6, 2016
@1ec5 1ec5 self-assigned this Jun 6, 2016
@1ec5
Copy link
Contributor Author

1ec5 commented Jun 6, 2016

Based on this guide, I attempted to also address the iOS side of #5218 by translating to the view center before rotating, but that caused the annotations to drift with respect to the map when panning from side to side.

translated

At this point, I’m eyeing a solution in which mbgl::Map exposes TransformState’s projection matrix or coordinatePointMatrix to the SDK. It should be possible to convert from that matrix to a CATransform3D that can be applied to the annotation views’ layers, although some additional involved steps may be required.

If we can’t get the views to respect the map’s projection matrix, I think we should remove the ability for annotation views to tilt, rotate, and scale and revisit for the next release.

/cc @jfirebaugh @tobrun

@1ec5 1ec5 added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 6, 2016
@tobrun
Copy link
Member

tobrun commented Jun 6, 2016

@1ec5 That looks like a great approach to me. While I feel the same when it comes to holding back on tilt. I don't see an issue with rotate or scale. These are currently implemented without side effects and can be ported to use the solution mentioned above later on. Side note for this is that a lift is needed to start implementing the CATransform3D alternative on Android for View Annotations (Camera graphics object from the Android SDK as we already use on MyLocationView).

update now I'm thinking this could also be related to impact of scale of view annotations when zooming out in #5169 (which is not implemented on android yet) so my assumptions above could be wrong.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 6, 2016

now I'm thinking this could also be related to impact of scale of view annotations when zooming out in #5169 (which is not implemented on android yet)

Correct. Scaling based on viewing distance is implemented in the iOS SDK, but the effect is suboptimal. For one thing, all annotations appear to shrink as the pitch increases, because they're all sized based on the annotations farthest back rather than at the center of the view (where there is supposed to be no distortion). Moreover, I don't feel good about relying on magic values throughout these transform calculations. Without basing the transforms on mbgl's matrix, it'll be difficult to make a view-backed annotation behave like a GL annotation when it comes to scaling.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 7, 2016

I split out and merged the less risky parts of this PR as #5262, #5263, #5264. The remaining changes either need further work to produce the desired effect or need us to better understand the performance implications.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 8, 2016

The naming and design of the freeAxes property and MGLAnnotationViewBillboardAxis enum was based on SceneKit’s SCNBillboardConstraint.freeAxes and SCNBillboardAxis. I thought it was an elegant approach at first, but it has been difficult to keep the different axes straight: which axis affects whether the view tilts with the map? Does it tilt when MGLAnnotationViewBillboardAxisX is on or off? It’s telling that the SCNBillboardAxis documentation resorts to a diagram featuring a potted plant for clarity. On top of everything else, MGLMapView doesn’t support banking the camera, so MGLAnnotationViewBillboardAxisZ ends up being meaningless.

So I’m thinking of restoring the original API with more descriptive names, like tiltsToMatchCamera and rotatesToMatchCamera (with UITextField.adjustsFontSizeToFitWidth as precedent for such a long name).

@1ec5
Copy link
Contributor Author

1ec5 commented Mar 16, 2017

At this point, I’m eyeing a solution in which mbgl::Map exposes TransformState’s projection matrix or coordinatePointMatrix to the SDK. It should be possible to convert from that matrix to a CATransform3D that can be applied to the annotation views’ layers, although some additional involved steps may be required.

The SceneKit integration described in #3668 (comment) would benefit from access to the map's transform matrix as well. For that particular example (which is awesome!), an orthographic projection may be desirable, but something more realistic might require a perspective transform.

/cc @suzukieng

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 21, 2017

peterqliu/threebox#12 looks like it addresses the same use case as this PR. In particular, it synthesizes a projection matrix that matches the underlying map. If this matrix manages to account for horizontal translation on a tilted plane, that would be a good candidate to be ported here.

/cc @kronick

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 23, 2017

Transforming the annotation container view instead of individual annotation views ensures that each annotation view appears to match the underlying map’s transform. However, this approach redundantly applies the map’s perspective matrix to the annotations’ positions, causing them to appear at the wrong locations. Additionally, it prevents individual annotation views from opting out of lying flat against the map.

flattened hydrants

Along the lines of #5245 (comment), a more correct approach would be to apply the perspective rotation transform to each individual annotation view, but the anchor point (vanishing point) would need to be moved to the center of the container view (without moving the layer) to avoid this effect:

uncentered hydrants

@1ec5 1ec5 force-pushed the 1ec5-annotation-cleanup branch 2 times, most recently from 2e37836 to 5f135aa Compare June 29, 2017 04:55
1ec5 added 5 commits July 5, 2017 18:32
Replaced MGLAnnotationView’s flat property with a freeAxes property that allows 0–2 degrees of freedom (pitch and/or rotation).

Reformatted and copyedited MGLAnnotationView documentation. Removed the unnecessary custom getter on scalesWithViewingDistance.

Fixes #2528.
The transform matrix is correct, but applying it to the annotation container view incorrectly translates each annotation view, and applying it to each annotation view individually causes an incorrect anchor point to be used.
@jfirebaugh
Copy link
Contributor

Stale.

@jfirebaugh jfirebaugh closed this Oct 24, 2017
@jfirebaugh jfirebaugh deleted the 1ec5-annotation-cleanup branch October 24, 2017 20:40
@1ec5 1ec5 restored the 1ec5-annotation-cleanup branch October 26, 2017 16:31
@1ec5
Copy link
Contributor Author

1ec5 commented Nov 18, 2017

Tracking a revival of this PR in #10498.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold Google Maps parity For feature parity with the Google Maps SDK for Android or iOS iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align annotation rotation and pitch with map allow for annotation selection z-order change
3 participants