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.

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

Composable Pipeline Specialization #17373

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

ecoskey
Copy link
Contributor

@ecoskey ecoskey commented Jan 14, 2025

Objective

Our pipeline specialization APIs are confusing and duplicated in several places.
The relationship between the several traits we have (SpecializedMeshPipeline,
etc.) and the various wrappers that implement them (MaterialPipeline,
MeshPipeline etc.) isn't very clear-cut, and adding new logic often requires a new trait or wrapper.

Solution

Add a unifying trait: Specialize, a container: Specializer, along with
a derive macro to compose specializers together.

Rather than the workflow we have now, defining several layers of wrappers, which each defer to some internal logic, new specializers are "flat" and composed of a series of objects which take in a key and transform a pipeline descriptor one-by-one. Note that this means Specialize implementations no longer produce descriptors, but transform them. This also means that we have to get a "base" descriptor from somewhere. This can either be passed in manually when creating a Specializer, or created using Specializer::from_world for Specialize implementations that also satisfy FromWorld and HasBaseDescriptor.

What this might mean for our internal logic: we could split the view bind group layout info outside of MeshPipeline, allowing us to clean things up a little there and maybe move more of our mesh stuff to bevy_mesh. We could also make MaterialPipeline no longer a wrapper, to simplify things a bit. Also, making our specialization more modular should reduce duplication for users doing any kind of custom queuing.

Testing

  • Did some simple manual testing of the derive macro, it seems robust.

Showcase

What material specialization might look like after this change:

#[derive(Specialize, HasBaseDescriptor)]
#[specialize(RenderPipeline)]
pub struct SpecializeMeshMaterial<M: Material> {
    mesh: SpecializeMesh, // set mesh bind group layout and shader defs
    view: SpecializeView, // set view bind group layout and shader defs
    #[key(default)] // since type SpecializeMaterial::Key = (), we can hide it from the wrapper's external API
    #[base_descriptor] //defer to the HasBaseDescriptor impl of SpecializeMaterial, since it carries the vertex and fragment handles
    material: SpecializeMaterial<M>, // set material bind group layout
}

//generated implementation by the derive macro:
impl <M: Material> Specialize<RenderPipeline> for SpecializeMeshMaterial<M> {
    type Key = (MeshKey, ViewKey);

    fn specialize(&self, key: Self::Key, descriptor: &mut RenderPipelineDescriptor) {
        self.mesh.specialize(key.0, descriptor);
        self.view.specialize(key.1, descriptor);
        self.material.specialize((), descriptor);
    }
}

impl <M: Material> HasBaseDescriptor<RenderPipeline> for SpecializeMeshMaterial<M> {
    fn base_descriptor(&self) -> RenderPipelineDescriptor {
        self.material.base_descriptor()
    }
}

Migration Guide

For manual implementers

SpecializedRenderPipeline -> Specialize<RenderPipeline>.
SpecializedMeshPipeline -> Specialize<RenderPipeline> but include a mesh specializer.
SpecializedComputePipeline -> Specialize<ComputePipeline>.

SpecializedXPipelines<S> -> Specializer<X, S>

For derive macro usage

Derive macro names can't have generic parameters, so to derive Specialize<RenderPipeline> use #[derive(Specialize)] and #[specialize(RenderPipeline)].
To derive Specialize<RenderPipeline> use #[derive(Specialize)] and #[specialize(RenderPipeline)]. #[specialize(..targets)] can take multiple specialize targets, or use #[specialize(all)] to generate a fully generic implementation, at the cost of slightly worse error messages when the trait bounds aren't satisfied.

For Followup

  • optionally prehash sub-keys
  • Regarding SpecializedMeshPipeline, see if we want to search for "compatible" keys rather than the same key (how to do so quickly?)

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 14, 2025
still dying because of HashMap. Why?
@ecoskey ecoskey closed this Jan 17, 2025
@ecoskey ecoskey deleted the new_specialize branch January 17, 2025 01:19
@ecoskey ecoskey restored the new_specialize branch January 17, 2025 01:19
@ecoskey ecoskey reopened this Jan 17, 2025
@ecoskey ecoskey added the D-Macros Code that generates Rust code label Jan 19, 2025
Ok(field_info)
}

fn get_struct_fields<'a>(ast: &'a DeriveInput, derive_name: &str) -> syn::Result<&'a Fields> {
Copy link
Contributor

Choose a reason for hiding this comment

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

u can use bevy_macro_utils::get_struct_fields. Probably also for more things here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I used a custom version originally for better error messages, but I should probably just upstream those changes to get_struct_fields

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Macros Code that generates Rust code D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: Respond (With Priority)
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants