-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix raster dem crash style reload #1717
Conversation
8c56a31
to
a4da67f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add
- PR description
- bug label
- fix ktlint
- add changelog
Sure, thanks for hint, pushed it quickly to run the tests |
a4da67f
to
418dd48
Compare
terrainSource: String = "TERRAIN_SOURCE", | ||
demSourceUri: String = "mapbox://mapbox.mapbox-terrain-dem-v1" | ||
) { | ||
mapboxMap.loadStyle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
maybe add extra delay? potentially we can have super fast internet and super fast device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well worst case if it happens without any delay - then the crashes actually had happenned. Could you elaborate more why do you think delay is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate more why do you think delay is needed?
to guarantee a delay between different styles load and it can be reproducible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't need a delay, that's the thing, for this specific test - it happens when there is no delay so that style load events could collide with higher probability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean some delay in style loaded callbacks. To make listeners crossover effect more noticeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but they all are executed on the main thread - if I delay it they all will be delayed as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I can add bunch of randomly delayed calls, all right, no problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 100 launches with random delay of 0-5 millis
d69ac6f
to
35ea25e
Compare
Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>
Would be nice to add a test for a following scenario (pseudo code): map.loadStyle(URL) { DSL runtime modifications }
map.onStyleDataLoaded({
map.loadStyleJson(InvalidStyleJSON)
})
// verify that original style (URL) is loaded and runtime modifications are applied |
Such flow will not be handled correctly at the moment, turns out. |
terrainSource: String = "TERRAIN_SOURCE", | ||
demSourceUri: String = "mapbox://mapbox.mapbox-terrain-dem-v1" | ||
) { | ||
mapboxMap.loadStyle( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean some delay in style loaded callbacks. To make listeners crossover effect more noticeable.
* Fix possible crash when terrain source is added multiple times using runtime Style DSL. * Update changelog. * ktlint. * Fix tests. * PR fixes. * Fix integration test. * Update CHANGELOG.md Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com> * Review fixes. * Add test with delays. * Add test with delays. Co-authored-by: Kiryl Dzehtsiarenka <kiryl.dzehtsiarenka@mapbox.com>
Summary of changes
When we load styles many times in a row there's a possibility that
style data : sources
event from the previous style will arrive after new style has been started loading. Here we add the fix for it and actually wait for thestyle data : style
event after the style load has been initiated and skip all the other events.This is actually part of MAPSNAT-516 that couldn't be fixed in gl-native.
User impact (optional)
Pull request checklist:
@JvmOverloads
,@file:JvmName
, etc).make update-api
to update generated api files, if there's public API changes, otherwise theverify-api-*
CI steps might fail.check changelog
CI step will fail.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.