Fix inconsistency between Debug and serialized representation of Entity #12469
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Derive `Debug` impl for `Entity` - Add impl `Display` for `Entity` - Add `entity_display` display to check the output contains all required info
|
could you give this PR a more descriptive title? The title ends up in the commit history of the main branch |
alice-i-cecile
changed the title
alice-i-cecile
added
A-ECS
alice-i-cecile
approved these changes
Mar 14, 2024
james7132
approved these changes
Mar 14, 2024
james7132
added
the
S-Ready-For-Final-Review
|
Does this need a release note? Not to knock on the work here, but I don't think it does. |
alice-i-cecile
removed
the
C-Needs-Release-Note
Luracasmus
pushed a commit
to Luracasmus/bevy
that referenced
this pull request
Jun 25, 2024
# 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 <k.soeholm@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Fixes #12139
Solution
Debugimpl forEntityDisplayforEntityentity_displaytest to check the output contains all required infoI decided to go with
0v0|1234format as opposed to the0v0[1234]which was initially discussed in the issue.My rationale for this is that
[1234]may be confused for index values, which may be common in logs, and so searching for entities by text would become harder. I figured|1234would help the entity IDs stand out more.Additionally, I'm a little concerned that this change is gonna break existing logging for projects because
Debugis now going to be a multi-line output. But maybe this is ok.We could implement
Debugto be a single-line output, but then I don't see why it would be different fromDisplayat all.@alice-i-cecile Let me know if we'd like to make any changes based on these points.