[viewannotation] optimize cameraForAnnotations api MAPSAND-577 #1861

ank27 · 2022-11-23T12:49:12Z

Summary of changes

Currently ViewAnnotationManager.cameraForCoordinate doesn't work as expected when multiple viewannotations have different width and height.

This pr improves the api implementation by calculating correct bounds taken by ViewAnnotationOptions's width and height using different Projections api.

Before	After
before.mov	after.mov

User impact (optional)

Pull request checklist:

Briefly describe the changes in this PR.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality. If tests were not written, please explain why.
Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
Add example if relevant.
Document any changes to public APIs.
Run make update-api to update generated api files, if there's public API changes, otherwise the verify-api-* CI steps might fail.
Update CHANGELOG.md or use the label 'skip changelog', otherwise check changelog CI step will fail.
If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

alexander-kulikovskii · 2022-11-24T11:48:00Z

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

+    var west: Pair<ViewAnnotationOptions, Rect?>? = null
+    var south: Pair<ViewAnnotationOptions, Rect?>? = null
+
+    while (!isCorrectBound) {


how many iteration can be here in worst case? can it be forever loop?

Can't be a forever loop, the worst case iteration is 2 loops.
Initially, we are getting innerBounds cameraOption before while loop, then the next iteration is checking if it needs extra paddings. All other nullability checks have already been tested.

We discovered a case where this can loop forever on iOS - with certain combination of camera state/map size/view annotations there might be one pair(view annotation options & rect) to be e.g. southern-most for a particular zoom level, and for another zoom level the different pair can become southern-most.

Potentially this code is also susceptible to the same scenario, worthwhile to double check. @maios is leading the investigation of this on iOS.

I'd probably removed the loop, extracted the code calculating cameraOptionForCoordinates and explicitly called it twice to avoid any risks.

Yeah, make sense. although I couldn't reproduce the issue on android with the same setup. I will try to loop it with a counter and that would be our maximum tries to adjust the bounds.
I can see that this might not provide correct bounds for all cases, but we have discussed this with gl-native to reiterate over framing implementation.

added ticket to track this issue.

@ank27 at the same time why do we need a loop for two iterations? What about extracting to function and calling twice? Code will be more clear and not additional variables involved

In case we need to have more iteration in the future to have better bounds. but that's debatable.
btw why can't we use loop though?

@ank27 I was thinking that the loop is not needed if we know that we have fixed and limited number (== 2) of iterations to calculate the bounds. Now it seems that it's not - and I'm actually ok with having the loop with counter which can be controlled. The question is do we want it to be 2 or some larger value to be almost certain that the bounds will be correctly calculated.

2 iteration i found good for the cases i have tested, but i can't be sure of all edge-cases (for eg. pitched map and with bearing). so i am not 100% sure about fixed number.

The question is do we want it to be 2 or some larger value to be almost certain that the bounds will be correctly calculated.

cut a ticket to look more into finding correct calculations for all cases.

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

CHANGELOG.md

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

yunikkk · 2022-12-02T12:33:58Z

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

+      boundsCounter++
+      isCorrectBound = true
+      viewAnnotationOptions.forEach { options ->
+        val frame = getViewAnnotationOptionsFrame(options) ?: Rect(0, 0, 0, 0)


nit : make getViewAnnotationOptionsFrame return nun-nullable value? Options are non-null

getViewAnnotationOptionsFrame is a separate internal function, that can be used in some other calculations in future. making it non-nullable will force us to return Rect(0,0,0,0) in case of no viewannotation present, which might not be best in some scenario.

yunikkk · 2022-12-02T12:34:39Z

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt

+    // we run the loop twice to adjust bounds correctly to fit all the annotations.
+    var boundsCounter = 1
+    while (!isCorrectBound && boundsCounter <= MAX_ADJUST_BOUNDS_COUNTER) {
+      val zoom = cameraOptionForCoordinates.zoom


nit : declare val out of loop above

ohh wait, it's used inside while loop only, so I will keep it here.

kiryldz · 2022-12-02T12:56:46Z

@ank27 can you please rebase it? seems to be pretty outdated

sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerAddTest.kt

kiryldz

LGTM with several nits

* optimize cameraforannotations api * update cameraforannotation implementation * update apis to adjust paddings * update cameraforannotation implementation * address review comments * limit bounds calculation loop * address review comments * update changelog

#1889) * optimize cameraforannotations api * update cameraforannotation implementation * update apis to adjust paddings * update cameraforannotation implementation * address review comments * limit bounds calculation loop * address review comments * update changelog

* use unix time for unique version code * publish_testapp_playstore

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch 2 times, most recently from b4eda9b to 66b343f Compare November 24, 2022 10:34

ank27 marked this pull request as ready for review November 24, 2022 10:34

ank27 requested a review from a team as a code owner November 24, 2022 10:34

ank27 self-assigned this Nov 24, 2022

ank27 added feature 🍏 bug 🪲 Something isn't working and removed feature 🍏 labels Nov 24, 2022

ank27 changed the title ~~[viewannotation] optimize cameraForAnnotations api~~ [viewannotation] optimize cameraForAnnotations api MAPSAND-577 Nov 24, 2022

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch from 84fe297 to b9e01a4 Compare November 24, 2022 10:55

kiryldz reviewed Nov 24, 2022

View reviewed changes

sdk/src/test/java/com/mapbox/maps/shadows/ShadowCoordinateBounds.java Outdated Show resolved Hide resolved

kiryldz reviewed Nov 24, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

alexander-kulikovskii reviewed Nov 24, 2022

View reviewed changes

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch from fb369ad to 91cc451 Compare November 25, 2022 11:14

yunikkk reviewed Nov 25, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

yunikkk reviewed Nov 25, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

yunikkk reviewed Nov 25, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

yunikkk reviewed Nov 25, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch 2 times, most recently from 8a09463 to f16074a Compare November 28, 2022 10:11

ank27 requested review from yunikkk, alexander-kulikovskii and kiryldz November 30, 2022 12:12

kiryldz reviewed Dec 1, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

yunikkk reviewed Dec 1, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Show resolved Hide resolved

ank27 requested review from kiryldz, yunikkk and evil159 December 2, 2022 11:14

yunikkk reviewed Dec 2, 2022

View reviewed changes

sdk/src/main/java/com/mapbox/maps/ViewAnnotationManagerImpl.kt Outdated Show resolved Hide resolved

yunikkk reviewed Dec 2, 2022

View reviewed changes

yunikkk approved these changes Dec 2, 2022

View reviewed changes

kiryldz reviewed Dec 2, 2022

View reviewed changes

sdk/src/test/java/com/mapbox/maps/ViewAnnotationManagerAddTest.kt Outdated Show resolved Hide resolved

kiryldz approved these changes Dec 2, 2022

View reviewed changes

ank27 added 7 commits December 2, 2022 15:20

optimize cameraforannotations api

ab045e1

update cameraforannotation implementation

95e229e

update apis to adjust paddings

eea3a41

update cameraforannotation implementation

ffae0a0

address review comments

777d690

limit bounds calculation loop

ed0d69f

address review comments

eea6a1d

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch from 2ad7244 to eea6a1d Compare December 2, 2022 13:40

update changelog

c3d5a6b

ank27 force-pushed the ak-fix-cameraforAnnotation-api branch from 65aba12 to c3d5a6b Compare December 2, 2022 13:42

ank27 merged commit 3443151 into main Dec 2, 2022

ank27 deleted the ak-fix-cameraforAnnotation-api branch December 2, 2022 14:45

mapbox-github-ci-writer-public-1 bot pushed a commit that referenced this pull request Jul 27, 2023

fix playstore versioncode (#1861)

a36f6ce

* use unix time for unique version code * publish_testapp_playstore

kiryldz pushed a commit that referenced this pull request Aug 2, 2023

fix playstore versioncode (#1861) (#1862)

1fbd22e

* use unix time for unique version code * publish_testapp_playstore

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[viewannotation] optimize cameraForAnnotations api MAPSAND-577 #1861

[viewannotation] optimize cameraForAnnotations api MAPSAND-577 #1861

ank27 commented Nov 23, 2022 •

edited

Loading

alexander-kulikovskii Nov 24, 2022

ank27 Nov 25, 2022

evil159 Nov 30, 2022

yunikkk Nov 30, 2022

ank27 Nov 30, 2022 •

edited

Loading

ank27 Dec 1, 2022

yunikkk Dec 1, 2022

ank27 Dec 1, 2022

yunikkk Dec 2, 2022

ank27 Dec 2, 2022

yunikkk Dec 2, 2022

ank27 Dec 2, 2022

yunikkk Dec 2, 2022

ank27 Dec 2, 2022

kiryldz commented Dec 2, 2022

kiryldz left a comment

[viewannotation] optimize cameraForAnnotations api MAPSAND-577 #1861

[viewannotation] optimize cameraForAnnotations api MAPSAND-577 #1861

Conversation

ank27 commented Nov 23, 2022 • edited Loading

Summary of changes

User impact (optional)

Pull request checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ank27 Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiryldz commented Dec 2, 2022

kiryldz left a comment

Choose a reason for hiding this comment

ank27 commented Nov 23, 2022 •

edited

Loading

ank27 Nov 30, 2022 •

edited

Loading