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 display precision to diagnostics #6033

Open
liamcurry opened this issue Sep 20, 2022 · 4 comments
Open

Add display precision to diagnostics #6033

liamcurry opened this issue Sep 20, 2022 · 4 comments
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-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@liamcurry
Copy link

What problem does this solve or what need does it fill?

Diagnostics have no concept of display precision, so values for all diagnostics are displayed using a hard-coded static precision of 6 in the LogDiagnosticsPlugin:

"{name:<name_width$}: {value:>11.6}{suffix:1} (avg {average:>.6}{suffix:})",
name = diagnostic.name,
suffix = diagnostic.suffix,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
);
return;
}
}
info!(
target: "bevy diagnostic",
"{name:<name_width$}: {value:>.6}{suffix:}",

What solution would you like?

A new field could be added to bevy_diagnostic::Diagnostic, pub precision: usize:

pub struct Diagnostic {
pub id: DiagnosticId,
pub name: Cow<'static, str>,
pub suffix: Cow<'static, str>,
history: VecDeque<DiagnosticMeasurement>,
sum: f64,
max_history_length: usize,
pub is_enabled: bool,
}

What alternative(s) have you considered?

Instead of handling value formatting manually in plugins with format!, Diagnostic could alternatively provide a formatting function to format values. A new field could be added to bevy_diagnostic::Diagnostic, pub formatter: Arc<dyn (Fn(f64) -> String) + Send + Sync>.

This approach would give maximum flexibility, but it does incur the (possibly minimal) runtime performance cost of dynamic dispatch.

Additional context

This is relevant for a plugin I'm working on to dynamically visualize diagnostics.

https://github.com/sagan-software/bevy_diagnostic_visualizer

image

@liamcurry liamcurry added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 20, 2022
@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 D-Trivial Nice and easy! A great choice to get started with Bevy and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 21, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Sep 21, 2022

IMO we can get away with a simpler solution here. I think just allowing users to configure the display precision on a per-diagnostic basis would work totally fine.

@liamcurry
Copy link
Author

@alice-i-cecile do you mean configured as part of the LogDiagnosticsPlugin or somewhere else?

@zeroacez
Copy link
Contributor

I have never contributed before and would like to help out. Do you think I can do something here?

@alice-i-cecile
Copy link
Member

@alice-i-cecile do you mean configured as part of the LogDiagnosticsPlugin or somewhere else?

This could work. I think it makes more sense when adding diagnostics here:

diagnostics.add(Diagnostic::new(

@zeroacez yes, absolutely! Feel free to dive in and open a draft PR :) If you have any questions, feel free to either ask here, on your PR or on Discord and folks will be more than happy to help with either Rust, Bevy or Github questions you may have.

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-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants