aevyrie / bevy_framepace Goto Github PK

Just make a PR here: https://github.com/bevyengine/bevy-assets

It's not clear from reading the code why this was done.

I can also imagine use cases (slideshow apps!) where this would be a genuine limitation.

I would like to use this plugin in Bevy v0.10.

Attached is a patch that adds a PID controller. I found that the derivative factor adds stutter, but using proportional and integral factors improves the 'sleep error' (difference between target sleep time and actual sleep time) dramatically. The PID design is fundamentally different from the current controller, which is setting the sleep time equal to the previous error, whereas the PID controller tries to reduce that error to zero (which makes more sense to me).

• The PID controller fixes the 3-5 FPS frame loss that the current design suffers from.
• The PID controller does NOT appear compatible with vsync when 'delay waiting for a frame to present' is not incorporated. I noticed worse latency when vsync is enabled. It seems to be random luck that the current design works well with vsync.

I hacked the bevy render function to record frame presentation delays and incorporated those into the framepace app. Doing so resulted in extremely low latencies with vsync enabled (even better than the current code, afaict). It would require a pretty critical PR to update the core bevy render function in order to get timing info on the frame presentation step.

add_PID_controller.patch

bevy_framepace panics when closing the game's window. I'm on Windows 10.

[dependencies]
bevy = "0.8"
bevy_framepace = "0.5"

use bevy::prelude::*;
use bevy_framepace::FramepacePlugin;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_plugin(FramepacePlugin::default())
        .run();
}

PS C:\Users\Hayde\Documents\Development\bevy_test> cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.42s
     Running `target\debug\bevy_test.exe`
2022-08-01T23:20:59.485277Z  INFO bevy_render::renderer: AdapterInfo { name: "Radeon RX 570 Series", vendor: 4098, device: 26591, device_type: DiscreteGpu, backend: Vulkan }
2022-08-01T23:20:59.615713Z  INFO bevy_framepace: [Update] Detected refresh rate is: 60 fps
2022-08-01T23:20:59.676518Z  WARN bevy_framepace: [Frame Drop] 73.53ms (+56578.85μs)
2022-08-01T23:20:59.755949Z  WARN bevy_framepace: [Frame Drop] 27.63ms (+10683.25μs)
2022-08-01T23:21:00.918444Z  WARN bevy_framepace: [Frame Drop] 90.74ms (+73786.45μs)
2022-08-01T23:21:00.923103Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)
2022-08-01T23:21:00.923727Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)
2022-08-01T23:21:00.934491Z  INFO bevy_winit: Skipped event for closed window: WindowId(00000000-0000-0000-0000-000000000000)
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', C:\Users\Hayde\.cargo\registry\src\github.com-1ecc6299db9ec823\bevy_framepace-0.5.0\src\lib.rs:101:51
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\bevy_test.exe` (exit code: 101)

I noticed this plugin doesn't work when compiled to wasm. This is because std::instant isn't supported on wasm aswell as the spin_sleep library. I was wondering if its possible to have a different method of frame pacing when compiling for web that doesn't use the spin_sleep library. so far replacing std::instant with the instant crate gets us part way there.

There are a number of unwraps here:

As far as I can tell, these could all just result in an early return instead, delaying the work if a monitor's frame rate could not be properly assessed.

Looks like RenderExtractApp is provided by the PipelinedRenderingPlugin, but it's disabled on wasm32.

I get a 'Sub-App with label '"RenderExtractApp"' does not exist panic.

When used, we're taking the inverse every frame (and converting it to an f32).

Instead, store an f32 frame time in the FrameRateLimit resource (see #10) and use that directly to save on wasted work and reduce complexity.

Provider helper methods to allow users to specify the settings with either frame rates or frame times.

Inserting the entire plugin as a resource is confusing and unidiomatic.

Instead:

1. Merge FrameRateLimit and FrameRateLimitParam into a single type.
2. Newtype WarnOnFrameDrop.
3. Add FrameRateLimit and WarnOnFrameDrop as resources.

I coundn't help but notice this crate uses spinlocks while thread parking exists. Why not use thread parking instead?

It's not clear why this is done, and this should be explained to both contributors and end users.

bevyengine/bevy#10692 (comment)

Incase it matters, i'm on an m1 mac

INFO bevy_render::renderer: AdapterInfo { name: "Apple M1 Pro", vendor: 0, device: 0, device_type: DiscreteGpu, backend: Metal }

There are constructors, builder patterns and mutators, all scattered together.

We should probably pick one style and stick to it.

There are two problems here:

1. There's no comment explaining intent.
2. The selected system (update_texture_cache_system feels like a very arbitrary point in time.

Cargo.toml lists render as a required feature which causes bevy to be built with a lot of unnecessary features like bevy_pbr and beby_gltf. This is unnecessary and can be simplified to only bevy_render.

Seems to be a string that gets reused that describes when the logs are occuring?

Framepacing adds a delay between the previous frame's presentation and the beginning of the current frame's update cycle. It calculates that delay based on the duration of the previous frame's update cycle.

If the current frame's update cycle takes longer than the previous frame, then the current frame's duration (from previous frame's presentation to current frame's presentation), will be longer than the target duration. This is a problem when vsync is enabled, because the current frame will miss the frame buffer refresh that comes after the previous frame's presentation step (the previous frame was 'just in time' for the buffer swap, but the current frame is 'just too late'). As a result, the previous frame will be presented twice. Depending on the vsync buffering strategy and how many frames are buffered, the current frame may be stuck on the frame presentation step all the way until the next buffer refresh.

Even very low variance in update cycle times can therefore lead to frequent frame drops on machines with unbuffered vsync. On machines with buffered vsync I think you will instead see occasional stutter based on desync between frame presentation and the time stamps of the beginning of update cycles.

Frame loss for unbuffered vsync can be solved by adding a 'buffer' to the framepace sleep based on the variance of app times (e.g. 2 standard deviations).

Since vsync is not always on, you'd need an additional framerate limiter just prior to the frame presentation step, which can absorb any remaining time to reach the target frametime. @aevyrie has observed that on machines with buffered vsync you get better results by targeting a slower framerate than the monitor refresh period -> a framerate limiter would allow compile-time configuration to achieve the best strategy for any given machine.

Every branch allocates a String just to pass it to write!(). write!() supports all the normal formatting that println!() uses, so you can just use write!() in every branch.
It doesn't matter too much though, as its only really used in the demo app.

It works on native build, but running cargo make web for wasm doesn't limit the framerate. Thanks.

Here is my repo test: https://github.com/Nickan/bevytest/tree/BevyFramepace

Edit: Provide github repo link

It feels like the match statement could be a much simpler if matches!(settings.frame_rate_limit, FrameRateLimitParam::Auto).

No significant logic is occurring elsewhere. Should be tackled after you merge FrameRateLimit and FrameRateLimitParam into a single type.

The crate needs a RenderApp to work, which is missing in headless mode.

Perhaps there is a way to remove the dependency on the RenderApp in the settings, or at least under feature?

https://github.com/aevyrie/bevy_framepace/blob/main/src/lib.rs#L229 prints every frame, can it be removed?

The logic here only reruns when the plugin settings or WinitWindows change, but I don't think that will happen just by dragging a window to another screen:

This then causes bevy_framepace to always use the initial screen's refresh rate, even when the window is moved over to another screen with a different refresh rate later, effectively rendering the plugin ineffective on that screen.

If calling detect_frametime every frame is too much work, perhaps the library could be made to work without having an accurate refresh rate value, by measuring the time bevy spends waiting for VSync? In my own experiments, I've managed to measure this by timing the executing time of the prepare_windows system.