-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
Bug Fix: updatePatternPositions fix #3339
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
=======================================
Coverage 75.46% 75.46%
=======================================
Files 241 241
Lines 19271 19273 +2
Branches 4342 4344 +2
=======================================
+ Hits 14543 14545 +2
Misses 4728 4728 ☔ View full report in Codecov by Sentry. |
Hey, I think I addressed all your issues. I rarely use Jest, so I'm assuming this is correct? Let me know if I missed anything. |
Forget to ask for a changelog entry before I merge. |
Sure. I'll add as soon as I get the bounty so you know you can merge. |
Ok, ping me. |
Fixes #3106 and most likely mapbox/mapbox-gl-js#9724
Launch Checklist
CHANGELOG.md
under the## main
section.Case Definition
Take the example from this gist:
https://gist.github.com/CraigglesO/2c9a3f741c6f3c7d276dde282aa82e6e
Zoom from starting zoom 2 to zoom 4

BEFORE:
AFTER:

The Problem
Assume we just zoomed to zoom 4:
When data is shipped over and it's time to render, the fill will check the current pattern property. Here is the console log:
This is WRONG. At zoom 4, it should be volcano_11 for both cases.
Meawhile the reason this is actually an issue is that the WorkerTile parsing the layer information at the bucket population checks for patterns and when it finally runs addPatternDependencies we see this:

The reason is because the WorkerTile constantly updates the style/layer states because it's always running maybePrepare before studyng the layers to ship back to the main thread.
The easiest fix (albeit a bandaid to a larger problem) is to edit updatePatternPositionsInProgram to render SOMETHING (basically just avoid complete failure). Since image dimensions are shared on demand for each tile, the WorkerTile never shared anything about zoo, so when it can't find any zoo data it completely fails. Whereas now, the function will find a way to succeed even if it can't find the data.
Perhaps other sections of the codebase actually correct for this behavior but the BEST solution is to find a way to force incremental updates from the styler more frequently. This would fix any other layer's having similar properties from not updating properly as well, but after a decent effort into digging I couldn't figure out the best way to update layers more frequently without potentially breaking some things.