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.

Sign up for GitHub

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

Ensure ExtendedMaterial works with reflection (to enable bevy_egui_inspector integration) #10548

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Ensure ExtendedMaterial works with reflection (to enable bevy_egui_inspector integration) #10548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f17f5b5
feature: Added reflect type bounds to ExtendedMaterial for bevy_egui_…
Braymatter Nov 14, 2023
7093067
chore: cargo fmt
Braymatter Nov 14, 2023
d836023
fix: Use Reflect instead of additional traitbounds
Braymatter Nov 14, 2023
90c0bcd
cleanup: Remove unnecessary trait bounds
Braymatter Nov 14, 2023
7daef50
chore: fmt
Braymatter Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/bevy_pbr/src/extended_material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use bevy_asset::{Asset, Handle};
use bevy_reflect::TypePath;
use bevy_reflect::{Reflect, FromReflect};
use bevy_render::{
mesh::MeshVertexBufferLayout,
render_asset::RenderAssets,
Expand Down Expand Up @@ -97,13 +97,13 @@ pub trait MaterialExtension: Asset + AsBindGroup + Clone + Sized {
/// When used with `StandardMaterial` as the base, all the standard material fields are
/// present, so the `pbr_fragment` shader functions can be called from the extension shader (see
/// the `extended_material` example).
#[derive(Asset, Clone, TypePath)]
pub struct ExtendedMaterial<B: Material, E: MaterialExtension> {
#[derive(Asset, Clone, Reflect)]
pub struct ExtendedMaterial<B: Material + FromReflect, E: MaterialExtension + FromReflect> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why FromReflect bounds here rather than Reflect?

Copy link
Contributor Author

@Braymatter Braymatter Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Braymatter Braymatter Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Braymatter Braymatter Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a

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

Copy link
Member

@MrGVSV MrGVSV Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub base: B,
pub extension: E,
}

impl<B: Material, E: MaterialExtension> AsBindGroup for ExtendedMaterial<B, E> {
impl<B: Material + FromReflect, E: MaterialExtension + FromReflect> AsBindGroup for ExtendedMaterial<B, E> {
type Data = (<B as AsBindGroup>::Data, <E as AsBindGroup>::Data);

fn unprepared_bind_group(
Expand Down Expand Up @@ -148,7 +148,7 @@ impl<B: Material, E: MaterialExtension> AsBindGroup for ExtendedMaterial<B, E> {
}
}

impl<B: Material, E: MaterialExtension> Material for ExtendedMaterial<B, E> {
impl<B: Material + FromReflect, E: MaterialExtension + FromReflect> Material for ExtendedMaterial<B, E> {
fn vertex_shader() -> bevy_render::render_resource::ShaderRef {
match E::vertex_shader() {
ShaderRef::Default => B::vertex_shader(),
Expand Down
3 changes: 1 addition & 2 deletions examples/shader/extended_material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
//! Demonstrates using a custom extension to the `StandardMaterial` to modify the results of the builtin pbr shader.

use bevy::reflect::TypePath;
use bevy::{
pbr::{ExtendedMaterial, MaterialExtension, OpaqueRendererMethod},
prelude::*,
Expand Down Expand Up @@ -73,7 +72,7 @@ fn rotate_things(mut q: Query<&mut Transform, With<Rotate>>, time: Res<Time>) {
}
}

#[derive(Asset, AsBindGroup, TypePath, Debug, Clone)]
#[derive(Asset, AsBindGroup, Reflect, Debug, Clone)]
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
struct MyExtension {
// We need to ensure that the bindings of the base material and the extension do not conflict,
// so we start from binding slot 100, leaving slots 0-99 for the base material.
Expand Down
Loading