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

Opt-out component reflection #2968

Open
alice-i-cecile opened this issue Oct 15, 2021 · 4 comments
Open

Opt-out component reflection #2968

alice-i-cecile opened this issue Oct 15, 2021 · 4 comments
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged

Comments

@alice-i-cecile
Copy link
Member

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

Currently, registering components for reflection is tedious and error-prone.

This makes use cases where you want to serialize many different component types (such as game saving, scenes or networking) very frustrating to use.

As you can see in the reflection example, users must manually register every type they wish to reflect.

What solution would you like?

Now that #2254 is in place, make Reflect an additional trait bound on Component. Automatically derive Reflect for components, unless there is an existing manual impl for them.

When components are first added to the app, automatically register them.

What alternative(s) have you considered?

Other strategies for automatically registering components may exist, and may allow us to create a more complete list before the app begins (rather than causing spiky work during the game).

Some components may not have any validly reflected values; perhaps these should simply not implement Reflect?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk labels Oct 15, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Oct 15, 2021

unless there is an existing manual implementation for them

I don't think this is quite the logic we want; we both can't detect that, and don't want to require that every single component implements Reflect

Instead, I think it should be opt out with an attribute at the derive site.

Additionally, a better way (imo) to handle this registration would be to add a method to Component which returns Option<TypeRegistration> or whatever data we need for reflection

This does also raise an interesting question about how coupled bevy_ecs and bevy_reflect would be; in both of these worlds, you couldn't use bevy_ecs without reflection, although they are otherwise conceptually unlinked.

We could avoid this by making the method on component just be a generic fn register (&mut World) hook, but that feels prone to panicing, e.g. if registering systems before DefaultPlugins is added.

@alice-i-cecile
Copy link
Member Author

don't want to require that every single component implements Reflect

I think this deserves a bit more investigation. There are a lot of interesting and non-obvious use cases enabled by reflection. Many of those will both be more ergonomic and faster (no branching!) if we have strong guarantees.

However, the existing reflection is too frustrating and limited to really have a sense of how widely we can use it.

We could avoid this by making the method on component just be a generic fn register (&mut World) hook, but that feels prone to panicing, e.g. if registering systems before DefaultPlugins is added.

I think that we could bypass that particular panic with a more structured approach: see #1255 and bevyengine/rfcs#33.

Decoupling plus a generic register hook is interesting 🤔 I agree with your arguments about coupling, assuming that we don't want to require Reflect for all components. And even in that case, it's nice for standalone users of bevy_ecs.

@DJMcNab
Copy link
Member

DJMcNab commented Oct 15, 2021

By don't want to require that every component implements reflect, I mean in terms of components which inherently cannot implement reflect, such as bevy_winit's WinitWindowId for example

(We'd expect the window to get rehydrated based on the fact that it has a WindowProperties component)

I hate calling back to the windows as entities examples when I haven't gotten around to implementing it
But I really like the idea of it.

@alice-i-cecile
Copy link
Member Author

By don't want to require that every component implements reflect, I mean in terms of components which inherently cannot implement reflect, such as bevy_winit's WinitWindowId for example

So for this, we already have ignorable fields in reflection. Wouldn't this just be the case of a reflected component with no non-ignored fields?

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Dec 12, 2021
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-Reflection Runtime information about types A-Scenes Serialized ECS data stored on the disk C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
Status: Open
Development

No branches or pull requests

2 participants