Ensure ExtendedMaterial works with reflection (to enable bevy_egui_inspector integration)

[Braymatter]

Objective

• Ensure ExtendedMaterial can be referenced in bevy_egui_inspector correctly

Solution

Add a more manual TypePath implementation to work around bugs in the derive macro.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Why FromReflect bounds here rather than Reflect?

[Braymatter]

Not an expert, or even novice on bevy_reflect, but according to the compiler FromReflect seems to be required by TypePath. I tried with Reflect initially, and Reflect + TypePath bounds just now and FromReflect seems to be the thing that works.

[MrGVSV]

Interesting. What's the error you're getting if you just add TypePath as a bound?

My thought is that we ideally don't want to force reflection on types that don't want or need it (i.e. allow the user to define a B and E that are not strictly Reflect).

[Braymatter]

I think we need reflect on this otherwise we're going to need to do a weird wrapper pattern for any custom material in order to inspect it at runtime.

StandardMaterial already implements reflect, and there's an immediate use for it in the form of runtime inspection, which is really important for our debugging process.

[MrGVSV]

What's the code that's causing this error? I ask because I'm wondering if it's an issue with how we're using it rather than how the type is defined. Like, if we're registering concrete types, then there probably shouldn't be an issue. But if we're trying to make the system/plugin generic, then we probably do need these bounds added.

[Braymatter]

Just a

register_type::<ExtendedMaterial<StandardMaterial,MyCustomMaterial>>>()

[MrGVSV]

I think I found the problem: Asset requires TypePath, but the TypePath impl generated by the Reflect derive has additional FromReflect bounds on the generic type parameters, which means that TypePath is only implemented where ExtendedMaterial<B, M> is Reflect.

The fix would be to opt-out of Reflect's automatic TypePath implementation and derive it separately:

#[derive(Asset, Clone, Reflect, TypePath)]
#[reflect(type_path = false)]
pub struct ExtendedMaterial<B: Material, E: MaterialExtension> {
    pub base: B,
    pub extension: E,
}

However, this apparently reveals a bug with the TypePath derive. Since it reuses some of the Reflect derive logic, it actually picks up the #[reflect(type_path = false)] attribute and just doesn't generate anything 😅.

As a temporary workaround so this isn't blocked, I recommend doing something like this:

#[derive(Asset, Clone, Reflect)]
#[reflect(type_path = false)]
pub struct ExtendedMaterial<B: Material, E: MaterialExtension> {
    pub base: B,
    pub extension: E,
}

// We don't use the `TypePath` derive here due to a bug where `#[reflect(type_path = false)]`
// causes the `TypePath` derive to not generate an implementation.
impl_type_path!((in bevy_pbr::extended_material) ExtendedMaterial<B: Material, E: MaterialExtension>);

The above code should compile and not be a breaking change!

[Braymatter]

Oh nice! Would love for this to make it in for .12.1 -- I just pushed the suggested change

[alice-i-cecile]

@Braymatter can you update your PR title and description to match the new changes?

[Braymatter]

Suggested changes made, since this is no longer breaking is it possible to get it into 12.1? @alice-i-cecile

[alice-i-cecile]

Yep, this is now 0.12.1 compatible :D It's in the milestone, so Cart will make the final call on cherrypicking it when he prepares the release.

[MrGVSV]

I would update the Migration Guide and PR description btw