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 log_diagnostics_plugin #6118

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::{Asset, Assets};
use bevy_app::prelude::*;
use bevy_diagnostic::{Diagnostic, DiagnosticId, Diagnostics, MAX_DIAGNOSTIC_NAME_WIDTH};
use bevy_diagnostic::{
Diagnostic, DiagnosticId, Diagnostics, DisplayPercision, MAX_DIAGNOSTIC_NAME_WIDTH,
};
use bevy_ecs::system::{Res, ResMut};

/// Adds an asset count diagnostic to an [`App`] for assets of type `T`.
Expand Down Expand Up @@ -48,6 +50,7 @@ impl<T: Asset> AssetCountDiagnosticsPlugin<T> {
}
),
20,
DisplayPercision::IntegerValue,
));
}

Expand Down
34 changes: 33 additions & 1 deletion crates/bevy_diagnostic/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,30 @@ pub struct DiagnosticMeasurement {
pub value: f64,
}

/// Enum for display percision. Choose `IntegerValue` for discrete values and `DecimalValue(num_of_decimals)` to specify the number of decimals
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Enum for display percision. Choose `IntegerValue` for discrete values and `DecimalValue(num_of_decimals)` to specify the number of decimals
/// The number of digits that should be displayed for a diagnostic.
///
/// Choose `IntegerValue` for discrete values and `DecimalValue(num_of_decimals)` to specify the number of decimals.

#[derive(Debug)]
pub enum DisplayPercision {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precision, not percision :) In the docs too.

IntegerValue,
DecimalValue(usize),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have an enum for this?

Running the code in the rust playground:

fn main() {
    let x = 5.678912345;
    dbg!(format!("{x:.6}"));
    dbg!(format!("{x:.0}"));
}

This prints:

[src/main.rs:3] format!("{x:.6}") = "5.678912"
[src/main.rs:4] format!("{x:.0}") = "6"

I don't think an enum is necessary? You can just use 0 to print an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just added the enum. Just using a usize is how I did it to begin with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. @alice-i-cecile I have a hard time understanding why turning this into an enum is better. Can you clarify why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certain diagnostics will always be integer-valued: users should not be able to configure the number of decimals for them. Other diagnostics are float-like, and users may want to round to the nearest whole number.

To me, these cases are distinct (even though the same number of decimal places are shown). Representing that in the type system means that we'll be able to expose the correct controls to users later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do IntegerValue and DecimalValue(0) have different behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in the current version of this PR. I'll play with this by next Monday, and try to get a full design here as a PR to @zeroacez branch :)


impl DisplayPercision {
pub fn get_num_of_decimals(&self) -> usize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think num_decimals is a perfectly clear and shorter name here :)

match self {
DisplayPercision::IntegerValue => 0,
DisplayPercision::DecimalValue(x) => *x,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indicates that making an enum is redundant, as you can just use usize directly here.

}
}

// trait DiagnosticDisplayPercision {
// type DisplayPercision;
// }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not leave intentionally commented out code in the codebase. If it's not meant to be included, please remove it. Otherwise, please complete the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it in in regards to this:

Also we put some trait code in comments that we tried to use so we don't have to do any new imports but it didn't work. Could it be done using this? Or is there maybe an other way?

But if there is no answer I'll remove it :)


// impl DiagnosticDisplayPercision for Diagnostic {
// type DisplayPercision = DisplayPercision;
// }

Comment on lines +49 to +56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// trait DiagnosticDisplayPercision {
// type DisplayPercision;
// }
// impl DiagnosticDisplayPercision for Diagnostic {
// type DisplayPercision = DisplayPercision;
// }

/// A timeline of [`DiagnosticMeasurement`]s of a specific type.
/// Diagnostic examples: frames per second, CPU usage, network latency
#[derive(Debug)]
Expand All @@ -41,6 +65,7 @@ pub struct Diagnostic {
ema_smoothing_factor: f64,
max_history_length: usize,
pub is_enabled: bool,
pub num_of_decimals: DisplayPercision, // Seems like it needs to be usize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub num_of_decimals: DisplayPercision, // Seems like it needs to be usize
pub num_of_decimals: DisplayPercision,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name num_of_decimals is clunky and does not clarify its intent. I prefer the name precision as mentioned in the original issue.

}

impl Diagnostic {
Expand Down Expand Up @@ -73,11 +98,12 @@ impl Diagnostic {
.push_back(DiagnosticMeasurement { time, value });
}

/// Create a new diagnostic with the given ID, name and maximum history.
/// Create a new diagnostic with the given ID, name, maximum history and number of decimals.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Create a new diagnostic with the given ID, name, maximum history and number of decimals.
/// Create a new diagnostic with the given ID, name, maximum history and decimal precision.

pub fn new(
id: DiagnosticId,
name: impl Into<Cow<'static, str>>,
max_history_length: usize,
num_of_decimals: DisplayPercision, // Choose number of decimals for display precision.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
num_of_decimals: DisplayPercision, // Choose number of decimals for display precision.
num_of_decimals: DisplayPercision,

) -> Diagnostic {
let name = name.into();
if name.chars().count() > MAX_DIAGNOSTIC_NAME_WIDTH {
Expand All @@ -98,6 +124,7 @@ impl Diagnostic {
ema: 0.0,
ema_smoothing_factor: 2.0 / 21.0,
is_enabled: true,
num_of_decimals,
}
}

Expand Down Expand Up @@ -195,6 +222,11 @@ impl Diagnostic {
pub fn clear_history(&mut self) {
self.history.clear();
}

/// Get the number of decimals to display for this diagnostic.
pub fn get_num_of_decimals(&self) -> usize {
self.num_of_decimals.get_num_of_decimals()
}
}

/// A collection of [Diagnostic]s
Expand Down
9 changes: 7 additions & 2 deletions crates/bevy_diagnostic/src/entity_count_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use bevy_app::{App, Plugin};
use bevy_ecs::{entity::Entities, system::ResMut};

use crate::{Diagnostic, DiagnosticId, Diagnostics};
use crate::{Diagnostic, DiagnosticId, Diagnostics, DisplayPercision};

/// Adds "entity count" diagnostic to an App
#[derive(Default)]
Expand All @@ -19,7 +19,12 @@ impl EntityCountDiagnosticsPlugin {
DiagnosticId::from_u128(187513512115068938494459732780662867798);

pub fn setup_system(mut diagnostics: ResMut<Diagnostics>) {
diagnostics.add(Diagnostic::new(Self::ENTITY_COUNT, "entity_count", 20));
diagnostics.add(Diagnostic::new(
Self::ENTITY_COUNT,
"entity_count",
20,
DisplayPercision::IntegerValue,
));
}

pub fn diagnostic_system(mut diagnostics: ResMut<Diagnostics>, entities: &Entities) {
Expand Down
27 changes: 22 additions & 5 deletions crates/bevy_diagnostic/src/frame_time_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{Diagnostic, DiagnosticId, Diagnostics};
use crate::{Diagnostic, DiagnosticId, Diagnostics, DisplayPercision};
use bevy_app::prelude::*;
use bevy_core::FrameCount;
use bevy_ecs::system::{Res, ResMut};
Expand All @@ -23,10 +23,27 @@ impl FrameTimeDiagnosticsPlugin {
DiagnosticId::from_u128(73441630925388532774622109383099159699);

pub fn setup_system(mut diagnostics: ResMut<Diagnostics>) {
diagnostics.add(Diagnostic::new(Self::FRAME_TIME, "frame_time", 20).with_suffix("ms"));
diagnostics.add(Diagnostic::new(Self::FPS, "fps", 20));
diagnostics
.add(Diagnostic::new(Self::FRAME_COUNT, "frame_count", 1).with_smoothing_factor(0.0));
diagnostics.add(
Diagnostic::new(
Self::FRAME_TIME,
"frame_time",
20,
DisplayPercision::DecimalValue(6),
)
.with_suffix("ms"),
);
diagnostics.add(Diagnostic::new(
Self::FPS,
"fps",
20,
DisplayPercision::DecimalValue(6),
));
diagnostics.add(Diagnostic::new(
Self::FRAME_COUNT,
"frame_count",
1,
DisplayPercision::IntegerValue,
));
}

pub fn diagnostic_system(
Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_diagnostic/src/log_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,22 @@ impl LogDiagnosticsPlugin {
// so we reserve two columns for it; however,
// Do not reserve columns for the suffix in the average
// The ) hugging the value is more aesthetically pleasing
"{name:<name_width$}: {value:>11.6}{suffix:2} (avg {average:>.6}{suffix:})",
"{name:<name_width$}: {value:>11.num_of_decimals$}{suffix:2} (avg {average:>.num_of_decimals$}{suffix:})",
name = diagnostic.name,
suffix = diagnostic.suffix,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
num_of_decimals = diagnostic.get_num_of_decimals(),
);
return;
}
}
info!(
target: "bevy diagnostic",
"{name:<name_width$}: {value:>.6}{suffix:}",
"{name:<name_width$}: {value:>.num_of_decimals$}{suffix:}",
name = diagnostic.name,
suffix = diagnostic.suffix,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
num_of_decimals = diagnostic.get_num_of_decimals(),
);
}
}
Expand Down
3 changes: 2 additions & 1 deletion examples/diagnostics/custom_diagnostic.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! This example illustrates how to create a custom diagnostic.

use bevy::{
diagnostic::{Diagnostic, DiagnosticId, Diagnostics, LogDiagnosticsPlugin},
diagnostic::{Diagnostic, DiagnosticId, Diagnostics, DisplayPercision, LogDiagnosticsPlugin},
prelude::*,
};

Expand All @@ -28,6 +28,7 @@ fn setup_diagnostic_system(mut diagnostics: ResMut<Diagnostics>) {
SYSTEM_ITERATION_COUNT,
"system_iteration_count",
10,
DisplayPercision::IntegerValue,
));
}

Expand Down