-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Type erased materials #19667
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
Type erased materials #19667
Conversation
Material
trait bound from all render world systems and resources
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
Material
trait bound from all render world systems and resources@@ -566,7 +566,7 @@ ron = "0.10" | |||
flate2 = "1.0" | |||
serde = { version = "1", features = ["derive"] } | |||
serde_json = "1.0.140" | |||
bytemuck = "1.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the way that we're erasing material key (aka bind group data) in this PR at the moment is by going through bytemuck (see MaterialProperties
where it's stored as a SmallVec
), which basically puts it in our public api.
I'm somewhat uncertain about this change as it enforces new constraints on AsBindGroup::Data
, namely the bytemuck derives and repr c. On the one hand, our use of bind group data internal to the engine is always in the form of flags and so this just makes that explicit. On the other hand, if people want to have weird material keys, maybe it isn't our place to stop them.
The alternative is doing Box<dyn Any>
, but I ran into problems with making that hashable. The key bit here is that while the erased key only needs to be downcast to the concrete material key in the event you are respecializing, it needs to be hashable for the internal pipeline cache check. While actual respecialization is rare, the cache check happens any time a mesh/material changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand how this relates to changing the bytemuck version 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry! It needs to match the version used in bevy_pbr
. If we wanted to add that constraint, we should probably re-export it.
I'm not either at all, but having a bunch of fields was gross. Part of the goal here too is to be able to re-use this for 2d, and so it would be ideal to keep it somewhat general. I have a version of this PR where I convert everything to entities and components and it's nice and clean but results in a lot more changes. I think we can get there but I wanted to do this as an incremental step. I'd be happy to remove the labels since ideally we remove them before 0.17 but idk, having 27 shader fields wasn't great either. |
crates/bevy_pbr/src/material.rs
Outdated
); | ||
|
||
#[cfg(feature = "meshlet")] | ||
render_app.add_systems( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that these don't depend on material generics, lets move them into the meshlet plugin.
( | ||
check_views_lights_need_specialization.in_set(RenderSystems::PrepareAssets), | ||
// specialize_shadows also needs to run after prepare_assets::<PreparedMaterial>, | ||
// which is fine since ManageViews is after PrepareAssets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will ManageViews always be after PrepareAssets or might something change it down the line? i think a redundant edge for defensiveness doesnt have overhead, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so cool. so good
}) | ||
} | ||
|
||
fn bind_group_data(&self) -> Self::Data {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor thing, but if it's valid to not return anything this should probably just the default behaviour of the trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a couple of things that could still be simplified, but this is a good starting point and we can iterate on everything else. I'm very excited for what kind of features this will unlock for us!
LGTM to me
Objective
Closes #18075
In order to enable a number of patterns for dynamic materials in the engine, it's necessary to decouple the renderer from the
Material
trait.This opens the possibility for:
AsBindGroup
.Solution
In short, remove all trait bounds from render world material systems and resources. This means moving a bunch of stuff onto
MaterialProperties
and engaging in some hacks to make specialization work. Rather than storing the bind group data inMaterialBindGroupAllocator
, right now we're storing it in a closure onMaterialProperties
. TBD if this has bad performance characteristics.Benchmarks
many_cubes
:cargo run --example many_cubes --release --features=bevy/trace_tracy -- --vary-material-data-per-instance
:@DGriffin91's Caldera

cargo run --release --features=bevy/trace_tracy -- --random-materials
@DGriffin91's Caldera with 20 unique material types (i.e.

MaterialPlugin<M>
) and random materials per meshcargo run --release --features=bevy/trace_tracy -- --random-materials
TODO
Fix meshletsShowcase
See the example for a custom material implemented without the use of the
Material
trait and thusAsBindGroup
.