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

Refactor render_system into multiple systems #11233

Closed
wants to merge 2 commits into from

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Jan 6, 2024

Objective

Framepacing requires access to frame presentation times. Framerate smoothing requires the ability to insert systems around frame presentation.

Solution

Refactor render_system into multiple systems so the new present_system can be isolated.

The old render_system was renamed to render_graph_system to encourage intentional migration.


Changelog

Changed

  • The system render_system was replaced with a set of systems in bevy_render::renderer: render_graph_system, present_system, finalize_render_system, send_time_system.

Migration Guide

Systems that were scheduled .after(bevy_render::renderer::render_system) should now be scheduled .after(bevy_render::renderer::send_time_system) to preserve the same behavior. Systems scheduled .before(bevy_render::renderer::render_system) can be scheduled .before(bevy_render::renderer::render_graph_system) .


Alternatives

This API change is fragile, since any changes to system ordering may invalidate a framepace implementation that manually schedules its systems around present_system. One alternative would be higher-granularity system sets, or even an extra system set for just present_system.


Useful resources

Copy link
Contributor

github-actions bot commented Jan 6, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@hymm
Copy link
Contributor

hymm commented Jan 6, 2024

could you give more detail on where the framepacing systems need to go and why? Moving the time into another system is going to make the time more inaccurate as it'll add some slop from the executor.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Jan 6, 2024

could you give more detail on where the framepacing systems need to go and why?

Framepacing systems need to go immediately before and after frame presentation. This lets us:

  • Get accurate timing information about frame presentation.
  • Insert delays before frame presentation to get framerate smoothing (increasing frame time when we want the current frame to match historical frames). This has to be immediately before frame presentation for accuracy.
  • Insert delays after frame presentation to get framepacing (delaying input polling as much as possible). EDIT: bevy_framepace implements this in RenderSet::Cleanup, which also works.

@nicopap
Copy link
Contributor

nicopap commented Jan 6, 2024

bevy_framepace adds the frame_limiter to the RenderSet::Cleanup only if there is no RenderExtractApp. There is no comments, but I suspect this is to handle pipelined rendering.

  • When pipelined rendering is not enabled, there is no RenderExtractApp, hence, it is added to RenderSet::Cleanup (supposedly, after the rendering)
  • When pipelined rendering is enabled, frame_limiter is added to Main on the RenderExtractApp. The winit loop starts as soon as RenderExtractApp ends (or on timer or window events depending on user-set UpdateMode), so it stalls the loop start until the very last moment, while still handling properly pipelined rendering.

@matiqo15 matiqo15 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 6, 2024
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 6, 2024
@UkoeHB
Copy link
Contributor Author

UkoeHB commented Jan 6, 2024

After reading this comment from @nicopap and this comment from @daxpedda, I am thoroughly confused. Where is the vblank delay, how am I supposed to measure it accurately, and how can I insert custom delays immediately before the vblank for framerate smoothing? Are these answers different for non-pipelined vs pipelined rendering? Are these answers different depending on platform (on Mac M1, the vblank is in surface_texture.present() afaict).

On another note, this PR is incomplete (or needs a follow-up). For apps with pipelined rendering we need to measure how long the main app blocks while waiting to extract into the render app (handoff slop). This can be done (theoretically) by running a subapp immediately before RenderExtractApp to collect the slop start time, and then run a system in RenderSet::ExtractCommands to collect the slop end time. However, subapp order is not configurable. Alternatively we can modify RenderExtractApp so the update_rendering() function runs a custom schedule (or custom function?) at the start (very hacky).

I want to implement a generic 'slop-reporting' mechanism that can report slop durations to the framepace engine, which collects and analyzes slop, and decides what delays to add before and after the vblank.

@daxpedda
Copy link
Contributor

daxpedda commented Jan 6, 2024

Winit offers (currently?) no way to access VBlank timings.
RedrawRequested uses some OS/compositor APIs to let it handle VSync for you.
This is basically frame-pacing provided by the OS/compositor, which depending on it's implementation is usually very crude.
Speaking in more detail about Web (the backend I maintain in Winit), it does very successfully try to align you to the monitors refresh rate; if you take longer to render it will take that into account and trigger the next RedrawRequested earlier, and so on.

See rust-windowing/winit#2412 for more details.

AFAIK some Winit backends are potentially capable of actually providing VBlank timings, at least Wayland and Windows I believe, but this is currently not implemented in Winit.
This can also be done on the GPU side of things, which is tracked here: gfx-rs/wgpu#2869.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Jan 7, 2024

After a great deal of thought I've concluded this PR does not accomplish anything based on the current frame-presentation API, and there is no way forward without gfx-rs/wgpu#2869.

@UkoeHB UkoeHB closed this Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants