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

Fix flicker on map resize #4429

Closed
wants to merge 9 commits into from

Conversation

daylightwarbler
Copy link

@daylightwarbler daylightwarbler commented Jul 18, 2024

Fixes #2971 and #4158. Flicker on resize was introduced in #2157, released in v3.0.0, when map resizing was switched to using resizeObserver from using window resize events.

The fix is to immediately render the map upon resizing. The lead was this thread.

Caveat: I'm not sure what to pass for the paintStartTimeStamp for map._render(). I set it to Date.now(). 0 also works fine.

Before and after gifs (half speed):

before
after

  • 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.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.73%. Comparing base (8818fe3) to head (f2ee69c).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4429      +/-   ##
==========================================
- Coverage   87.88%   87.73%   -0.16%     
==========================================
  Files         246      246              
  Lines       33450    33451       +1     
  Branches     2208     2217       +9     
==========================================
- Hits        29399    29347      -52     
- Misses       3050     3101      +51     
- Partials     1001     1003       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Jul 19, 2024

Looks good, can you fix the failing tests, and add another test if needed please?

@daylightwarbler
Copy link
Author

Not sure how to debug the failing test yet. I can confirm that jest --coverage --selectProjects unit returns a nonzero exit code when I run it with the changes (echo $? prints 1), but it doesn't print any error output on my machine. The CI job produced a type error in painter.ts, but I don't get any errors for npm run lint.

@HarelM
Copy link
Collaborator

HarelM commented Jul 19, 2024

npm run test-unit --reportes=default
See more options here:
https://github.com/maplibre/maplibre-gl-js/tree/main/test#running-tests

@HarelM
Copy link
Collaborator

HarelM commented Aug 1, 2024

Any updates on this?

@birkskyum
Copy link
Member

It seems like merging main fixed the test... Trying again to make sure it's not a fluke.

@HarelM
Copy link
Collaborator

HarelM commented Aug 6, 2024

I'm not sure I understand how this solves the problem.
When calling _update, which is done in the resize event, the _update method should add a requestAnimtionFrame with a call to _render.
So, I'm not sure I understand how forcing a call to render solves this issue...

Also, as stated before, please add a test for this bug.

@daylightwarbler
Copy link
Author

@HarelM I think it’s a timing problem. _update() calls triggerRepaint(), which calls _render(), but in testing this I noticed that since the _render() call is in this browser.frameAsync callback, it happens after about a ~10ms delay, compared to if it’s called directly in the resize handler, like in this commit. When I add an explicit 10ms delay before the new _render() call in the resize handler, the flicker comes back. Perhaps this points to a simpler solution.

@xabbu42
Copy link
Contributor

xabbu42 commented Aug 10, 2024

Maybe #4535 can help here too?

@HarelM
Copy link
Collaborator

HarelM commented Aug 11, 2024

This needs a test nevertheless, but if #4535 solves this, it will be a better solution from my point of view.

@xabbu42
Copy link
Contributor

xabbu42 commented Aug 14, 2024

Maybe #4535 can help here too?

Unfortunately thats not the case. I can reproduce the flicker in that branch.

@HarelM
Copy link
Collaborator

HarelM commented Sep 2, 2024

Closing in favor of:

If the linked PR is not enough, please comment on it or comment in the linked closed issue.

@HarelM HarelM closed this Sep 2, 2024
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.

White flickering on resize in version 3.x.x
5 participants