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

Allow users to customize history length in FrameTimeDiagnosticsPlugin #17259

Merged
merged 4 commits into from
Jan 12, 2025

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Jan 9, 2025

Objective

I have an application where I'd like to measure average frame rate over the entire life of the application, and it would be handy if I could just configure this on the existing FrameTimeDiagnosticsPlugin.

Probably fixes #10948?

Solution

Add max_history_length to FrameTimeDiagnosticsPlugin, and because smoothing_factor seems to be based on history length, add that too.

Discussion

I'm not totally sure that DEFAULT_MAX_HISTORY_LENGTH is a great default for FrameTimeDiagnosticsPlugin (or any diagnostic?). That's 1/3 of a second at typical game frame rates. Moreover, the default print interval for LogDiagnosticsPlugin is 1 second. So when the two are combined, you are printing the average over the last third of the duration between now and the previous print, which seems a bit wonky. (related: #11429)

I'm pretty sure this default value discussed and the current value wasn't totally arbitrary though.

Maybe it would be nice for Diagnostic to have a with_max_history_length_and_also_calculate_a_good_default_smoothing_factor method? And then make an explicit smoothing factor in FrameTimeDiagnosticsPlugin optional?

Or add a new(max_history_length: usize) method to FrameTimeDiagnosticsPlugin that sets a reasonable default smoothing_factor? edit: This one seems like a no-brainer, doing it.

Alternatives

It's really easy to roll your own FrameTimeDiagnosticsPlugin, but that might not be super interoperable with, for example, third party FPS overlays. Still, might be the right call.

Testing

cargo run --example many_sprites (modified to use a custom max_history_length)

Migration Guide

FrameTimeDiagnosticsPlugin now contains two fields. Use FrameTimeDiagnosticsPlugin::default() to match Bevy's previous behavior or, for example, FrameTimeDiagnosticsPlugin::new(60) to configure it.

@rparrett rparrett force-pushed the customize-frame-time-diag branch from 5ccdd81 to 2999bad Compare January 9, 2025 13:39
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 10, 2025
@alice-i-cecile
Copy link
Member

I think that DEFAULT_MAX_HISTORY_LENGTH is a bad default, but that change should be split out IMO. The existing changes are nicely uncontroversial :)

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 10, 2025
@BenjaminBrienen BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 12, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 12, 2025
Merged via the queue into bevyengine:main with commit f004789 Jan 12, 2025
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

with_smoothing_factor is currently kind of useless
3 participants