Cleanups in diagnostics (bevyengine#3871)

- changed `EntityCountDiagnosticsPlugin` to not use an exclusive system to get its entity count
- removed mention of `WgpuResourceDiagnosticsPlugin` in example `log_diagnostics` as it doesn't exist anymore
- added ability to enable, disable ~~or toggle~~ a diagnostic (fix bevyengine#3767)
- made diagnostic values lazy, so they are only computed if the diagnostic is enabled
- do not log an average for diagnostics with only one value
- removed `sum` function from diagnostic as it isn't really useful
- ~~do not keep an average of the FPS diagnostic. it is already an average on the last 20 frames, so the average FPS was an average of the last 20 frames over the last 20 frames~~
- do not compute the FPS value as an average over the last 20 frames but give the actual "instant FPS"
- updated log format to use variable capture
- added some doc
- the frame counter diagnostic value can be reseted to 0

crates/bevy_asset/src/diagnostic/asset_count_diagnostics_plugin.rs

@@ -48,6 +48,6 @@ impl<T: Asset> AssetCountDiagnosticsPlugin<T> {
     }
 
     pub fn diagnostic_system(mut diagnostics: ResMut<Diagnostics>, assets: Res<Assets<T>>) {
-        diagnostics.add_measurement(Self::diagnostic_id(), assets.len() as f64);
+        diagnostics.add_measurement(Self::diagnostic_id(), || assets.len() as f64);
     }
 }

crates/bevy_diagnostic/src/diagnostic.rs

@@ -37,22 +37,30 @@ pub struct Diagnostic {
     history: VecDeque<DiagnosticMeasurement>,
     sum: f64,
     max_history_length: usize,
+    pub is_enabled: bool,
 }
 
 impl Diagnostic {
+    /// Add a new value as a [`DiagnosticMeasurement`]. Its timestamp will be [`Instant::now`].
     pub fn add_measurement(&mut self, value: f64) {
         let time = Instant::now();
-        if self.history.len() == self.max_history_length {
-            if let Some(removed_diagnostic) = self.history.pop_front() {
-                self.sum -= removed_diagnostic.value;
+        if self.max_history_length > 1 {
+            if self.history.len() == self.max_history_length {
+                if let Some(removed_diagnostic) = self.history.pop_front() {
+                    self.sum -= removed_diagnostic.value;
+                }
             }
-        }
 
-        self.sum += value;
+            self.sum += value;
+        } else {
+            self.history.clear();
+            self.sum = value;
+        }
         self.history
             .push_back(DiagnosticMeasurement { time, value });
     }
 
+    /// Create a new diagnostic with the given ID, name and maximum history.
     pub fn new(
         id: DiagnosticId,
         name: impl Into<Cow<'static, str>>,
@@ -74,28 +82,30 @@ impl Diagnostic {
             history: VecDeque::with_capacity(max_history_length),
             max_history_length,
             sum: 0.0,
+            is_enabled: true,
         }
     }
 
+    /// Add a suffix to use when logging the value, can be used to show a unit.
     #[must_use]
     pub fn with_suffix(mut self, suffix: impl Into<Cow<'static, str>>) -> Self {
         self.suffix = suffix.into();
         self
     }
 
+    /// Get the latest measurement from this diagnostic.
     #[inline]
     pub fn measurement(&self) -> Option<&DiagnosticMeasurement> {
         self.history.back()
     }
 
+    /// Get the latest value from this diagnostic.
     pub fn value(&self) -> Option<f64> {
         self.measurement().map(|measurement| measurement.value)
     }
 
-    pub fn sum(&self) -> f64 {
-        self.sum
-    }
-
+    /// Return the mean (average) of this diagnostic's values.
+    /// N.B. this a cheap operation as the sum is cached.
     pub fn average(&self) -> Option<f64> {
         if !self.history.is_empty() {
             Some(self.sum / self.history.len() as f64)
@@ -104,10 +114,12 @@ impl Diagnostic {
         }
     }
 
+    /// Return the number of elements for this diagnostic.
     pub fn history_len(&self) -> usize {
         self.history.len()
     }
 
+    /// Return the duration between the oldest and most recent values for this diagnostic.
     pub fn duration(&self) -> Option<Duration> {
         if self.history.len() < 2 {
             return None;
@@ -122,6 +134,7 @@ impl Diagnostic {
         None
     }
 
+    /// Return the maximum number of elements for this diagnostic.
     pub fn get_max_history_length(&self) -> usize {
         self.max_history_length
     }
@@ -133,6 +146,11 @@ impl Diagnostic {
     pub fn measurements(&self) -> impl Iterator<Item = &DiagnosticMeasurement> {
         self.history.iter()
     }
+
+    /// Clear the history of this diagnostic.
+    pub fn clear_history(&mut self) {
+        self.history.clear();
+    }
 }
 
 /// A collection of [Diagnostic]s
@@ -144,6 +162,7 @@ pub struct Diagnostics {
 }
 
 impl Diagnostics {
+    /// Add a new [`Diagnostic`].
     pub fn add(&mut self, diagnostic: Diagnostic) {
         self.diagnostics.insert(diagnostic.id, diagnostic);
     }
@@ -156,18 +175,31 @@ impl Diagnostics {
         self.diagnostics.get_mut(&id)
     }
 
+    /// Get the latest [`DiagnosticMeasurement`] from an enabled [`Diagnostic`].
     pub fn get_measurement(&self, id: DiagnosticId) -> Option<&DiagnosticMeasurement> {
         self.diagnostics
             .get(&id)
+            .filter(|diagnostic| diagnostic.is_enabled)
             .and_then(|diagnostic| diagnostic.measurement())
     }
 
-    pub fn add_measurement(&mut self, id: DiagnosticId, value: f64) {
-        if let Some(diagnostic) = self.diagnostics.get_mut(&id) {
-            diagnostic.add_measurement(value);
+    /// Add a measurement to an enabled [`Diagnostic`]. The measurement is passed as a function so that
+    /// it will be evaluated only if the [`Diagnostic`] is enabled. This can be useful if the value is
+    /// costly to calculate.
+    pub fn add_measurement<F>(&mut self, id: DiagnosticId, value: F)
+    where
+        F: FnOnce() -> f64,
+    {
+        if let Some(diagnostic) = self
+            .diagnostics
+            .get_mut(&id)
+            .filter(|diagnostic| diagnostic.is_enabled)
+        {
+            diagnostic.add_measurement(value());
         }
     }
 
+    /// Return an iterator over all [`Diagnostic`].
     pub fn iter(&self) -> impl Iterator<Item = &Diagnostic> {
         self.diagnostics.values()
     }

crates/bevy_diagnostic/src/entity_count_diagnostics_plugin.rs

@@ -1,8 +1,5 @@
 use bevy_app::{App, Plugin};
-use bevy_ecs::{
-    system::{IntoExclusiveSystem, ResMut},
-    world::World,
-};
+use bevy_ecs::{entity::Entities, system::ResMut};
 
 use crate::{Diagnostic, DiagnosticId, Diagnostics};
 
@@ -13,7 +10,7 @@ pub struct EntityCountDiagnosticsPlugin;
 impl Plugin for EntityCountDiagnosticsPlugin {
     fn build(&self, app: &mut App) {
         app.add_startup_system(Self::setup_system)
-            .add_system(Self::diagnostic_system.exclusive_system());
+            .add_system(Self::diagnostic_system);
     }
 }
 
@@ -25,10 +22,7 @@ impl EntityCountDiagnosticsPlugin {
         diagnostics.add(Diagnostic::new(Self::ENTITY_COUNT, "entity_count", 20));
     }
 
-    pub fn diagnostic_system(world: &mut World) {
-        let entity_count = world.entities().len();
-        if let Some(mut diagnostics) = world.get_resource_mut::<Diagnostics>() {
-            diagnostics.add_measurement(Self::ENTITY_COUNT, entity_count as f64);
-        }
+    pub fn diagnostic_system(mut diagnostics: ResMut<Diagnostics>, entities: &Entities) {
+        diagnostics.add_measurement(Self::ENTITY_COUNT, || entities.len() as f64);
     }
 }

crates/bevy_diagnostic/src/frame_time_diagnostics_plugin.rs

@@ -37,29 +37,23 @@ impl FrameTimeDiagnosticsPlugin {
         time: Res<Time>,
         mut state: ResMut<FrameTimeDiagnosticsState>,
     ) {
-        state.frame_count = state.frame_count.wrapping_add(1);
-        diagnostics.add_measurement(Self::FRAME_COUNT, state.frame_count as f64);
+        diagnostics.add_measurement(Self::FRAME_COUNT, || {
+            state.frame_count = state.frame_count.wrapping_add(1);
+            state.frame_count as f64
+        });
 
         if time.delta_seconds_f64() == 0.0 {
             return;
         }
 
-        diagnostics.add_measurement(Self::FRAME_TIME, time.delta_seconds_f64());
-        if let Some(fps) = diagnostics
-            .get(Self::FRAME_TIME)
-            .and_then(|frame_time_diagnostic| {
-                frame_time_diagnostic
-                    .average()
-                    .and_then(|frame_time_average| {
-                        if frame_time_average > 0.0 {
-                            Some(1.0 / frame_time_average)
-                        } else {
-                            None
-                        }
-                    })
-            })
-        {
-            diagnostics.add_measurement(Self::FPS, fps);
-        }
+        diagnostics.add_measurement(Self::FRAME_TIME, || time.delta_seconds_f64());
+
+        diagnostics.add_measurement(Self::FPS, || 1.0 / time.delta_seconds_f64());
+    }
+}
+
+impl FrameTimeDiagnosticsState {
+    pub fn reset_frame_count(&mut self) {
+        self.frame_count = 0;
     }
 }

crates/bevy_diagnostic/src/log_diagnostics_plugin.rs

@@ -53,30 +53,29 @@ impl LogDiagnosticsPlugin {
 
     fn log_diagnostic(diagnostic: &Diagnostic) {
         if let Some(value) = diagnostic.value() {
-            if let Some(average) = diagnostic.average() {
-                info!(
-                    target: "bevy diagnostic",
-                    // Suffix is only used for 's' as in seconds currently,
-                    // so we reserve one column for it; however,
-                    // Do not reserve one column for the suffix in the average
-                    // The ) hugging the value is more aesthetically pleasing
-                    "{name:<name_width$}: {value:>11.6}{suffix:1} (avg {average:>.6}{suffix:})",
-                    name = diagnostic.name,
-                    value = value,
-                    suffix = diagnostic.suffix,
-                    average = average,
-                    name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
-                );
-            } else {
-                info!(
-                    target: "bevy diagnostic",
-                    "{name:<name_width$}: {value:>.6}{suffix:}",
-                    name = diagnostic.name,
-                    value = value,
-                    suffix = diagnostic.suffix,
-                    name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
-                );
+            if diagnostic.get_max_history_length() > 1 {
+                if let Some(average) = diagnostic.average() {
+                    info!(
+                        target: "bevy diagnostic",
+                        // Suffix is only used for 's' as in seconds currently,
+                        // so we reserve one column for it; however,
+                        // Do not reserve one column for the suffix in the average
+                        // The ) hugging the value is more aesthetically pleasing
+                        "{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:}",
+                name = diagnostic.name,
+                suffix = diagnostic.suffix,
+                name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
+            );
         }
     }
 
@@ -87,11 +86,18 @@ impl LogDiagnosticsPlugin {
     ) {
         if state.timer.tick(time.delta()).finished() {
             if let Some(ref filter) = state.filter {
-                for diagnostic in filter.iter().map(|id| diagnostics.get(*id).unwrap()) {
+                for diagnostic in filter.iter().flat_map(|id| {
+                    diagnostics
+                        .get(*id)
+                        .filter(|diagnostic| diagnostic.is_enabled)
+                }) {
                     Self::log_diagnostic(diagnostic);
                 }
             } else {
-                for diagnostic in diagnostics.iter() {
+                for diagnostic in diagnostics
+                    .iter()
+                    .filter(|diagnostic| diagnostic.is_enabled)
+                {
                     Self::log_diagnostic(diagnostic);
                 }
             }
@@ -105,11 +111,18 @@ impl LogDiagnosticsPlugin {
     ) {
         if state.timer.tick(time.delta()).finished() {
             if let Some(ref filter) = state.filter {
-                for diagnostic in filter.iter().map(|id| diagnostics.get(*id).unwrap()) {
+                for diagnostic in filter.iter().flat_map(|id| {
+                    diagnostics
+                        .get(*id)
+                        .filter(|diagnostic| diagnostic.is_enabled)
+                }) {
                     debug!("{:#?}\n", diagnostic);
                 }
             } else {
-                for diagnostic in diagnostics.iter() {
+                for diagnostic in diagnostics
+                    .iter()
+                    .filter(|diagnostic| diagnostic.is_enabled)
+                {
                     debug!("{:#?}\n", diagnostic);
                 }
             }

examples/diagnostics/custom_diagnostic.rs

@@ -33,5 +33,5 @@ fn setup_diagnostic_system(mut diagnostics: ResMut<Diagnostics>) {
 
 fn my_system(mut diagnostics: ResMut<Diagnostics>) {
     // Add a measurement of 10.0 for our diagnostic each time this system runs.
-    diagnostics.add_measurement(SYSTEM_ITERATION_COUNT, 10.0);
+    diagnostics.add_measurement(SYSTEM_ITERATION_COUNT, || 10.0);
 }