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

[Merged by Bors] - fix diagnostic length for asset count #2165

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
15 changes: 13 additions & 2 deletions crates/bevy_asset/src/diagnostic/asset_count_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{Asset, Assets};
use bevy_app::prelude::*;
use bevy_diagnostic::{Diagnostic, DiagnosticId, Diagnostics};
use bevy_diagnostic::{Diagnostic, DiagnosticId, Diagnostics, MAX_DIAGNOSTIC_NAME_WIDTH};
use bevy_ecs::system::{IntoSystem, Res, ResMut};

/// Adds "asset count" diagnostic to an App
Expand Down Expand Up @@ -29,9 +29,20 @@ impl<T: Asset> AssetCountDiagnosticsPlugin<T> {
}

pub fn setup_system(mut diagnostics: ResMut<Diagnostics>) {
let asset_type_name = std::any::type_name::<T>();
let max_length = MAX_DIAGNOSTIC_NAME_WIDTH - "asset_count ".len();
diagnostics.add(Diagnostic::new(
Self::diagnostic_id(),
format!("asset_count {}", std::any::type_name::<T>()),
format!(
"asset_count {}",
if asset_type_name.len() > max_length {
asset_type_name
.split_at(asset_type_name.len() - max_length + 1)
.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.1
.0

This should be the first &str returned by split_at (assuming you want the first part of the type name).

Copy link
Member Author

Choose a reason for hiding this comment

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

I want the second, the first is most likely to be part of the path: asset_count bevy_render::texture::texture::Texture

} else {
asset_type_name
}
),
20,
));
}
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_diagnostic/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use bevy_log::warn;
use bevy_utils::{Duration, Instant, StableHashMap, Uuid};
use std::{borrow::Cow, collections::VecDeque};

use crate::log_diagnostics_plugin::MAX_LOG_NAME_WIDTH;
use crate::MAX_DIAGNOSTIC_NAME_WIDTH;

/// Unique identifier for a [Diagnostic]
#[derive(Debug, Copy, Clone, Hash, Eq, PartialEq, PartialOrd, Ord)]
Expand Down Expand Up @@ -59,12 +59,12 @@ impl Diagnostic {
max_history_length: usize,
) -> Diagnostic {
let name = name.into();
if name.chars().count() > MAX_LOG_NAME_WIDTH {
if name.chars().count() > MAX_DIAGNOSTIC_NAME_WIDTH {
// This could be a false positive due to a unicode width being shorter
warn!(
"Diagnostic {:?} has name longer than {} characters, and so might overflow in the LogDiagnosticsPlugin\
Consider using a shorter name.",
name, MAX_LOG_NAME_WIDTH
name, MAX_DIAGNOSTIC_NAME_WIDTH
)
}
Diagnostic {
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_diagnostic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,7 @@ impl Plugin for DiagnosticsPlugin {
app.init_resource::<Diagnostics>();
}
}

/// The width which diagnostic names will be printed as
/// Plugin names should not be longer than this value
pub const MAX_DIAGNOSTIC_NAME_WIDTH: usize = 32;
Comment on lines +22 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of having this limit in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

it came in #1505

Copy link
Member Author

Choose a reason for hiding this comment

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

it was to clean up how it's displayed, and it's an issue only for this diagnostics as all other have a fixed name

8 changes: 2 additions & 6 deletions crates/bevy_diagnostic/src/log_diagnostics_plugin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ impl Default for LogDiagnosticsPlugin {
}
}

/// The width which diagnostic names will be printed as
/// Plugin names should not be longer than this value
pub(crate) const MAX_LOG_NAME_WIDTH: usize = 32;

impl Plugin for LogDiagnosticsPlugin {
fn build(&self, app: &mut bevy_app::AppBuilder) {
app.insert_resource(LogDiagnosticsState {
Expand Down Expand Up @@ -71,15 +67,15 @@ impl LogDiagnosticsPlugin {
// Do not reserve one column for the suffix in the average
// The ) hugging the value is more aesthetically pleasing
format!("{:.6}{:}", average, diagnostic.suffix),
name_width = MAX_LOG_NAME_WIDTH,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
);
} else {
info!(
target: "bevy diagnostic",
"{:<name_width$}: {:>}",
diagnostic.name,
format!("{:.6}{:}", value, diagnostic.suffix),
name_width = MAX_LOG_NAME_WIDTH,
name_width = crate::MAX_DIAGNOSTIC_NAME_WIDTH,
);
}
}
Expand Down