[Merged by Bors] - Introduce SystemLabel's for RenderAssetPlugin, and change Image preparation system to run before others

[blaind]

Objective

Fixes StandardMaterial texture update (see sample code below).

Most probably fixes #3674 (did not test)

Solution

Material updates, such as PBR update, reference the underlying GpuImage. Like here:

bevy/crates/bevy_pbr/src/pbr_material.rs

Line 177 in 9a7852d

.get_image_texture(gpu_images, &material.base_color_texture)

However, currently the GpuImage update may actually happen after the material update fetches the gpu image. Resulting in the material actually not being updated for the correct gpu image.

In this pull req, I introduce new systemlabels for the renderassetplugin. Also assigned the RenderAssetPlugin:: to the PreAssetExtract stage, so that it is executed before any material updates.

Code to test.

Expected behavior:

• should update to red texture

Unexpected behavior (before this merge):

• texture stays randomly as green one (depending on the execution order of systems)

use bevy::{
    prelude::*,
    render::render_resource::{Extent3d, TextureDimension, TextureFormat},
};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .add_system(changes)
        .run();
}

struct Iteration(usize);

#[derive(Component)]
struct MyComponent;

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
    mut images: ResMut<Assets<Image>>,
) {
    commands.spawn_bundle(PointLightBundle {
        point_light: PointLight {
            ..Default::default()
        },
        transform: Transform::from_xyz(4.0, 8.0, 4.0),
        ..Default::default()
    });

    commands.spawn_bundle(PerspectiveCameraBundle {
        transform: Transform::from_xyz(-2.0, 0.0, 5.0)
            .looking_at(Vec3::new(0.0, 0.0, 0.0), Vec3::Y),
        ..Default::default()
    });

    commands.insert_resource(Iteration(0));

    commands
        .spawn_bundle(PbrBundle {
            mesh: meshes.add(Mesh::from(shape::Quad::new(Vec2::new(3., 2.)))),
            material: materials.add(StandardMaterial {
                base_color_texture: Some(images.add(Image::new(
                    Extent3d {
                        width: 600,
                        height: 400,
                        depth_or_array_layers: 1,
                    },
                    TextureDimension::D2,
                    [0, 255, 0, 128].repeat(600 * 400), // GREEN
                    TextureFormat::Rgba8Unorm,
                ))),
                ..Default::default()
            }),
            ..Default::default()
        })
        .insert(MyComponent);
}

fn changes(
    mut materials: ResMut<Assets<StandardMaterial>>,
    mut images: ResMut<Assets<Image>>,
    mut iteration: ResMut<Iteration>,
    webview_query: Query<&Handle<StandardMaterial>, With<MyComponent>>,
) {
    if iteration.0 == 2 {
        let material = materials.get_mut(webview_query.single()).unwrap();

        let image = images
            .get_mut(material.base_color_texture.as_ref().unwrap())
            .unwrap();

        image
            .data
            .copy_from_slice(&[255, 0, 0, 255].repeat(600 * 400));
    }

    iteration.0 += 1;
}

[blaind]

Actually, there is still another (root?) problem left

changing the let material = materials.get_mut(webview_query.single()).unwrap(); to being .get() instead of mutable get, does not trigger the material change anymore.

Result: image is not being updated.

[superdump]

Actually, there is still another (root?) problem left

changing the let material = materials.get_mut(webview_query.single()).unwrap(); to being .get() instead of mutable get, does not trigger the material change anymore.

Result: image is not being updated.

Note that RenderAsset extraction only notes created/modified/removed assets and then immutably borrowing an asset marks none of those things. Then, I think this is because the mutation of the Image causes creation of a completely new GpuImage. But if the material is not mutably borrowed, then it will not be marked as modified and so that new GpuImage will not be used and updated in the material.

Separately, I wonder if this setup is leaking textures on the GPU or if there's wonderful RAII cleanup stuff that happens when the old GpuImage (texture, texture view, sampler) is dropped. I would guess so but I'm not sure.

[superdump]

I'm not sure about this approach. @cart - what do you think?

[superdump]

Is there any problem with using the same labels for many systems?

[cart]

Nope this is fine / a pattern we use in a number of places.

[blaind]

Note that RenderAsset extraction only notes created/modified/removed assets and then immutably borrowing an asset marks none of those things. Then, I think this is because the mutation of the Image causes creation of a completely new GpuImage. But if the material is not mutably borrowed, then it will not be marked as modified and so that new GpuImage will not be used and updated in the material.

Did some more debugging, and this seems to be cause.

With images.get_mut(), the render_queue.write_texture and hence also new TextureView creation occurs.

However, the StandardMaterial.prepare_asset is not triggered anymore, and thereby (probably) the old TextureView is referenced instead of the new one in the bind group (is this cached for future frames?).

The whole process of creating new GpuImage, TextureView, StandardMaterial etc. seems quite a bit of low-performance approach especially if the texture needs to be updated on each frame.

Maybe some alternative approaches (mapping the GPU texture memory and updating directly? could be used), especially in the cases where the image.data would change, with the texture metadata staying intact.

[superdump]

Yeah, it's not really set up for in-place modification, just uploading the necessary data. rafx has a shadow map atlas which then needs to update sub-rectangles of the full texture, possibly many of them each frame. I imagine we'd want to have some kind of API to allow users to do that - update some sub-rectangle of the texture and have that be used. I don't know if the creation of the new texture, texture view, etc take any significant amount of time, but updating more data than necessary in a texture could easily take too much time (and bandwidth.)

[blaind]

Yeah, it's not really set up for in-place modification, just uploading the necessary data. rafx has a shadow map atlas which then needs to update sub-rectangles of the full texture, possibly many of them each frame. I imagine we'd want to have some kind of API to allow users to do that - update some sub-rectangle of the texture and have that be used. I don't know if the creation of the new texture, texture view, etc take any significant amount of time, but updating more data than necessary in a texture could easily take too much time (and bandwidth.)

That would be great, especially if it allowed updating a portion (rectangle) of the texture.

Btw, also noticed that bevy_ui seems to have invalidation of GpuImages. Didn't find similar functionality from bevy_pbr

bevy/crates/bevy_ui/src/render/mod.rs

Line 431 in 9a7852d

AssetEvent::Modified { handle } => image_bind_groups.values.remove(handle),

Another direction to investigate, could be to run render_queue.write_texture in the Image.prepare_asset stage, but for existing Texture (instead of creating new texture).

[CptPotato]

I think adding labels to the asset's extract + prepare systems is a nice addition.

One issue I see with this specific approach is that there's a fixed amount of "stages" if I understand correctly. This would mean that longer dependency chains can't be handled with this.

Would it make sense to automatically assign every extract and prepare system a unique label and then allow specifying dependencies in the asset's RenderAsset impl? (both for extract and prepare)

[superdump]

Perhaps we could add a fn label() -> &’static str; to RenderAsset if it’s not possible otherwise?

[CptPotato]

Perhaps we could add a fn label() -> &’static str; to RenderAsset if it’s not possible otherwise?

That sounds like a simple enough approach. In that case a default implementation could maybe fallback to a label based on the asset's uuid (not sure how the const and 'static plays out).

Though I don't know if this is the right direction.

[cart]

Sorry for the criminal levels of neglect here. Now that we have "auto system labeling", we can make the api a little more generic. Something like:

pub struct RenderAssetPlugin<A: RenderAsset> {
    labels: Vec<fn(ParallelSystemDescriptor) -> ParallelSystemDescriptor>,
    marker: PhantomData<fn() -> A>,
}

impl<A: RenderAsset> Default for RenderAssetPlugin<A> {
    fn default() -> Self {
        Self {
            labels: Vec::new(),
            marker: PhantomData,
        }
    }
}

impl<A: RenderAsset> RenderAssetPlugin<A> {
    pub fn prepare_after_asset<B: Asset>(mut self) -> Self {
        self.prepare_after(prepare_assets::<B>)
    }
    pub fn prepare_after(mut self, label: impl SystemLabel) -> Self {
        self.labels.push(|descriptor| descriptor.after(label));
        self
    }
}

impl<A: RenderAsset> Plugin for RenderAssetPlugin<A> {
    fn build(&self, app: &mut App) {
        if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
            let mut prepare_system = ParallelSystemDescriptor::new(prepare_assets::<A>);
            for labeler in self.labels.iter() {
                prepare_system = (labeler)(prepare_system);
            }
            render_app
                .init_resource::<ExtractedAssets<A>>()
                .init_resource::<RenderAssets<A>>()
                .init_resource::<PrepareNextFrameAssets<A>>()
                .add_system_to_stage(RenderStage::Extract, extract_render_asset::<A>)
                .add_system_to_stage(RenderStage::Prepare, prepare_system);
        }
    }
}

However!!! While this is more flexible, for the case of "preparing images first", I don't think manually adding a dependency is desirable. People defining their own materials shouldn't need to worry about adding a dependency on Image prep. Images should just "default" to being first. Therefore, I think the impl in this PR is a reasonable way to do this generically.

If we have other specific cases that need the more flexible approach, I'm down to add something like the code above. But otherwise, I think we should merge the current PR as-is.

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• check-benches
• check-doc
• check-markdown-links
• check-missing-examples-in-docs
• check-unused-dependencies
• ci
• markdownlint
• miri
• run-examples