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

Add back synced frame method and use it for triggerRepaint. #4535

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xabbu42
Copy link
Contributor

@xabbu42 xabbu42 commented Aug 10, 2024

Launch Checklist

An experimental fix for #4534. With this I can again a capture a single frame with spector.js, as all the rendering is directly done inside the requestAnimationFrame callback.

Unfortunatly tests dont run through but fail after some random point. Probably a race I introduced?

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

@xabbu42 xabbu42 mentioned this pull request Aug 10, 2024
6 tasks
@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.29%. Comparing base (ba721b4) to head (ca568bf).
Report is 7 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (ba721b4) and HEAD (ca568bf). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (ba721b4) HEAD (ca568bf)
9 8
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4535       +/-   ##
===========================================
- Coverage   87.79%   65.29%   -22.50%     
===========================================
  Files         246      246               
  Lines       33450    33457        +7     
  Branches     2219     1455      -764     
===========================================
- Hits        29367    21847     -7520     
- Misses       3081    10765     +7684     
+ Partials     1002      845      -157     

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

@HarelM
Copy link
Collaborator

HarelM commented Aug 11, 2024

requestAnimationFrame is not a method that runs right away as far as I know, it might have higher priority than a promise, but I'm not sure I saw it anywhere, so I'm not sure I know how this fix will actually solve anything, but let me know if it does.
I think the code introduced here is fine in terms of how it looks.
A test is needed to cover the relevant issue (spector.js if I understood correctly).

In case this is not ready for review, please change it to a draft and ping me when you change it back to regular PR.

THANKS!

@xabbu42
Copy link
Contributor Author

xabbu42 commented Aug 13, 2024

this experiment is not about the time between the call to requestAnimationFrame and when the passed callback is called, but about the time between the call to the passed callback and the actual map._render call. Currently the passed callback only schedules the _render and then returns whereas before the callback directly called _render.

Come to think of it, it should be possible to keep with the promise based interface for frameAsync for the time between the call to frameAsync and when the callback of requestAnimationFrame actually gets triggered while at the same time calling _render directly in the callback. I'll try to implement that approach.

@HarelM
Copy link
Collaborator

HarelM commented Aug 13, 2024

Can we pass down the callback to the request animation frame and then call the resolve at the end? Would that work? Or is this the scratched out part in your comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants