Add Display implementation to DebugName. #13760

gagnus · 2024-06-08T22:15:44Z

Objective

When writing "in game" debugging tools, quite often you need the name of an entity (for example an entity tree). DebugName is the usual way of doing that.
A recent change to Entity's Debug implementation meant it was no longer a minimal {index}v{generation} but instead a more verbose auto generated Debug.
This made DebugName's Debug implementation also verbose

Solution

I changed DebugName to derive Debug automatically and added a new (preferred) Display implementation for it which is the preferred name for an entity. If the entity has a Name component its the contents of that, otherwise it is {index}v{generation} (though this does not use Display of the Entity as that is more verbose than this).

Testing

I've added a new test in name.rs which tests the Display implementation for DebugName by using to_string.

Migration Guide

In code which uses DebugName you should now use the Display implementation rather than the Debug implementation (ie {} instead of {:?} if you were printing it out).

…ties which have one or the entity's own Display impl for entities which don't.

…part was redundant (it was generation << 32 | index). This might be contraversial and isn't required for the DebugName change.

alice-i-cecile · 2024-06-08T22:23:01Z

Additionally (more controversial maybe) I have returned the Display implementation in Entity to just be {index}v{generation} rather than also having |{bits} on (where {bits} is a 64-bit decimal version of generation << 32 | index).

I don't like this change personally, but we should definitely split this off to make sure the other work doesn't get blocked.

…he bits part was redundant (it was generation << 32 | index). This might be contraversial and isn't required for the DebugName change." This reverts commit 38d14fa.

…s the Display for entities is more verbose than this.

gagnus · 2024-06-09T09:04:42Z

I've updated to no longer change the Display implementation for Entity and instead use {index}v{generation} directly in DebugName.

crates/bevy_core/src/name.rs

janhohenheim

Good change, just some minor nitpicks :)

crates/bevy_core/src/name.rs

janhohenheim · 2024-06-21T09:40:22Z

crates/bevy_core/src/name.rs

@@ -100,26 +100,31 @@ impl std::fmt::Debug for Name {
 ///     for (name, mut score) in &mut scores {
 ///         score.0 += 1.0;
 ///         if score.0.is_nan() {
-///             bevy_utils::tracing::error!("Score for {:?} is invalid", name);
+///             // use the Display impl to return either the Name


This kind of implementation information seems a bit too in-depth for an example. Additionally, the part about debug being more verbose is imo implicitly to be expected.

Suggestion: move this information outside of the example (maybe a section # Implementation?) and remove references to Debug

Thanks for this, have updated the code if you could check its what you meant.

Perfect, thanks :)

crates/bevy_core/src/name.rs

review comment Co-authored-by: Jan Hohenheim <[email protected]>

review fix Co-authored-by: Andres O. Vela <[email protected]>

alice-i-cecile · 2024-06-22T19:38:48Z

@eidloi, does this solve #13980 for you?

eidloi · 2024-06-22T19:45:40Z

@eidloi, does this solve #13980 for you?

No, because it needs querying an additional component which may not be possible in some contexts. I mean it's not a bad thing to have but I find it somewhat redundant.

gagnus · 2024-06-25T07:24:17Z

Part of the original change was to make Display on an entity simpler (get rid of the redundant bit), but that change was I think more controversial than this. I’d support doing that, so this change is just a switch between two displays.

# Objective bevyengine#12469 changed the `Debug` impl for `Entity`, making sure it's actually accurate for debugging. To ensure that its can still be readily logged in error messages and inspectors, this PR added a more concise and human-friendly `Display` impl. However, users found this form too verbose: the `to_bits` information was unhelpful and too long. Fixes bevyengine#13980. ## Solution - Don't include `Entity::to_bits` in the `Display` implementation for `Entity`. This information can readily be accessed and logged for users who need it. - Also clean up the implementation of `Display` for `DebugName`, introduced in bevyengine#13760, to simply use the new `Display` impl (since this was the desired format there). ## Testing I've updated an existing test to verify the output of `Entity::display`. --------- Co-authored-by: Kristoffer Søholm <[email protected]>

accurate for debugging. To ensure that its can still be readily logged in error messages and inspectors, this PR added a more concise and human-friendly `Display` impl. However, users found this form too verbose: the `to_bits` information was unhelpful and too long. Fixes #13980. - Don't include `Entity::to_bits` in the `Display` implementation for `Entity`. This information can readily be accessed and logged for users who need it. - Also clean up the implementation of `Display` for `DebugName`, introduced in #13760, to simply use the new `Display` impl (since this was the desired format there). I've updated an existing test to verify the output of `Entity::display`. --------- Co-authored-by: Kristoffer Søholm <[email protected]>

gagnus · 2024-07-07T11:47:03Z

@alice-i-cecile will this automatically go into 0.15 or can it go into an update to 0.14?

alice-i-cecile · 2024-07-07T12:32:44Z

Looks like this will be in 0.15 :)

gagnus added 2 commits June 8, 2024 22:35

Add a new Display impl for DebugName which displays the Name for enti…

eb2ddda

…ties which have one or the entity's own Display impl for entities which don't.

Returned Display for Entity to just {index}v{generation} as the bits …

38d14fa

…part was redundant (it was generation << 32 | index). This might be contraversial and isn't required for the DebugName change.

alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Jun 8, 2024

gagnus added 2 commits June 9, 2024 09:57

Revert "Returned Display for Entity to just {index}v{generation} as t…

fac703e

…he bits part was redundant (it was generation << 32 | index). This might be contraversial and isn't required for the DebugName change." This reverts commit 38d14fa.

Use {index}v{generation} in the Display implementation of DebugName a…

3e02519

…s the Display for entities is more verbose than this.

cargo fmt --all

e591f6b

alice-i-cecile reviewed Jun 9, 2024

View reviewed changes

crates/bevy_core/src/name.rs Show resolved Hide resolved

alice-i-cecile approved these changes Jun 9, 2024

View reviewed changes

alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 9, 2024

janhohenheim suggested changes Jun 21, 2024

View reviewed changes

andresovela reviewed Jun 21, 2024

View reviewed changes

crates/bevy_core/src/name.rs Outdated Show resolved Hide resolved

gagnus and others added 3 commits June 22, 2024 10:48

Update crates/bevy_core/src/name.rs

d701354

review comment Co-authored-by: Jan Hohenheim <[email protected]>

Update crates/bevy_core/src/name.rs

a7c199f

review fix Co-authored-by: Andres O. Vela <[email protected]>

Suggestion on better documentation

b3baea3

janhohenheim approved these changes Jun 22, 2024

View reviewed changes

janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 22, 2024

alice-i-cecile added this pull request to the merge queue Jun 25, 2024

Merged via the queue into bevyengine:main with commit 1df811e Jun 25, 2024
28 checks passed

alice-i-cecile mentioned this pull request Jun 25, 2024

Don't show .to_bits in Display impl for Entity #14011

Merged

janhohenheim added this to the 0.15 milestone Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Display implementation to DebugName. #13760

Add Display implementation to DebugName. #13760

gagnus commented Jun 8, 2024 •

edited

Loading

alice-i-cecile commented Jun 8, 2024

gagnus commented Jun 9, 2024

janhohenheim left a comment

janhohenheim Jun 21, 2024

gagnus Jun 22, 2024

janhohenheim Jun 22, 2024

alice-i-cecile commented Jun 22, 2024

eidloi commented Jun 22, 2024

gagnus commented Jun 25, 2024

gagnus commented Jul 7, 2024

alice-i-cecile commented Jul 7, 2024

Add Display implementation to DebugName. #13760

Add Display implementation to DebugName. #13760

Conversation

gagnus commented Jun 8, 2024 • edited Loading

Objective

Solution

Testing

Migration Guide

alice-i-cecile commented Jun 8, 2024

gagnus commented Jun 9, 2024

janhohenheim left a comment

Choose a reason for hiding this comment

janhohenheim Jun 21, 2024

Choose a reason for hiding this comment

gagnus Jun 22, 2024

Choose a reason for hiding this comment

janhohenheim Jun 22, 2024

Choose a reason for hiding this comment

alice-i-cecile commented Jun 22, 2024

eidloi commented Jun 22, 2024

gagnus commented Jun 25, 2024

gagnus commented Jul 7, 2024

alice-i-cecile commented Jul 7, 2024

gagnus commented Jun 8, 2024 •

edited

Loading