Add display precision to ``log_diagnostics_plugin`` by zeroacez · Pull Request #6118 · bevyengine/bevy

zeroacez · 2022-09-28T10:58:24Z

Objective

Solution

Added a variable num_of_decimals to the struct Diagnostic and modified log_diagnostics_plugin to use this variable as the display precision.

…Temp standard 6.

…ecision.

… display precision.

zeroacez · 2022-09-28T11:07:25Z

I think I have done some sort of solution to the problem but I'm a little unsure of how the info! macro actually works and how the formatting works. I used $ but I don't really know why.
I took the approach to add a variable to Diagnostic. Doing this made me have to change some other places to use 6 as a temporary standard for the num_of_decimals. But at least now it's possible to set this variable when initializing the struct.

alice-i-cecile · 2022-11-02T02:44:33Z

crates/bevy_diagnostic/src/diagnostic.rs

    sum: f64,
    max_history_length: usize,
    pub is_enabled: bool,
+    pub num_of_decimals: usize, // Seams like it needs to be usize


Suggested change

pub num_of_decimals: usize, // Seams like it needs to be usize

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

Well that was a little bit embarrassing but it has been fixed now! ;)

alice-i-cecile · 2022-11-02T12:32:37Z

Progress so far is great, but I'd like to see about making this user configurable before merging it.

zeroacez · 2022-11-03T08:39:00Z

Thank you!
Sounds good. Did you have a specific way you wanted to go about making it user configurable?

alice-i-cecile · 2022-11-03T13:13:03Z

crates/bevy_asset/src/diagnostic/asset_count_diagnostics_plugin.rs

                }
            ),
            20,
+            6, // TODO: let user pick num_of_decimals


This should be hard set to 0: you can't load a fractional asset.

alice-i-cecile · 2022-11-03T13:13:54Z

crates/bevy_diagnostic/src/entity_count_diagnostics_plugin.rs


    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, 6));


Same here: fractional entities aren't a thing.

alice-i-cecile · 2022-11-03T13:18:09Z

Let me take a look in earnest! So, we can already configure this when building them manually, which is great. But for the plugins, it looks like there's two kinds in play:

Plugins that count something discrete, and should never display any decimal points (these need another refactor after this to just store and report a u64).
Plugins that should be configurable, as they report a floating point value.

For 2, I'd recommend the new pattern introduced in #6360 :)

zeroacez · 2022-11-22T11:51:27Z

Hey! I'm back looking at this and I don't think I really understand what you want me to do. Are you suggesting that I update the log_diagnostics_plugin to use this new standard or that i change something in diagnostic to take the num_of_decimals as an argument? Or do you have something entirely different in mind?

Sorry I've been busy lately but now I'm ready and hoping to get this done ASAP.

alice-i-cecile · 2022-11-22T17:57:28Z

Good questions, sorry for being unclear.

Diagnostic::num_of_decimals should store an enum. One arm should be used for integer values, the other for decimal values.

update the log_diagnostics_plugin to use this new standard

Yes: I think we can use this pattern to solve // TODO: let user pick num_of_decimals :)

…ed get fuctunality for the number of decimals to be displayed.

…imals` function to display number of decimals

zeroacez · 2022-11-24T14:05:06Z

Ok I got some help to understand what you wanted, still a little bit unsure if this is what you were thinking about, and we came up with this solution. So now it's an enum and there is a get_num_of_decimals function that returns the value and is used in log_diagnostic in log_diagnostic_plugin.

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?

alice-i-cecile · 2022-11-24T14:30:07Z

crates/bevy_diagnostic/src/diagnostic.rs

    max_history_length: usize,
    pub is_enabled: bool,
-    pub num_of_decimals: usize, // Seems like it needs to be usize
+    pub num_of_decimals: DisplayPercision, // Seems like it needs to be usize


Suggested change

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

pub num_of_decimals: DisplayPercision,

alice-i-cecile · 2022-11-24T14:30:33Z

crates/bevy_diagnostic/src/diagnostic.rs


+/// Enum for display percision. Choose IntergerValue for discrete values and DecimalValue(num_of_decimals) to specify the number of decimals
+#[derive(Debug)]
+pub enum DisplayPercision {


Precision, not percision :) In the docs too.

alice-i-cecile · 2022-11-24T14:31:52Z

crates/bevy_diagnostic/src/diagnostic.rs

    pub value: f64,
 }

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


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.

alice-i-cecile · 2022-11-24T14:32:35Z

crates/bevy_diagnostic/src/diagnostic.rs

+}
+
+impl DisplayPercision {
+    pub fn get_num_of_decimals(&self) -> usize {


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

alice-i-cecile · 2022-11-24T14:34:02Z

Good progress! This is the right direction for sure.

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?

Unsure; let me ask for some additional eyes to help get you unstuck :)

harudagondi · 2022-11-24T14:47:10Z

crates/bevy_diagnostic/src/diagnostic.rs

    ema_smoothing_factor: f64,
    max_history_length: usize,
    pub is_enabled: bool,
+    pub num_of_decimals: DisplayPercision, // Seems like it needs to be usize


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.

harudagondi · 2022-11-24T14:53:37Z

crates/bevy_diagnostic/src/diagnostic.rs

+pub enum DisplayPercision {
+    IntegerValue,
+    DecimalValue(usize),
+}


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.

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

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

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.

Do IntegerValue and DecimalValue(0) have different behavior?

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 :)

harudagondi · 2022-11-24T14:55:15Z

crates/bevy_diagnostic/src/diagnostic.rs

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


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

harudagondi · 2022-11-24T14:55:42Z

crates/bevy_diagnostic/src/diagnostic.rs

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


Suggested change

// trait DiagnosticDisplayPercision {

// type DisplayPercision;

// }

// impl DiagnosticDisplayPercision for Diagnostic {

// type DisplayPercision = DisplayPercision;

// }

harudagondi · 2022-11-24T14:56:45Z

crates/bevy_diagnostic/src/diagnostic.rs

    }

-    /// 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.


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.

harudagondi · 2022-11-24T14:57:07Z

crates/bevy_diagnostic/src/diagnostic.rs

        id: DiagnosticId,
        name: impl Into<Cow<'static, str>>,
        max_history_length: usize,
+        num_of_decimals: DisplayPercision, // Choose number of decimals for display precision.


Suggested change

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

num_of_decimals: DisplayPercision,

…m_of_decimals` to `precision`

james7132 · 2022-11-24T15:18:23Z

crates/bevy_diagnostic/src/diagnostic.rs

+
+// trait DiagnosticDisplayPercision {
+//     type DisplayPercision;
+// }


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.

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 :)

bas-ie · 2024-10-05T03:18:23Z

Backlog cleanup: closing due to inactivity, existing issue #6033 remains.

zeroacez added 3 commits September 28, 2022 11:44

Temporary fix for num_of_decimals for the Diagnistic struct. …

3fabb59

…Temp standard 6.

Added a variable num_of_decimals to Diagnistic for display pr…

6a8908f

…ecision.

Modified the log_diagnostics_plugin to use num_of_decimals as…

02bbb33

… display precision.

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 labels Sep 28, 2022

zeroacez marked this pull request as ready for review October 5, 2022 07:26

alice-i-cecile reviewed Nov 2, 2022

View reviewed changes

zeroacez added 2 commits November 2, 2022 09:53

Merge branch 'main' into add_display_precision

c903bb0

Corrected spelling mistake

d018ff2

alice-i-cecile reviewed Nov 3, 2022

View reviewed changes

zeroacez added 3 commits November 22, 2022 10:26

Merge branch 'main' into add_display_precision

7042f31

set num_of_decimals to 0

f3028c6

Removed comments

f6e0f0b

alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 22, 2022

zeroacez added 3 commits November 24, 2022 14:54

Updated to use the new pattern and removed some comments

14d4415

Added an enum DisplayPercision to be used for num_of_decimals and add…

ea4e567

…ed get fuctunality for the number of decimals to be displayed.

Updated the log_diagnostic function to call the new `get_num_of_dec…

5ff64fb

…imals` function to display number of decimals

Comments were incorrect

b25209e

alice-i-cecile reviewed Nov 24, 2022

View reviewed changes

harudagondi suggested changes Nov 24, 2022

View reviewed changes

Fixed spelling mistakes, rewrote and removed comments and renamed `nu…

22a4fb7

…m_of_decimals` to `precision`

james7132 reviewed Nov 24, 2022

View reviewed changes

bas-ie closed this Oct 5, 2024

	pub num_of_decimals: usize, // Seams like it needs to be usize
	pub num_of_decimals: usize, // Seems like it needs to be usize

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

-/// 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.

	/// 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.

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

Uh oh!

Conversation

zeroacez commented Sep 28, 2022

Objective

Solution

Uh oh!

zeroacez commented Sep 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Nov 2, 2022

Uh oh!

zeroacez commented Nov 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Nov 3, 2022

Uh oh!

zeroacez commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Nov 22, 2022

Uh oh!

zeroacez commented Nov 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Nov 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bas-ie commented Oct 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zeroacez commented Nov 22, 2022 •

edited

Loading