Notes on adding a bevy_pbr_types crate

[greeble-dev]

These notes document my attempt at moving parts of bevy_pbr into a new bevy_pbr_types crate.

Why?

I want to make bevy_gltf less dependent on render crates like bevy_pbr. This would be good for compile times and other things - see my previous notes for more info.

A secondary goal is to document options for the render crate restructure. I feel that working through one narrow case in full can surface useful information, and solving problems for one crate can benefit other crates.

Also, I just really like refactoring things. This is a normal hobby enjoyed by normal people such as myself.

How?

bevy_gltf mostly wants to construct component and asset data defined in bevy_pbr - it doesn't need 95% of the code. So I moved these components and assets into a new bevy_pbr_types crate - then bevy_gltf depends only on this crate and not bevy_pbr.

StandardMaterial

The biggest hurdle is StandardMaterial. bevy_gltf just wants to construct it as an asset, but the same struct also does bind group layout. This is required by the Material trait, which is bound on Asset + AsBindGroup.

We don't want to pull all the bind group stuff into bevy_pbr_types - instead we want StandardMaterial to be just an Asset and let something else do the rest.

First, I changed Material to remove the Asset bound. Instead it links to an asset type, and requires a from_source_asset method to construct itself from that asset:

- pub trait Material: Asset + AsBindGroup + Clone + Sized {
+ pub trait Material: AsBindGroup + Sized + TypePath + Send + Sync {
+     type SourceAsset: Asset + Clone;
+ 
+     fn from_source_asset(source_asset: Self::SourceAsset) -> Self;

This means MeshMaterial3d can point to an Asset instead of a Material:

- pub struct MeshMaterial3d<M: Material>(pub Handle<M>);
+ pub struct MeshMaterial3d<M: Asset + Clone>(pub Handle<M>);

Now any Asset type can be a material, and it's up to the renderer internals to say how that type gets turned into a Material.

But at this point we haven't achieved much since StandardMaterial still implements both Asset and Material.

Next I made a duplicate of StandardMaterial called StandardMaterialInternal (which is obviously bad, but I'll get to that later). StandardMaterial is an Asset. StandardMaterialInternal is a Material + bind group layout, and gets a from_source_asset implementation that takes StandardMaterial:

impl Material for StandardMaterialInternal {
    type SourceAsset = StandardMaterial;

    fn from_source_asset(source_asset: Self::SourceAsset) -> Self {
        Self {
            base_color: source_asset.base_color,
            base_color_channel: source_asset.base_color_channel,
            ... remaining 60 lines of boilerplate ...

Once that's all done, StandardMaterial and MeshMaterial3d can move to bevy_pbr_types.

Problems

Duplicating the whole of StandardMaterial is a big oof. There's also some boilerplate elsewhere - various custom materials still implement both Asset and Material, but need this faff to link them together:

  impl Material for CustomMaterial {
+     type SourceAsset = Self;
+ 
+     fn from_source_asset(source_asset: Self::SourceAsset) -> Self {
+         source_asset
+     }

In practice the only thing that calls from_source_asset is PreparedMaterial::prepare_asset, which immediately turns the struct into uniforms and bind groups and then throws it away. If that work can be done without the intermediate struct then a lot of the duplication and boilerplate can disappear.

Another issue is that users might be confused by MeshMaterial3d<M: Asset>, since there's no indication of what types are actually compatible. One solution might be to add an empty MaterialAsset trait just to make the dependency clear, but that means yet more boilerplate for material implementors.

Light Components

bevy_gltf wants to construct PointLight, DirectionalLight and SpotLight components. These are fairly plain data, except they're tied to renderer internals by required components and hooks.

#[require(Frustum, VisibleMeshEntities, Transform, Visibility, VisibilityClass)]
#[component(on_add = view::add_visibility_class::<LightVisibilityClass>)]
pub struct SpotLight {

The required components can move to PbrPlugin::build, although it's a bit ugly:

  .register_type::<SpotLight>()
+ .register_required_components::<SpotLight, Frustum>()
+ .register_required_components::<SpotLight, VisibleMeshEntities>()
+ .register_required_components::<SpotLight, Transform>()
+ .register_required_components::<SpotLight, Visibility>()
+ .register_required_components::<SpotLight, VisibilityClass>()

In theory the on_add hook can also move PbrPlugin::build:

render_app.world_mut()
    .register_component_hooks::<SpotLight>()
    .on_add(|world, context| {
        bevy_render::view::add_visibility_class::<LightVisibilityClass>(world, context);
    });
}

But I couldn't get that working. It seems to register fine but never triggers. register_component_hooks_by_id also didn't work as the id couldn't be found. So for now I've left the on_add hook on the component.

The Rest

Various types and consts also moved to bevy_pbr_types, because bevy_gltf uses them directly or they're used by StandardMaterial or the light components. These were all simple cut and pastes.

And We're Done?

- bevy_pbr = { path = "../bevy_pbr", version = "0.16.0-dev" }
+ bevy_pbr_types = { path = "../bevy_pbr_types", version = "0.16.0-dev" }

Full branch is here: main...greeble-dev:bevy:gltf-pbr-types

So Was It All Worth It?

Maybe. The immediate goal is achieved, although there's only a slight improvement in build times since it hasn't reduced the critical path by much.

The main issue is whether the material changes are a good idea. Adding all that faff and boilerplate is bad if the only goal is reducing bevy_gltf dependencies. But maybe they end up aligning with other efforts to refactor materials and make them more data-driven.

What's Next?

bevy_pbr_types and bevy_gltf are still dependent on bevy_render. This could be solved by creating bevy_render_types, but I don't currently have any plans to pull that thread.