maplibre-gl-js icon indicating copy to clipboard operation
maplibre-gl-js copied to clipboard

Fix flicker on map resize

Open daylightwarbler opened this issue 1 year ago • 11 comments

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

  • [x] Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • [x] Briefly describe the changes in this PR.
  • [x] Link to related issues.
  • [x] Include before/after visuals or gifs if this PR includes visual changes.
  • [ ] Post benchmark scores.
  • [x] Add an entry to CHANGELOG.md under the ## main section.

daylightwarbler avatar Jul 18 '24 21:07 daylightwarbler

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.73%. Comparing base (8818fe3) to head (f2ee69c). Report is 545 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 18 '24 22:07 codecov-commenter

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

HarelM avatar Jul 19 '24 04:07 HarelM

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.

daylightwarbler avatar Jul 19 '24 19:07 daylightwarbler

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

HarelM avatar Jul 19 '24 19:07 HarelM

Any updates on this?

HarelM avatar Aug 01 '24 19:08 HarelM

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

birkskyum avatar Aug 05 '24 23:08 birkskyum

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.

HarelM avatar Aug 06 '24 20:08 HarelM

@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.

daylightwarbler avatar Aug 06 '24 21:08 daylightwarbler

Maybe #4535 can help here too?

xabbu42 avatar Aug 10 '24 13:08 xabbu42

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

HarelM avatar Aug 11 '24 09:08 HarelM

Maybe #4535 can help here too?

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

xabbu42 avatar Aug 14 '24 08:08 xabbu42

Closing in favor of:

  • #4632

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

HarelM avatar Sep 02 '24 11:09 HarelM