Fix view annotations triggering excessive onMeasure [MAPSAND-516] #1744
Conversation
| currentViewsDrawnMap.keys.forEach { id -> | ||
| if (positionDescriptorCoreList.indexOfFirst { it.identifier == id } == -1) { | ||
| currentlyDrawnViewIdSet.forEach { id -> | ||
| if (positionDescriptorUpdatedList.indexOfFirst { it.identifier == id } == -1) { |
There was a problem hiding this comment.
if I understand correctly in worst case it can be O(N)?
maybe save somehow id + position?
| // pos : [identifier: 42, width: 282, height: 196, leftTopCoordinate: [x: 99.99999999998583, y: 368.9999999999887]] | ||
| // pos : [identifier: 44, width: 282, height: 196, leftTopCoordinate: [x: 148.00000000000566, y: 616.9999999999659]] | ||
| // pos : [identifier: 45, width: 282, height: 196, leftTopCoordinate: [x: 483.0000000000149, y: 566.9999999999759]] | ||
| val wrapContentWithAllowOverlapFalseViewAdded = positionDescriptorUpdatedList.size == 1 && |
There was a problem hiding this comment.
?
| val wrapContentWithAllowOverlapFalseViewAdded = positionDescriptorUpdatedList.size == 1 && | |
| val wrapContentWithAllowOverlapFalseViewAdded = positionDescriptorUpdatedList.size >= 1 && |
There was a problem hiding this comment.
Don't think we can do that, workaround specifically handles case when there's single view. I'll also create a ticket to gl-native to fix the root issue
Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>
yunikkk
changed the title
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ViewGroup.LayoutParams.MATCH_PARENT, | ||
| ) | ||
| // place the view annotations above the map (index 0) but below the compass, ruler and other plugin views |
There was a problem hiding this comment.
Is it expected that view annotation should be placed below the compass/scalebar? Do we want to expose some configurations to adjust the z-order of the view annotations?
There was a problem hiding this comment.
that was not possible before that and in order to control Z index we have the concept of selected state for view annotatotions so it seems that we're totally OK here.
There was a problem hiding this comment.
@peng this kind of customisations makes sense to me, actually, though outside the scope of this PR I guess
There was a problem hiding this comment.
Dropped a ticket https://mapbox.atlassian.net/browse/MAPSAND-535
| internal fun FrameLayout.hasChildView(view: View) = children.any { it == view } | ||
|
|
||
| internal fun MapView.getChildViewIndex(view: View) = children.indexOf(view) | ||
| internal fun FrameLayout.getChildViewIndex(view: View) = children.indexOf(view) |
There was a problem hiding this comment.
As discussed in the last kotlin group session, it's generally a bad practice to extend Android system APIs, but I guess it's less of a problem for internal methods, and in tests.
There was a problem hiding this comment.
Yes, not a problem here - since it's test utils, but a good example of something that could be dangerous in the sdk code indeed!
Added simple run of view annotation activity before/after change, onMeasure was 8.3 percent of all the time doFrame spent on the main thread. |
* Fix view annotations triggering onMeasure by doing bringToFront() on every update. * Implemented algorithm to detect if bringToFront is needed instead of using DiffUtil. * ktlint. * Bring back view hiding until its positioned. * Changelog. * Update CHANGELOG.md Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com> * Fixed tests and review comments. * Fix ui test. * ktlint fix. * Add 10.9.0 section to changelog. * Fix layout name. * Fix incorrectly compared descriptors. Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>
* update stylegen to main * update-api * expose lights3d public * update apis and tests * publish_android_snapshot * publish_android_snapshot * rebase develop and update api --------- Co-authored-by: Tobrun Van Nuland <tobrun.van.nuland@gmail.com>
Problem was caused calling bringToFront() on every update of the annotations, that happens on any map animation or gesture.
bringToFront() was needed to guarantee that :
onViewAnnotationPositionsUpdate(@NonNull List<ViewAnnotationPositionDescriptor> positions);callback). This order changes if some ViewAnnotation is being selected, for exampleAlso I've added separate FrameLayout to the MapView on top of the actual surface/texture view but below any other plugin - so that removing or adding new view annotation does not interfere with the plugins at all.
MapView adds a SurfaceView/TextureView that actually displays map at the index 0 here, then additional views are added on top of it, thus before the fix the layout was looking the following way :
After the fix it becomes :
Summary of changes
User impact (optional)
Before changes, simple map layout with 1 view annotation, browsing map (so onMeasure is called every frame):
onMeasure calls take
117.767nanos out of1.409.936that choreographer.doFrame spent, roughly 8.3%.After changes, same layout and map :
Tested on the Android emulator.
No onMeasure calls at all.
The difference will be more noticeable the more complex layout is used.
Pull request checklist:
@JvmOverloads,@file:JvmName, etc).make update-apito update generated api files, if there's public API changes, otherwise theverify-api-*CI steps might fail.check changelogCI step will fail.v10.[version]release branch fix / enhancement, merge it tomainfirstly 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.