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

Reflect derived traits on all components and resources #15187

Closed
blazepaws opened this issue Sep 13, 2024 · 6 comments · Fixed by #15230
Closed

Reflect derived traits on all components and resources #15187

blazepaws opened this issue Sep 13, 2024 · 6 comments · Fixed by #15230
Labels
A-Cross-Cutting Impacts the entire engine A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use 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!

Comments

@blazepaws
Copy link
Contributor

What problem does this solve or what need does it fill?

Some components and resources reflect their derived traits (e.g. Name), but most only derive some of them (e.g. Transform).
To me it is unexpected that I have to manually add some fundamental traits (like Default, Debug, Serialize and Deserialize) to the reflection system retroactively. This way the user can rely on the consistency of which traits are reflected.

A practical example is a plugin I'm working on. It has to serialize entities using the reflection system.
It serializes all components that have ReflectSerialize. But by default that leads to partial serializations.

What solution would you like?

I propose to add reflection to the following traits on components and resources that implement them:

  • Default
  • Debug
  • Hash
  • PartialEq
  • Serialize
  • Deserialize
  • Component (I did not find an example where this one is actually missing. It's in this list for completeness sake)

All of these are already reflected on some component.

What alternative(s) have you considered?

It is of course possible to go through the bevy code and manually register all bevy types:

app.register_type_data::<X, ReflectSerialize>().register_type_data::<X, ReflectDeserialize>();

The problems with this approach are:

  • Consistency. Some components and resources don't need to be added like this.
  • Maintenance burden on generic code when new components are introduced. Since reflection only shows up at runtime this can lead to bugs easily.

Additional context

Additional notes:

It is necessary to benchmark compile times. The macro increases compile times. If the compile times become unacceptable the set of traits added to reflection needs to be reviewed. Taking traits out of the reflection system in a consistent manner would be breaking changes as some types already make use of these.

Feature gates need to be respected (particularly bevy_reflect and serialize). At this point I still need to review which feature gates exist for which subcrates.

@blazepaws blazepaws added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 13, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Cross-Cutting Impacts the entire engine 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! and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Sep 13, 2024
@alice-i-cecile
Copy link
Member

This should be done, but is easy to forget. When tackling this, please handle it one crate or so at a time: it's helpful to avoid merge conflicts and review burden.

@blazepaws
Copy link
Contributor Author

blazepaws commented Sep 13, 2024

I'm working on this but can only get started on Sunday, so no pull request just yet.

I'm also wondering if a lint could be written to remind people to do this on future components and resources.
I'll investigate that one when I'm done with this issue.

@janhohenheim
Copy link
Member

janhohenheim commented Sep 13, 2024

FYI, there was talk of deprecating reflect(Debug), so by adding this, you may be increasing future workload. Can we skip that one? Ping @MrGVSV

Edit: leave it in, per reasoning below

@MrGVSV
Copy link
Member

MrGVSV commented Sep 13, 2024

FYI, there was talk of deprecating reflect(Debug), so by adding this, you may be increasing future workload. Can we skip that one? Ping @MrGVSV

I'm on the fence about deprecating it. I agree that generally speaking, the reflection-based Debug impl should work for most cases. However, there are many cases where a custom Debug impl might be desired. And the PR to deprecate it (#11625) uses autoref specialization magic to automatically implement it.

I'd like to see benchmarks and/or a breakdown of performance, maintainability, and support implications before committing to that idea. Or at the very least, I'd like to know that Bevy is comfortable with utilizing these quasi-specialization hacks.

Until then, I think it's perfectly fine—and probably desirable—to add reflect(Debug) to types.

github-merge-queue bot pushed a commit that referenced this issue Sep 14, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 15, 2024
@blazepaws
Copy link
Contributor Author

I went through all subcrates and fixed everything I could find. I consider this done.

@janhohenheim
Copy link
Member

@blazepaws much appreciated, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use 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!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants