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

Inconsistency between Debug and serialized representation of Entity #12139

Closed
Zeenobit opened this issue Feb 26, 2024 · 6 comments · Fixed by #12469 or #14539
Closed

Inconsistency between Debug and serialized representation of Entity #12139

Zeenobit opened this issue Feb 26, 2024 · 6 comments · Fixed by #12469 or #14539
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Contentious There are nontrivial implications that should be thought through

Comments

@Zeenobit
Copy link
Contributor

Bevy version

0.13

What went wrong

There is an inconsistency between the Debug representation of an Entity:

impl fmt::Debug for Entity {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}v{}", self.index(), self.generation())
    }
}

And its Serialize representation:

impl Serialize for Entity {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: serde::Serializer,
    {
        serializer.serialize_u64(self.to_bits())
    }
}

This makes it difficult to debug serialized outputs, since the serialized entity IDs cannot be easily mapped to runtime entities via human inspection. Ideally, these representations should be consistent for easier debugging.

Additional information

I wanted to just open a PR on this, but I think the issue warrants some discussion. Mainly on two points:

  • Which representation is "correct"?
  • If we swap to #v# format for serialized data, are we ok with invalidating existing scenes?
@Zeenobit Zeenobit added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Feb 26, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Scenes Serialized ECS data stored on the disk S-Needs-Investigation This issue requires detective work to figure out what's going wrong A-Networking Sending data between clients, servers and machines S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Triage This issue needs to be labelled S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels Feb 26, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 26, 2024

Ah: I know what we should do. The serialized representation should stay the same, optimized for information density. This is just an opaque identifier. However, we should make the debug representation more verbose, and report index, generation and raw_bits separately.

In the future, we will be packing more information into here (e.g. entity disabling) that we want to add to the debug output, but we should avoid adding bloat (or semantic meaning) to the serialized representation. Displaying the raw bits separately gives us the best of both worlds, as users will be able to make this correspondence themselves.

@Zeenobit
Copy link
Contributor Author

Zeenobit commented Feb 26, 2024

I like that idea.

My only concern there is that the current "#v#" format for Debug is very compact. This makes it very useful for log messages, since the user can just do info!("{entity:?} is a cool entity!").
This would output something like:
"2v2 is a cool entity"

I'm concerned if we make the Debug more verbose, we'd lose this feature. This would open the door to people using either the entity index/generation or the raw bits in log messages, which would produce highly inconsistent log outputs overall.

@alice-i-cecile
Copy link
Member

I'm concerned if we make the Debug more verbose, we'd lose this feature. This would open the door to people using either the entity index/generation or the raw bits in log messages, which would produce highly inconsistent log outputs overall.

That seems like a prime case for a Display impl then: "pretty, humand-readable output" is what belongs there. Debug should be maximally clear and helpful IMO.

@Zeenobit
Copy link
Contributor Author

Just a random idea I had on this topic:

If we take this approach, we could make the Debug impl compact as well. Maybe if we pick a format like: #v#[raw_bits]
So then Display could look like 12v7. And Debug could look like 12v7[12345678] or 12v7.12345678

Alternatively, we could make Display use the 12v7.12345678 format, and Debug to be even more verbose with field names as needed.

@alice-i-cecile
Copy link
Member

I like 12v7[12345678] for the display impl, and then a properly verbose Debug impl :)

@alice-i-cecile alice-i-cecile added S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Trivial Nice and easy! A great choice to get started with Bevy and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Feb 27, 2024
Zeenobit added a commit to Zeenobit/bevy that referenced this issue Mar 14, 2024
- Derive `Debug` impl for `Entity`
- Add impl `Display` for `Entity`
- Add `entity_display` display to check the output contains all required info
github-merge-queue bot pushed a commit that referenced this issue Mar 14, 2024
…ty (#12469)

# Objective

Fixes #12139 

## Solution

- Derive `Debug` impl for `Entity`
- Add impl `Display` for `Entity`
- Add `entity_display` test to check the output contains all required
info

I decided to go with `0v0|1234` format as opposed to the `0v0[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 `|1234` would help the entity IDs stand out
more.

Additionally, I'm a little concerned that this change is gonna break
existing logging for projects because `Debug` is now going to be a
multi-line output. But maybe this is ok.

We could implement `Debug` to be a single-line output, but then I don't
see why it would be different from `Display` at all.

@alice-i-cecile Let me know if we'd like to make any changes based on
these points.
@Zeenobit
Copy link
Contributor Author

Zeenobit commented Jul 22, 2024

@alice-i-cecile I want to re-open this issue. I don't think the fmt::Debug and fmt::Display implementation we proposed in the discussions here actually solves the initial problem. Additionally, it introduces a new issue that I was originally concerned about.

The Problem

The serialized output of Bevy scenes uses the raw bit representation of entities, i.e. 4294967303.
Many systems, including internal Bevy code, currently log entities using fmt::Debug (i.e. {:?}).
This is because previously, fmt::Debug has format #v#, which was short and concise, but didn't include the raw bits.

After this change, the fmt::Debug implementation is now derived. This means now any log that uses {:?} now displays:
Entity Entity { index: 16, generation: 3 } does not exist
Unless we retroactively fix existing log outputs, this is currently producing very ugly logs, not just for Bevy, but for everyone downstream!

Additionally, this still doesn't fix the initial problem, because fmt::Debug still doesn't include the raw bits.
I can't easily map Entity { index: 16, generation: 3 } to 4294967303.

My Proposal

To me, Entity should not have a fmt::Display implementation.
99.9% of the time an entity is only being displaying for debug purposes.
Because of this, I propose fmt::Debug should be what fmt::Display is now, in addition to the raw bits: i.e. #v#[rawbits]

This not only avoids the need to retroactively fix logs, but also solves the initial problem by including the raw bits into the debug output.

Any specialized debugger that needs to show additional data for the entity most likely would need to format it explicitly. Trying to implement fmt::Display as a "pretty debug" output is not something I believe Bevy should handle.


The only other alternative I see around this issue is that we retroactively fix existing log locations to use {} instead of {:?} and include the raw bits where appropriate. To me, this is a much more intrusive change.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! labels Jul 22, 2024
Zeenobit added a commit to Zeenobit/bevy that referenced this issue Jul 30, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 31, 2024
# Objective

Fixes #12139

## Solution

See this comment on original issue for my proposal:
#12139 (comment)

This PR is an implementation of this proposal.

I modified the implementation of `fmt::Debug` to instead display
`0v0#12345` to ensure entity index, generation, and raw bits are all
present in the output for debug purposes while still keeping log message
concise.

`fmt::Display` remains as is (`0v0`) to offer an even shorter output.

To me, this is the most non-intrusive fix for this issue.

## Testing

Add `fn entity_debug` test

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Networking Sending data between clients, servers and machines A-Scenes Serialized ECS data stored on the disk C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
2 participants