Allow Custom Implementations of ReflectComponent

[zicklag]

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

While working on a scripting interface to Bevy I've run into the use-case to provide a custom implementation of ReflectComponent for a custom Reflect type.

Essentially the custom Reflect type is a JsComponent type that could represent any number of different ComponentIds, though they are all backed by the same Rust type. The JsComponent type stores the ComponentId of the component that it represents internally, and this requires a custom implementation of ReflectComponent to allow the component to be inserted with the proper component ID when adding it to the Bevy world.

Unfortunately, I can't make a custom implementation of ReflectComponent, because it's fields are private.

What solution would you like?

This is largely a question, but do you think it would make sense to make the fields of ReflectComponent public, to allow custom implementations?

What alternative(s) have you considered?

The alternative I can imagine is that I just create a CustomReflectComponent type data that I check for instead of ReflectComponent, but that is more intrusive. In my current use-case, that requires hard-coding support for my custom component type into the bevy_mod_js_scripting scripting interface, instead of having my custom component transparently supported along with all the other components, simply by overriding the ReflectComponent implementation.

Additional context

None.

[zicklag]

@MrGVSV and @jakobhellermann, pinging you just in case you have thoughts.

[alice-i-cecile]

I'd be fine to either make these fields public, or add a constructor method. I suspect the latter will be safer, as it avoids accidentally mutating the fields of pre-existing ReflectComponent in ways that can cause UB.

[MrGVSV]

I think your use case makes sense and it feels like reflection should allow doing dangerous things like overwriting a ReflectComponent. (And I don't have enough context on bevy_scene + scripting to really suggest anything better haha.)

I agree with @alice-i-cecile that it would be best to protect those fields from unsuspecting users. A constructor would be good but there's a lot of fields to set. Maybe we should use a builder pattern? Or pass a config object?

Also would you need to do the same sort of thing for ReflectResource?

[zicklag]

I suspect the latter will be safer, as it avoids accidentally mutating the fields of pre-existing ReflectComponent in ways that can cause UB.

I can't tell immediately whether assigning to those fields could cause undefined behavior, unless of course it was the reflect_mut function which is itself unsafe. It seems like it might be fine.

Also, users of ReflectComponent will almost never have a mutable ReflectComponent anyway, because you'll almost always just call TypeRegistry::get_type_data(), which returns an immutable reference.

In other words I think it'd be highly unlikely to modify one on accident, but I'm not really against a constructor either way.

Maybe we should use a builder pattern? Or pass a config object?

If we did it, I think I'd prefer a config object. There aren't a lot of builders in the Bevy API so far, at least as far as I've used.

I guess the config object would literally be an identical struct, but with all the fields public. Then you'd pass that into a ReflectComponent::new() function, maybe.

Also would you need to do the same sort of thing for ReflectResource?

Yes, actually. I hadn't thought of resources yet, but its the same scenario.