mapbox-maps-android
mapbox-maps-android copied to clipboard
Add check for heading updates for avoid map-idle blocking
Summary of changes
Preventing notifying locationConsumers
if userHeading
changed on less than 1 degrees
I faced with issue, when event map-idle
was never emitted on some devices. After long investigation, my colleagues and me found some clue:
- According to documentation
map-idle
would sendwhen there are no ongoing transitions and the
maphas rendered all requested non-volatile tiles
. - Issue is missing out on our devices, when heading updates are disabled.
Further research has led us to fact, that the reason is constantly updates of user heading, which comes from LocationCompassEngine.onSensorChanged
.
For fix this problem I suppose to filter userHeading
updates, when they are small enough to be noticeable.
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 theverify-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 tomain
firstly and then port tov10.[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.
Hi @kifio thank you for the contribution and the detailed investigation/reasoning, these are valid points. We have lately merged https://github.com/mapbox/mapbox-maps-android/pull/1398 , which introduced a threshold to trigger the bearing update, the change was introduced inside the location component plugin before we send bearing updates to puck animators, which would work for both default location provider and custom location provider that users create later. Do you think that https://github.com/mapbox/mapbox-maps-android/pull/1398 will fix your issue?
For more context, https://github.com/mapbox/mapbox-maps-android/pull/1398 will be released with v10.7.0-beta.1 later this week, in the mean time, you can try to validate the fix using snapshot following the instructions here and use the latest hash on the main branch 10.6.0-7deaac8ee-SNAPSHOT
.
Hi, @pengdev . Thank you for fast response. Sure, I will check https://github.com/mapbox/mapbox-maps-android/pull/1398 in the near future today or maybe tomorrow morning. I will keep you informed.
Hello, @pengdev .
I tried to use 10.6.0-7deaac8ee-SNAPSHOT
, but still got the issue.
Method updateCurrentBearing
calls continuously, but checkabs(bearings.last() - lastBearing) < BEARING_UPDATE_THRESHOLD)
always returns false
, cause value lastBearing
doesn't change.
Also, differences between values of bearings.last()
often less than BEARING_UPDATE_THRESHOLD
, but this looks like minor problem.
@kifio Thanks for trying it out.
Method
updateCurrentBearing
calls continuously, but checkabs(bearings.last() - lastBearing) < BEARING_UPDATE_THRESHOLD)
always returnsfalse
, cause valuelastBearing
doesn't change.
This is expected behaviour, as the check is done inside the updateCurrentBearing
function, and abs(bearings.last() - lastBearing) < BEARING_UPDATE_THRESHOLD)
always returns false
means that we are returning updateCurrentBearing
function without updating the animator and the puck properties, thus won't interrupt with the map-idle call.
Also, differences between values of
bearings.last()
often less thanBEARING_UPDATE_THRESHOLD
, but this looks like minor problem.
We will only update the bearing if the delta is above the BEARING_UPDATE_THRESHOLD
, do you have any suggestions on the threshold according to your experiments?
And could you please clarify the expected behaviour?
@pengdev Thank you for clarification reply. Let me investigate my problem with all this new info :) I would answer your question a bit later.
Hi, @pengdev.
I checked it one more time:
Condition if (!forceUpdate && (abs(bearings.last() - lastBearing) < BEARING_UPDATE_THRESHOLD))
doesn't help in my case :)
I run application a few times, and each time it was almost identical: forceUpdate
was false
, bearings.last()
was near -60
degrees and lastBearing
was near 280
or 290
degrees.
So the second part of condition was false, because of abs(-340) is not less than BEARING_UPDATE_THRESHOLD and animationManager.animateBearing( *targets, options = options )
always was called in my case.
I suppose, that lastBearing
should be changed a bit often then now, to having actual value for this condition, but i'm not sure. Also, when I try to launch app with debugger, or attach debugger to process, condition continued return false, but I start getting map-idle
event by some reasons. I start think, that issue not related with animator. But fix from my pull-request still working for me.
UPD: I tried 10.6.0-7deaac8ee-SNAPSHOT and 10.7.0-beta.1. Result the same.