Pipeline keys #10333

robtfm · 2023-11-01T15:23:18Z

Objective

address the increasing complexity and monolithicness of bitfield-based pipeline keys.

part of a general drive towards pluggability/modularity in the rendering engine, which also requires

extensible view group
a shader plugin mechanism

both of which i've laid some groundwork for but are not fully implemented yet.

Solution

introduced a `PipelineKey` macro and related traits. PipelineKey types are either:

unit enums
tuples or structs containing primitives or other PipelineKeys
dynamic keys, for which components can be registered
custom implementations on foreign types (wgpu::TextureFormat, etc)

The traits define methods to

pack into a KeyPrimitive type (currently a u128, may want to use a u64)
unpack from the KeyPrimitive type back to the source type
merge parts into composites
extract parts from composites

SystemKeys can be registered which set a key value for an entity based on query data. This allows the key value for an entity to be determined by the key itself.

ShaderDefs can be defined for pipeline keys based on the key value.

implemented usage for MeshPipeline and MaterialPipeline (including the prepass pipeline).

the contents of the current MeshPipelineKey bitfield has been broken out into

view-specific parts (msaa, hdr, prepass settings, etc) which are built by SystemKeys and attached to the view
mesh-specific parts which are calculated in prepare_assets::<Mesh>
material-specific parts which are calculated in prepare_materials

the MaterialPipeline now uses a NewMeshPipelineKey struct containing all of the above parts. the shader defs for main pass materials and prepasses are taken from the keys.

todo

[x] document everything properly
[x] collapse some keys onto the types they are based on (no need for a TonemappingKey when Tonemapping itself is essentially the same thing)
[x] at least in debug, validate bitsize vs KeyPrimitive size (added without cfg for now)

todo as follow ups

[x] other pipelines (sprites, ui, deferred etc) still use bitfields, these should be migrated
[x] legacy code to do with the old MeshPipelineKeys bitfield can then be removed
[ ] vertex attributes and defs should also be part of the mesh key, rather than handled separately
[ ] implement KeyTypeConcrete for arbitrary-sized ints (need to choose or build an implementation)
[ ] use a dynamic view key so we can make parts of the architecture optional

performance:

i haven't tested much. i saw a 2% perf regression running many foxes, but i always struggle to get consistent benchmarks. but once all the work is done i would expect performance benefits from making render-feature plugins optional, and eliminating time spent on them when not used.

Migration Guide

AsBindGroup::Data types need to derive PipelineKey, and specialize functions now take a PipelineKey<Self::Key> param, which derefs to the raw key type.

superdump · 2023-11-01T15:50:16Z

Just as a note, I want to move toward a more ‘as needed’ pipeline specialisation if it turns out to perform well enough, so that we can cache a pipeline id as a component on an entity and not update it ever unless a pipeline dependency changes. Perhaps caching the keys as components is a way of managing that.

nicopap

Guess I'll have to speed up work on my impl, because I think we can do much better.

The current issues I have with this:

We are basically reimplementing a bit packing macro, bitbybit should be able to handle the pack/unpack stuff. Maybe it is missing some features, but it's better to contribute those feature to bitbybit that maintain this massive proc macro. Or at worse, maintain a fork of bitbybit with those features we need
The naming on PipelineMetaStore is confusing. I don't understand it.
This limits every pipeline key to have 128 bits (is that right?)
I think https://gist.github.com/nicopap/6c9b351f0df8555f66e30d2ceeda9209 has a better API

nicopap · 2023-11-10T16:40:57Z

crates/bevy_render/src/pipeline_keys/composite.rs

+use bevy_utils::all_tuples;
+
+#[allow(unused_macros)]
+macro_rules! impl_composite_key_tuples {


I think you might have disregarded bitbybit a bit too quickly, as this looks very similar to the code it generates.

The big difference is that this supports dynamic keys, where components are registered at runtime.

nicopap · 2023-11-10T16:41:40Z

crates/bevy_render/src/pipeline_keys/mod.rs

+/// can be independently registered to allow extensibility.
+
+/// The primitive type underlying packed pipeline keys.
+pub type KeyPrimitive = u128;


So each key will have 128 bits? I recall there were concerns regarding hashing perf.

It could be 64 but with some compromises on current data - specifically the StandardMaterial data uses an i32 for depth bias which could comfortably be condensed. The view and mesh keys are around 30 bits all in

Correct me if I'm wrong, but doesn't ahash use AES instructions, which operate on XMM registers, which are 128 bits? So 128 bits should be as fast as 64 bits, in theory.

i don't know all the details, but i'm sure there'll be some hardware where it's as fast and some where it's not.

nicopap · 2023-11-11T11:22:53Z

I'm still going through the code, but I've a bunch of question. I'll have more questions later.

API Usage

How would this API be used to build a dynamic keys?

Many keys per entity

I see:

type KeyPrimitive = u128;

#[derive(Component, Default)]
pub struct PipelineKeys {
    packed_keys: HashMap<TypeId, (KeyPrimitive, u8)>,
}

What's going on here? Would what does having multiple keys per entity means? Wouldn't it be more judicious to use a generic component?

`SystemKey`

I don't understand what is SystemKey. It has nothing to do with systems right? It has more to do with specific plugins.
The PR description doesn't help:

SystemKeys can be registered which set a key value for an entity based on query data. This allows the key value for an entity to be determined by the key itself.

I think you mean "[…] for an entity to be determined by data outside of the key".

robtfm · 2023-11-11T12:40:12Z

How would this API be used to build a dynamic keys?

with a combination of:

// anywhere
#[derive(PipelineKey)]
[dynamic_key]
pub struct MyDynamicKey(KeyPrimitive);

// in plugin build
render_app
    .register_dynamic_key<MyDynamicKey, F>()
    // here or any other plugin
    .register_dynamic_key_part<MyDynamicKey, SomeOtherKey>();

this would make the dynamic key (which can be used by pipelines etc) equivalent to a struct which contains a member: SomeOtherKey member. the dynamic components can be extracted later if required, and will contribute shader defs (and later bindgroup layouts/data).
the idea here is to make things like EnvironmentMap or shadows entirely standalone and optional, with no knowledge of it required in pbr, render or anywhere else, except for registering the plugin (if we include it by default).

Many keys per entity

I see:
type KeyPrimitive = u128;

#[derive(Component, Default)]
pub struct PipelineKeys {
    packed_keys: HashMap<TypeId, (KeyPrimitive, u8)>,
}
What's going on here? Would what does having multiple keys per entity means? Wouldn't it be more judicious to use a generic component?

the idea of using a per-entity hashmap is to avoid table moves by adding many components. the multiple keys are all the individual key parts (MsaaKey, Tonemapping, etc etc), some of which are used in multiple pipeline keys (Msaa in particular is used in virtually every pipeline).

SystemKey

I don't understand what is SystemKey. It has nothing to do with systems right? It has more to do with specific plugins. The PR description doesn't help:

SystemKeys can be registered which set a key value for an entity based on query data. This allows the key value for an entity to be determined by the key itself.

I think you mean "[…] for an entity to be determined by data outside of the key".

maybe? i mean that the key gets constructed from world/query data using the function (which is almost a system) defined on the key itself, with this trait.
as a side note - i'd love to improve the api so that we could use something closer to raw systems, but i am not sure how, while still preserving the external entity iterator for the Query data. this is the same pattern we used in the GetBatchData trait.

pcwalton

This isn't a complete review, but I gave it a quick look over. My main comment is that I wonder whether the pipeline key stuff could be factored out into a separate bevy_bitfields crate or something. bevy_render is already quite large, so this would reduce compile time, and you've created a very generic bitfield crate here that might be useful in other contexts.

pcwalton · 2023-11-25T20:05:00Z

crates/bevy_render/src/pipeline_keys/mod.rs

+/// can be independently registered to allow extensibility.
+
+/// The primitive type underlying packed pipeline keys.
+pub type KeyPrimitive = u128;


Correct me if I'm wrong, but doesn't ahash use AES instructions, which operate on XMM registers, which are 128 bits? So 128 bits should be as fast as 64 bits, in theory.

pcwalton · 2023-11-25T20:12:42Z

crates/bevy_render/src/pipeline_keys/mod.rs

+        let positions = T::positions(store);
+        let so = positions.get(&TypeId::of::<K>()).unwrap();
+        self.packed &= !(((1 << so.size) - 1) << so.offset);
+        self.packed |= value.packed << so.offset;


Could maybe add a debug_assert! to make sure the packed value didn't overflow the size

the only way to construct a PackedPipelineKey is via new(), which does this check (i should probably change that to a debug assert though).

and any type inserted here needs to be part of the composite type already (else the positions lookup will fail) - you can't add new arbitrary types.

pcwalton · 2023-11-26T00:20:41Z

crates/bevy_pbr/src/material.rs

+#[derive(PipelineKey, Clone, Copy)]
+#[repr(u8)]
+#[custom_shader_defs]
+pub enum ScreenSpaceTransmissionQualityKey {


Can we just merge this into ScreenSpaceTransmissionQuality?

pcwalton · 2023-11-26T00:21:32Z

crates/bevy_pbr/src/render/mesh.rs

+pub struct MeshPipelineKey {
+    pub view_key: PbrViewKey,
+    pub mesh_key: MeshKey,
+    pub alpha_mode: AlphaKey,


I'd add comments here documenting what each of these is for.

pcwalton · 2023-11-26T00:22:28Z

crates/bevy_render/src/lib.rs

@@ -89,6 +92,8 @@ pub enum RenderSet {
    ManageViews,
    /// The copy of [`apply_deferred`] that runs immediately after [`ManageViews`](RenderSet::ManageViews).
    ManageViewsFlush,
+    /// blah


Fill in this comment :)

pcwalton · 2023-11-26T00:42:16Z

crates/bevy_render/src/pipeline_keys/packed_types.rs

+        | wgpu::PrimitiveTopology::LineList
+        | wgpu::PrimitiveTopology::LineStrip
+        | wgpu::PrimitiveTopology::TriangleList
+        | wgpu::PrimitiveTopology::TriangleStrip => (),


Same as above.

pcwalton · 2023-11-26T00:42:47Z

crates/bevy_render/src/pipeline_keys/packed_types.rs

+            3 => wgpu::PrimitiveTopology::TriangleList,
+            4 => wgpu::PrimitiveTopology::TriangleStrip,
+            _ => unreachable!(),
+        }


Might be worth macro_rules!ing this. Up to you.

several of these will be much easier once https://doc.rust-lang.org/std/mem/fn.variant_count.html stabilises.

i'm not clear how i could write a macro currently, what sort of thing did you have in mind? something that takes all the variants as args and generates the pack and unpack?

pcwalton · 2023-11-26T00:42:58Z

crates/bevy_render/src/pipeline_keys/packed_types.rs

+    // 2 variants => 1 bits
+    match value {
+        wgpu::Face::Front | wgpu::Face::Back => (),
+    }


Same as above.

pcwalton · 2023-11-26T00:43:47Z

crates/bevy_render/src/view/mod.rs

+        match self.0 {
+            false => vec!["TONEMAP_IN_SHADER".into()],
+            true => vec!["HDR".into()],
+        }


Could just be if instead of match

how do you feel about self.0.then_some("TONEMAP_IN_SHADER".into()).into_iter().collect() ?

i did that on GizmoConfigKey for example. it should definitely be consistent whichever one we use though.

pcwalton · 2023-11-26T00:44:16Z

crates/bevy_sprite/src/mesh2d/mesh.rs

+#[derive(PipelineKey)]
+pub struct Mesh2dPipelineKey {
+    pub view_key: Mesh2dViewKey,
+    pub mesh_key: MeshKey,


As above, comment these fields.

robtfm · 2023-11-27T00:38:10Z

My main comment is that I wonder whether the pipeline key stuff could be factored out into a separate bevy_bitfields crate or something. bevy_render is already quite large, so this would reduce compile time, and you've created a very generic bitfield crate here that might be useful in other contexts.

i chose not to as there are a few bits that are very pipeline-key specific - the KeyMeta member for ShaderDefFn, and the derive macro which implements all the base key traits as well as ShaderDefs (if not specified explicitly). i also plan to add more stuff like these in future for bind group management.

i can probably extract the KeyMeta specifics with a generic param. i'm not sure how to handle the macro, i don't think it's possible to call another derive macro from within a derive macro, and i don't really want every instance to have to derive two separate things, but maybe that's preferable.

robtfm added 18 commits October 28, 2023 00:36

wip

aeff65d

before any

9ad338b

cleaner macros

929127f

dyn working

5d5a9ec

renames

c72e85d

packed key

814ab24

u64 key

a325075

specialize use PipelineKey

78836e6

move shader defs to fn

5315299

fmt

73a868b

more keys

1daffef

use keys for PbrMaterial

0e8e987

fix deferred, clean enum macro, remove num_enum

4abf872

fix instancing

5daa254

move key store into pipeline cache

27e758a

fix load prepass normals

6e97ceb

cleanup

147456d

fix morph targets and instancing

57477a3

Merge remote-tracking branch 'upstream/main' into pipeline-keys

934226a

robtfm added 6 commits November 1, 2023 18:03

update for ssst

5e85e35

ci

d06e50b

ci

85e4765

NewMaterialPipelineKey -> MaterialPipelineKey

fd97008

new_packed_key -> packed_key

9bd5ed6

comments and cleanup

dfaafc4

rest of the pipelines

544c414

nicopap reviewed Nov 10, 2023

View reviewed changes

robtfm added 6 commits November 11, 2023 14:34

Merge remote-tracking branch 'upstream/main' into pipeline-keys

f5c3981

ui material

7ae539f

fmt

aad13e1

remove bevy_internal

1afd1fe

flatten Material2dPipelineKey

7cb67ce

Merge remote-tracking branch 'upstream/main' into pipeline-keys

ce6007d

robtfm mentioned this pull request Nov 21, 2023

Reintroduce missing prepass bindings #10667

Closed

JMS55 added this to the 0.13 milestone Nov 23, 2023

pcwalton self-requested a review November 25, 2023 19:50

pcwalton reviewed Nov 26, 2023

View reviewed changes

robtfm added 10 commits December 3, 2023 17:38

Merge remote-tracking branch 'upstream/main' into pipeline-keys

f675a3b

debug_assert

934ca77

ci

74d0ee3

SSTQKey -> SSTQ

8c9da2e

comment MeshPipelineKey

7d2b0f2

phase comment

fc1dae3

assert_eq -> debug_assert_eq

4fea970

remove packed type check fns

9c0fb76

texture_format shader_defs

0b58e58

comment Mesh2dPipelineKey

28ed8f0

alice-i-cecile modified the milestones: 0.13, 0.14 Jan 24, 2024

JMS55 removed this from the 0.14 milestone May 14, 2024

alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline keys #10333

Pipeline keys #10333

robtfm commented Nov 1, 2023 •

edited

Loading

superdump commented Nov 1, 2023

nicopap left a comment •

edited

Loading

nicopap Nov 10, 2023

robtfm Nov 10, 2023

nicopap Nov 10, 2023

robtfm Nov 10, 2023

pcwalton Nov 25, 2023

robtfm Nov 27, 2023

nicopap commented Nov 11, 2023

robtfm commented Nov 11, 2023

Many keys per entity

`SystemKey`

pcwalton left a comment

pcwalton Nov 25, 2023

pcwalton Nov 25, 2023

robtfm Nov 27, 2023

pcwalton Nov 26, 2023

pcwalton Nov 26, 2023

pcwalton Nov 26, 2023

pcwalton Nov 26, 2023

pcwalton Nov 26, 2023

robtfm Nov 27, 2023

pcwalton Nov 26, 2023

pcwalton Nov 26, 2023

robtfm Nov 27, 2023

pcwalton Nov 26, 2023

robtfm commented Nov 27, 2023

Pipeline keys #10333

Are you sure you want to change the base?

Pipeline keys #10333

Conversation

robtfm commented Nov 1, 2023 • edited Loading

Objective

Solution

introduced a PipelineKey macro and related traits. PipelineKey types are either:

implemented usage for MeshPipeline and MaterialPipeline (including the prepass pipeline).

todo

todo as follow ups

performance:

Migration Guide

superdump commented Nov 1, 2023

nicopap left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicopap commented Nov 11, 2023

API Usage

Many keys per entity

SystemKey

robtfm commented Nov 11, 2023

Many keys per entity

SystemKey

pcwalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robtfm commented Nov 27, 2023

robtfm commented Nov 1, 2023 •

edited

Loading

introduced a `PipelineKey` macro and related traits. PipelineKey types are either:

nicopap left a comment •

edited

Loading

`SystemKey`

`SystemKey`