[Merged by Bors] - bevy_reflect: Improve debug formatting for reflected types by MrGVSV · Pull Request #4218 · bevyengine/bevy

MrGVSV · 2022-03-15T22:11:34Z

Objective

Debugging reflected types can be somewhat frustrating since all dyn Reflect trait objects return something like Reflect(core::option::Option<alloc::string::String>).

It would be much nicer to be able to see the actual value— or even use a custom Debug implementation.

Solution

Added Reflect::debug which allows users to customize the debug output. It sets defaults for all ReflectRef subtraits and falls back to Reflect(type_name) if no Debug implementation was registered.

To register a custom Debug impl, users can add #[reflect(Debug)] like they can with other traits.

Example

Using the following structs:

#[derive(Reflect)]
pub struct Foo {
    a: usize,
    nested: Bar,
    #[reflect(ignore)]
    _ignored: NonReflectedValue,
}

#[derive(Reflect)]
pub struct Bar {
    value: Vec2,
    tuple_value: (i32, String),
    list_value: Vec<usize>,
    // We can't determine debug formatting for Option<T> yet
    unknown_value: Option<String>,
    custom_debug: CustomDebug
}

#[derive(Reflect)]
#[reflect(Debug)]
struct CustomDebug;

impl Debug for CustomDebug {
    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
        write!(f, "This is a custom debug!")
    }
}

pub struct NonReflectedValue {
    _a: usize,
}

We can do:

let value = Foo {
  a: 1,
  _ignored: NonReflectedValue { _a: 10 },
  nested: Bar {
    value: Vec2::new(1.23, 3.21),
    tuple_value: (123, String::from("Hello")),
    list_value: vec![1, 2, 3],
    unknown_value: Some(String::from("World")),
    custom_debug: CustomDebug
  },
};
let reflected_value: &dyn Reflect = &value;
println!("{:#?}", reflected_value)

Which results in:

Foo {
  a: 2,
  nested: Bar {
    value: Vec2(
      1.23,
      3.21,
    ),
    tuple_value: (
      123,
      "Hello",
    ),
    list_value: [
      1,
      2,
      3,
    ],
    unknown_value: Reflect(core::option::Option<alloc::string::String>),
    custom_debug: This is a custom debug!,
  },
}

Notice that neither Foo nor Bar implement Debug, yet we can still deduce it. This might be a concern if we're worried about leaking internal values. If it is, we might want to consider a way to exclude fields (possibly with a #[reflect(hide)] macro) or make it purely opt in (as opposed to the default implementation automatically handled by ReflectRef subtraits).

MrGVSV · 2022-03-15T22:19:48Z

Should we wrap the debug output with Reflect(...)? 🤔

This might be helpful to differentiate between concrete types and reflected ones.

PROMETHIA-27

Looks good! I think this'll be really convenient for debugging scripts.

MrGVSV · 2022-05-13T17:41:28Z

Looks good! I think this'll be really convenient for debugging scripts.

Yeah it was really annoying not being able to debug reflected types well 😖

Also, @PROMETHIA-27 any thoughts on wrapping output with Reflect(...)? Still unsure if that's desired or not.

PROMETHIA-27 · 2022-05-13T17:46:59Z

I think that's a good idea. Seems like it could very easily cause a very confusing situation trying to compare two seemingly equivalent things and not realizing that one is reflected, or something like that.
Though, if it causes heavy nesting of Reflect() (if there are structures being debugged that are reflected, and contain reflected things, and those contain more reflected things for example) then that could get noisy and problematic.
I would keep it for now; if it turns out to be largely just noise then it can be pretty trivially removed with a future PR.

MrGVSV · 2022-05-13T20:28:08Z

Though, if it causes heavy nesting of Reflect() (if there are structures being debugged that are reflected, and contain reflected things, and those contain more reflected things for example) then that could get noisy and problematic.

Hm 🤔

Reflect(bevy_reflect::tests::should_reflect_debug::Test {
    value: 123,
    list: Reflect([
        "A",
        "B",
        "C",
    ]),
    array: Reflect([
        1.0,
        2.0,
        3.0,
    ]),
    map: Reflect({
        123: 1.23,
    }),
    a_struct: Reflect(bevy_reflect::tests::should_reflect_debug::SomeStruct {
        foo: "A Struct!",
    }),
    a_tuple_struct: Reflect(bevy_reflect::tests::should_reflect_debug::SomeTupleStruct(
        "A Tuple Struct!",
    )),
    custom: Cool debug!,
    unknown: Reflect(Reflect(core::option::Option<alloc::string::String>)),
})

It doesn't seem too bad.

One awkward part is the Reflect(Reflect(...)) which happens for unknown/not-debuggable types. I can remove the inner Reflect(...) but then we lose that differentiator.

Not too sure what to do about that. We could leave the PR as is, allow the Reflect(Reflect(...)) syntax, remove the inner Reflect(...), or create a different differentiator for "unknown" types.

MrGVSV · 2022-05-13T21:11:13Z

We could maybe make Reflect::debug() instead return &dyn Debug? Then the Debug impl for Reflect could be the one to add the Reflect(...) wrapper. This would result in only the top level struct/list/etc. being wrapped, while most other types contained are not wrapped (except for the "unknown" types). But this means that each type or other implementor will need to define their own debug type (e.g. StructDebug, ListDebug, etc.).

If anyone else has any other thoughts on this whole Reflect(...) formatting thing, that would be very helpful in deciding which path to take 😅

# Objective Quick followup to #4712. While updating some [other PRs](#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations

dyc3

Definitely seems nice to have. Looks like there are some merge conflicts, though.

MrGVSV · 2022-05-21T20:38:41Z

Definitely seems nice to have. Looks like there are some merge conflicts, though.

Conflicts resolved!

…e#4739) # Objective Quick followup to bevyengine#4712. While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations

alice-i-cecile

bors r+

alice-i-cecile · 2022-05-30T16:06:46Z

This is very nice; simple useful functionality implemented in the obvious way.

bors · 2022-05-30T16:08:45Z

Canceled.

alice-i-cecile · 2022-05-30T16:28:06Z

bors r+

# Objective Debugging reflected types can be somewhat frustrating since all `dyn Reflect` trait objects return something like `Reflect(core::option::Option<alloc::string::String>)`. It would be much nicer to be able to see the actual value— or even use a custom `Debug` implementation. ## Solution Added `Reflect::debug` which allows users to customize the debug output. It sets defaults for all `ReflectRef` subtraits and falls back to `Reflect(type_name)` if no `Debug` implementation was registered. To register a custom `Debug` impl, users can add `#[reflect(Debug)]` like they can with other traits. ### Example Using the following structs: ```rust #[derive(Reflect)] pub struct Foo { a: usize, nested: Bar, #[reflect(ignore)] _ignored: NonReflectedValue, } #[derive(Reflect)] pub struct Bar { value: Vec2, tuple_value: (i32, String), list_value: Vec<usize>, // We can't determine debug formatting for Option<T> yet unknown_value: Option<String>, custom_debug: CustomDebug } #[derive(Reflect)] #[reflect(Debug)] struct CustomDebug; impl Debug for CustomDebug { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "This is a custom debug!") } } pub struct NonReflectedValue { _a: usize, } ``` We can do: ```rust let value = Foo { a: 1, _ignored: NonReflectedValue { _a: 10 }, nested: Bar { value: Vec2::new(1.23, 3.21), tuple_value: (123, String::from("Hello")), list_value: vec![1, 2, 3], unknown_value: Some(String::from("World")), custom_debug: CustomDebug }, }; let reflected_value: &dyn Reflect = &value; println!("{:#?}", reflected_value) ``` Which results in: ```rust Foo { a: 2, nested: Bar { value: Vec2( 1.23, 3.21, ), tuple_value: ( 123, "Hello", ), list_value: [ 1, 2, 3, ], unknown_value: Reflect(core::option::Option<alloc::string::String>), custom_debug: This is a custom debug!, }, } ``` Notice that neither `Foo` nor `Bar` implement `Debug`, yet we can still deduce it. This might be a concern if we're worried about leaking internal values. If it is, we might want to consider a way to exclude fields (possibly with a `#[reflect(hide)]` macro) or make it purely opt in (as opposed to the default implementation automatically handled by ReflectRef subtraits). Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

bors · 2022-05-30T16:59:28Z

Pull request successfully merged into main.

Build succeeded:

…e#4218) # Objective Debugging reflected types can be somewhat frustrating since all `dyn Reflect` trait objects return something like `Reflect(core::option::Option<alloc::string::String>)`. It would be much nicer to be able to see the actual value— or even use a custom `Debug` implementation. ## Solution Added `Reflect::debug` which allows users to customize the debug output. It sets defaults for all `ReflectRef` subtraits and falls back to `Reflect(type_name)` if no `Debug` implementation was registered. To register a custom `Debug` impl, users can add `#[reflect(Debug)]` like they can with other traits. ### Example Using the following structs: ```rust #[derive(Reflect)] pub struct Foo { a: usize, nested: Bar, #[reflect(ignore)] _ignored: NonReflectedValue, } #[derive(Reflect)] pub struct Bar { value: Vec2, tuple_value: (i32, String), list_value: Vec<usize>, // We can't determine debug formatting for Option<T> yet unknown_value: Option<String>, custom_debug: CustomDebug } #[derive(Reflect)] #[reflect(Debug)] struct CustomDebug; impl Debug for CustomDebug { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "This is a custom debug!") } } pub struct NonReflectedValue { _a: usize, } ``` We can do: ```rust let value = Foo { a: 1, _ignored: NonReflectedValue { _a: 10 }, nested: Bar { value: Vec2::new(1.23, 3.21), tuple_value: (123, String::from("Hello")), list_value: vec![1, 2, 3], unknown_value: Some(String::from("World")), custom_debug: CustomDebug }, }; let reflected_value: &dyn Reflect = &value; println!("{:#?}", reflected_value) ``` Which results in: ```rust Foo { a: 2, nested: Bar { value: Vec2( 1.23, 3.21, ), tuple_value: ( 123, "Hello", ), list_value: [ 1, 2, 3, ], unknown_value: Reflect(core::option::Option<alloc::string::String>), custom_debug: This is a custom debug!, }, } ``` Notice that neither `Foo` nor `Bar` implement `Debug`, yet we can still deduce it. This might be a concern if we're worried about leaking internal values. If it is, we might want to consider a way to exclude fields (possibly with a `#[reflect(hide)]` macro) or make it purely opt in (as opposed to the default implementation automatically handled by ReflectRef subtraits). Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

…e#4739) # Objective Quick followup to bevyengine#4712. While updating some [other PRs](bevyengine#4218), I realized the `ReflectTraits` struct could be improved. The issue with the current implementation is that `ReflectTraits::get_xxx_impl(...)` returns just the _logic_ to the corresponding `Reflect` trait method, rather than the entire function. This makes it slightly more annoying to manage since the variable names need to be consistent across files. For example, `get_partial_eq_impl` uses a `value` variable. But the name "value" isn't defined in the `get_partial_eq_impl` method, it's defined in three other methods in a completely separate file. It's not likely to cause any bugs if we keep it as it is since differing variable names will probably just result in a compile error (except in very particular cases). But it would be useful to someone who wanted to edit/add/remove a method. ## Solution Made `get_hash_impl`, `get_partial_eq_impl` and `get_serialize_impl` return the entire method implementation for `reflect_hash`, `reflect_partial_eq`, and `serializable`, respectively. As a result of this, those three `Reflect` methods were also given default implementations. This was fairly simple to do since all three could just be made to return `None`. --- ## Changelog * Small cleanup/refactor to `ReflectTraits` in `bevy_reflect_derive` * Gave `Reflect::reflect_hash`, `Reflect::reflect_partial_eq`, and `Reflect::serializable` default implementations

…e#4218) # Objective Debugging reflected types can be somewhat frustrating since all `dyn Reflect` trait objects return something like `Reflect(core::option::Option<alloc::string::String>)`. It would be much nicer to be able to see the actual value— or even use a custom `Debug` implementation. ## Solution Added `Reflect::debug` which allows users to customize the debug output. It sets defaults for all `ReflectRef` subtraits and falls back to `Reflect(type_name)` if no `Debug` implementation was registered. To register a custom `Debug` impl, users can add `#[reflect(Debug)]` like they can with other traits. ### Example Using the following structs: ```rust #[derive(Reflect)] pub struct Foo { a: usize, nested: Bar, #[reflect(ignore)] _ignored: NonReflectedValue, } #[derive(Reflect)] pub struct Bar { value: Vec2, tuple_value: (i32, String), list_value: Vec<usize>, // We can't determine debug formatting for Option<T> yet unknown_value: Option<String>, custom_debug: CustomDebug } #[derive(Reflect)] #[reflect(Debug)] struct CustomDebug; impl Debug for CustomDebug { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "This is a custom debug!") } } pub struct NonReflectedValue { _a: usize, } ``` We can do: ```rust let value = Foo { a: 1, _ignored: NonReflectedValue { _a: 10 }, nested: Bar { value: Vec2::new(1.23, 3.21), tuple_value: (123, String::from("Hello")), list_value: vec![1, 2, 3], unknown_value: Some(String::from("World")), custom_debug: CustomDebug }, }; let reflected_value: &dyn Reflect = &value; println!("{:#?}", reflected_value) ``` Which results in: ```rust Foo { a: 2, nested: Bar { value: Vec2( 1.23, 3.21, ), tuple_value: ( 123, "Hello", ), list_value: [ 1, 2, 3, ], unknown_value: Reflect(core::option::Option<alloc::string::String>), custom_debug: This is a custom debug!, }, } ``` Notice that neither `Foo` nor `Bar` implement `Debug`, yet we can still deduce it. This might be a concern if we're worried about leaking internal values. If it is, we might want to consider a way to exclude fields (possibly with a `#[reflect(hide)]` macro) or make it purely opt in (as opposed to the default implementation automatically handled by ReflectRef subtraits). Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 15, 2022

alice-i-cecile added A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Mar 15, 2022

MrGVSV changed the title ~~Improve debug formatting for reflected types~~ bevy_reflect: Improve debug formatting for reflected types Mar 28, 2022

MrGVSV added 5 commits May 12, 2022 21:29

Improved debug for reflected types

a30e403

Fixed doc example

24af17f

Add reflected debug test

4998d85

Fix build warnings

3526fed

Fix doc wording

6b28864

MrGVSV force-pushed the reflect-debug branch from 8c877e6 to 6b28864 Compare May 13, 2022 05:01

MrGVSV mentioned this pull request May 13, 2022

[Merged by Bors] - bevy_reflect: Small refactor and default Reflect methods #4739

Closed

MrGVSV added 2 commits May 12, 2022 22:31

Fix doc test

eb27aea

Fix more CI errors

fa929b7

PROMETHIA-27 approved these changes May 13, 2022

View reviewed changes

dyc3 approved these changes May 21, 2022

View reviewed changes

Merge branch 'main' into reflect-debug

81716cf

alice-i-cecile approved these changes May 30, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 30, 2022

Fix doc comment

4a260e3

bors bot changed the title ~~bevy_reflect: Improve debug formatting for reflected types~~ [Merged by Bors] - bevy_reflect: Improve debug formatting for reflected types May 30, 2022

bors bot closed this May 30, 2022

Uh oh!

Conversation

MrGVSV commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Example

Uh oh!

MrGVSV commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented May 13, 2022

Uh oh!

PROMETHIA-27 commented May 13, 2022

Uh oh!

MrGVSV commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrGVSV commented May 13, 2022

Uh oh!

dyc3 left a comment

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented May 21, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented May 30, 2022

Uh oh!

bors bot commented May 30, 2022

Uh oh!

alice-i-cecile commented May 30, 2022

Uh oh!

bors bot commented May 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MrGVSV commented Mar 15, 2022 •

edited

Loading

MrGVSV commented Mar 15, 2022 •

edited

Loading

MrGVSV commented May 13, 2022 •

edited

Loading