Add back synced frame method and use it for triggerRepaint.

[xabbu42]

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.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.95%. Comparing base (0cef572) to head (32bbbc7).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ui/map.ts	93.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4535   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files         282      282           
  Lines       38908    38917    +9     
  Branches     6826     6829    +3     
=======================================
+ Hits        35779    35788    +9     
  Misses       3002     3002           
  Partials      127      127           

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

[HarelM]

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]

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]

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?

[HarelM]

@xabbu42 what's the status of this PR?
Are you planning on marking it as ready for review or close it?

[xabbu42]

I still plan to finish this. There are uncaught exceptions during the unit tests which I still have to look into.

Sidenote: I was lead quite astray because the exceptions from the unit tests also aborted the render tests when running npm run test so all I see in the console are a lot of render test failures.

[HarelM]

npm run test is running all the tests in the repo, I always run just a single part of the test using: npm run test-unit, npm run test-build, npm run test-render etc.
Also the CI runs them separately so you can see which is failing and why.

[xabbu42]

I think the problem of the failing unit tests is more with the tests themself than a bug with this change.

This as far as I got analyzing them:

• there are two problematic tests 'recalculate zoom is done on the camera update transform' in map_zoom.test.ts and 'getZoom on moveend is the same as after the map end moving, with terrain on' in map_events.test.ts.
• they are unique as they both include map.terrain = createTerrain(); in the setup and then later trigger a map paint.
• with terrain set, Painter.maybeDrawDepthAndCoords calls Painter.useProgram without forceSimpleProjection to draw something
• Painter.useProgram without forceSimpleProjection expects this.style.projection to be set, but with the used empty style it isnt which triggers an exception

The map state produced by this tests can not happen with normal usage, as setTerrain would not work with the empty style used for tests.

Any pointers how this tests should be fixed?

[HarelM]

Looks like something is failing when the test harness is completing or something.
Try and use test.only to find the offeding test.
The test are failing because style.projection is not defined, so it should be mocked as part of the test.
I hope this helps, if not feel free to reach out in slack and I'll see how else I can help out.

[xabbu42]

I found a fix for the failing tests. Problem was that the style was not yet loaded and with these changes the tests triggered a render immediately instead of just scheduling it.

Last remaining question for me is if it would be worth it to try to eliminate the other uses of frameAsync. I don't have a strong opinion. My only 2c are that it may be bad to offer two ways of doing the same thing without clear guidance on when to use which. What do you think @HarelM?

[HarelM]

I generally tried to avoid the callback hell and this was one place that contributed to the complexity of reading the code.
I understand that this causes an issue using spectator.js, is it possible to fix the issue there as I don't think there's an actual problem with the code here, is there?
Bottom line, I like the fact that the code is using promises and I would be glad to be able to continue using promises.
Do you think it's achievable?

[xabbu42]

I don't see a way how this could be fixed generically on the spector.js side. They need some way to know when a single frame starts and ends.

But also, from reading https://developer.mozilla.org/en-US/docs/Web/API/Window/requestAnimationFrame, it seems wrong to me to just schedule the actual repaint in the callback and not execute it directly. Isn't the main advantage of using requestAnimationFrame to tightly couple the map paints with the screen refresh rate and don't we loose that coupling again by just scheduling a repaint and then immediately returning?

As far as I know there is no way to force synchronized execution of a promise. So there is no way keep the aesthetics of a promise based interface but executing the code synchronized under the hood.

So I currently don't think that your request is achievable. And there might be actual problems with the code as is although very hard to notice ones.

[HarelM]

I would consider removing other frameAsync calls as part of different PRs so that we can discuss case by case.
Other than that, this looks good, thanks!

[HarelM]

Can you please add a changelog item and I'll merge this.
Thanks.

[xabbu42]

Thanks!