More fully type erase all pbr types from specialization #22408
Conversation
tychedelia
added
A-Rendering
| type_id: TypeId, | ||
| } | ||
|
|
||
| impl ErasedMeshPipelineKey { |
There was a problem hiding this comment.
This is not sound:
let me_peek_uninit_bytes = format!(
"{:?}",
ErasedMeshPipelineKey::new(MaybeUninit::<u64>::uninit())
);miri says:
error: Undefined Behavior: calling a function with calling convention "Rust" using calling convention "C"
|
= note: Undefined Behavior occurred here
= note: (no span available)
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
We either need more bounds on T, or make this type not pub, or to have useless Debug and no Hash/PartialEq impls. (PartialEq and Hash are also unsound by this same principle)
As far as i know, theres no trait that enforces T to actually be all init: MaybeUninit::uninit() is an init value, because MaybeUninit doesnt have to have init bytes to be init. Theres also padding issues to account for.
This is also UB:
ErasedMeshPipelineKey::new(0u64).downcast::<&u64>();(I dont know why, but miri doesnt hit the type_id assert, it just hits UB)
Here's a fix to make it safe and sound: tychedelia#5
There was a problem hiding this comment.
Okay yeah, thanks for checking this. The From impl makes more sense anyway. Tbh this probably could be dyn Any just for maximal flexibility as we only hit it on the slow path. We'd just have to do the same vtable shenanigans that we do for the user key.
|
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. |
|
Fixes CI and pixel eagle: tychedelia#6 |
| pub user_specialize: Option< | ||
| fn( | ||
| &MaterialPipeline, | ||
| &dyn Any, |
There was a problem hiding this comment.
this is exactly what i wanted but couldnt figure out how to make the implementation work yay
| continue; | ||
| }; | ||
| #[derive(SystemParam)] | ||
| pub(crate) struct SpecializePrepassSystemParam<'w, 's> { |
There was a problem hiding this comment.
this is so much nicer extracted into a sys param
| } | ||
|
|
||
| /// Common [`Material`] properties, calculated for a specific material instance. | ||
| #[derive(Default)] |
There was a problem hiding this comment.
Lets try to keep this Default, can we make base_specialize: Option<BaseSpecializeFn>?
| pub specialize: Option< | ||
| pub base_specialize: Option<BaseSpecializeFn>, | ||
| pub prepass_specialize: Option<PrepassSpecializeFn>, | ||
| pub user_specialize: Option< |
There was a problem hiding this comment.
Not sure if this is intentional, but tychedelia#8 to add this as a type
4 participants
We remove the
M: Materialtrait bound in #19667, however we still had an implicit dependency on pbr viaMeshPipelineKeyand the concrete specializer type that prevents other rendering paths (i.e. 2d) from using our specialization infrastructure.Here we more completely erase pbr specifics in
MaterialPropertiesby the following pattern:specialize_*a world exclusive system.&mut Worldand can look up it's own specializer.I think there's more opportunity here to make this more ECS-y in the future by using entities to reference the specializer or something similar. But that's an exercise left for later once we work out what additional refactors may be needed to get 2d to work.
Tested a few scenes to confirm things still work.