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

Advocate for Debug implementation of Res/ResMut #2048

Closed
CGMossa opened this issue Apr 29, 2021 · 8 comments
Closed

Advocate for Debug implementation of Res/ResMut #2048

CGMossa opened this issue Apr 29, 2021 · 8 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@CGMossa
Copy link
Contributor

CGMossa commented Apr 29, 2021

I would like to start a small discussion over adding Debug for Res and ResMut.

What are the thoughts and ideas around this? I'd gladly start a PR on this, if that's
a welcome idea.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-ECS Entities, components, systems, and events labels Apr 29, 2021
@alice-i-cecile
Copy link
Member

Related to #1130.

From what I've gathered, we'd love to be able to support this, but we need to make sure it's painless. This would be particularly useful for the editor.

Not everything we wrap in a Res will have Debug, which means that we need an implementation that has a sane fallback for when that's not the case.

As a workaround for now, you should be able to unwrap the Res using dbg!(*my_res) and debug the underlying data.

@Ratysz
Copy link
Contributor

Ratysz commented Apr 29, 2021

For what it's worth, we can have blanket impl<T: Debug> Debug for Res<T> and impl<T: Debug> Debug for ResMut<T>. This is a partial solution: there is no fallback, so if we wanted to rely on it (in the editor, etc) we'd have to drag that bound everywhere, potentially into user code. But it would simplify prototyping in cases where the type is known.

I'm not sure there is a way to have a fallback without negative trait bounds and/or specialization.

@CGMossa
Copy link
Contributor Author

CGMossa commented Apr 29, 2021

I'm mostly advocating for impl<T: Debug> Debug for Res<T> as I think it is going to be very messy to make something that is also going to fit the contraints in the editor. But I'm not experienced enough here. This might be a longer discussion than I initially thought it would be. As I'm not sure you can have the blanket-impl also able to be overridden later on without it causing a huge mess (if it is at all possible to then impl<T> Debug for Res<T>.

@forbjok
Copy link
Contributor

forbjok commented Apr 29, 2021

I just tested, and having a blanket implementation of Debug for all Res where T implements Debug is certainly possible and seems to work fine. Maybe there are more advanced use cases that this doesn't cover, but I can't really see any downside to having this regardless.

impl<'w, T: Component> Debug for Res<'w, T>
where
    T: Debug,
{
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        Debug::fmt(self.value, f)
    }
}

@mockersf
Copy link
Member

mockersf commented Apr 29, 2021

Should the Debug of a Res mention in it's output that it's a Res, or should it be completely transparent to it's inner type?

I think Res<T> and T should not have the same Debug output.
It would be useful in debug logs to be able to differentiate between a Res and a ResMut for example.

@alice-i-cecile
Copy link
Member

@forbjok I agree, this won't be a complete solution but it certainly seems harmless to include (especially with the suggestion by @mockersf). We'll want to make sure we hit Local, NonSend and NonSendMut as well.

Could we do the same thing for components? I know that we have a Mut wrapper at least for them.

@forbjok
Copy link
Contributor

forbjok commented Apr 29, 2021

I made a pull request adding implementations for Res and ResMut.

bors bot pushed a commit that referenced this issue May 1, 2021
This commit adds blanket implementations of Debug for Res and ResMut, as discussed in #2048.
@NathanSWard
Copy link
Contributor

This issue should've been close by #2050 😄

ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
This commit adds blanket implementations of Debug for Res and ResMut, as discussed in bevyengine#2048.
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

No branches or pull requests

6 participants