mapbox-maps-android icon indicating copy to clipboard operation
mapbox-maps-android copied to clipboard

Add check for heading updates for avoid map-idle blocking

Open kifio opened this issue 2 years ago • 7 comments

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:

  1. According to documentation map-idle would send when there are no ongoing transitions and the map has rendered all requested non-volatile tiles.
  2. 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 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.

kifio avatar Jun 27 '22 08:06 kifio

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 27 '22 08:06 CLAassistant

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.

pengdev avatar Jun 27 '22 14:06 pengdev

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.

ghost avatar Jun 27 '22 14:06 ghost

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.

ghost avatar Jun 29 '22 11:06 ghost

@kifio Thanks for trying it out.

Method updateCurrentBearing calls continuously, but checkabs(bearings.last() - lastBearing) < BEARING_UPDATE_THRESHOLD) always returns false, cause value lastBearing 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 than BEARING_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 avatar Jul 01 '22 13:07 pengdev

@pengdev Thank you for clarification reply. Let me investigate my problem with all this new info :) I would answer your question a bit later.

ghost avatar Jul 04 '22 07:07 ghost

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.

ghost avatar Jul 06 '22 13:07 ghost