Skip to content
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

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Conversation

CraigglesO
Copy link
Contributor

Fixes #3106 and most likely mapbox/mapbox-gl-js#9724

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to 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:
Screenshot 2023-11-10 at 1 54 36 PM

AFTER:
Screenshot 2023-11-10 at 1 52 41 PM

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:

Screenshot 2023-11-10 at 1 57 35 PM

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:
Screenshot 2023-11-10 at 1 57 46 PM

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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (32cd9fe) 75.46% compared to head (b19a271) 75.46%.

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.
📢 Have feedback on the report? Share it here.

@CraigglesO
Copy link
Contributor Author

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.

@HarelM
Copy link
Collaborator

HarelM commented Nov 18, 2023

Forget to ask for a changelog entry before I merge.

@CraigglesO
Copy link
Contributor Author

Sure. I'll add as soon as I get the bounty so you know you can merge.

@HarelM
Copy link
Collaborator

HarelM commented Nov 18, 2023

Ok, ping me.
I want to start working on a pre release of version 4, so if this needs to be included in version 3.x this needs to be merged soon, as main will shortly be 4.x.

@HarelM HarelM merged commit 684b314 into maplibre:main Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fill-pattern does not display when changing patterns using "step", "zoom"
3 participants