add tonemapping LUT bindings for sprite and mesh2d pipelines

[arcashka]

Fixes #13118
If you use Sprite or Mesh2d and create Camera with

• hdr=false
• any tonemapper

You would get

wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
      note: label = `sprite_pipeline`
    Error matching ShaderStages(FRAGMENT) shader requirements against the pipeline
    Shader global ResourceBinding { group: 0, binding: 19 } is not available in the pipeline layout
    Binding is missing from the pipeline layout

Because of missing tonemapping LUT bindings

Solution

Add missing bindings for tonemapping LUT's to SpritePipeline & Mesh2dPipeline

Testing

I checked that

• tonemapping
• color_grading
• sprite_animations
• 2d_shapes
• meshlet
• deferred_rendering
  examples are still working

2d cases I checked with this code:

use bevy::{
    color::palettes::css::PURPLE, core_pipeline::tonemapping::Tonemapping, prelude::*,
    sprite::MaterialMesh2dBundle,
};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, toggle_tonemapping_method)
        .run();
}

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<ColorMaterial>>,
    asset_server: Res<AssetServer>,
) {
    commands.spawn(Camera2dBundle {
        camera: Camera {
            hdr: false,
            ..default()
        },
        tonemapping: Tonemapping::BlenderFilmic,
        ..default()
    });
    commands.spawn(MaterialMesh2dBundle {
        mesh: meshes.add(Rectangle::default()).into(),
        transform: Transform::default().with_scale(Vec3::splat(128.)),
        material: materials.add(Color::from(PURPLE)),
        ..default()
    });

    commands.spawn(SpriteBundle {
        texture: asset_server.load("asd.png"),
        ..default()
    });
}

fn toggle_tonemapping_method(
    keys: Res<ButtonInput<KeyCode>>,
    mut tonemapping: Query<&mut Tonemapping>,
) {
    let mut method = tonemapping.single_mut();

    if keys.just_pressed(KeyCode::Digit1) {
        *method = Tonemapping::None;
    } else if keys.just_pressed(KeyCode::Digit2) {
        *method = Tonemapping::Reinhard;
    } else if keys.just_pressed(KeyCode::Digit3) {
        *method = Tonemapping::ReinhardLuminance;
    } else if keys.just_pressed(KeyCode::Digit4) {
        *method = Tonemapping::AcesFitted;
    } else if keys.just_pressed(KeyCode::Digit5) {
        *method = Tonemapping::AgX;
    } else if keys.just_pressed(KeyCode::Digit6) {
        *method = Tonemapping::SomewhatBoringDisplayTransform;
    } else if keys.just_pressed(KeyCode::Digit7) {
        *method = Tonemapping::TonyMcMapface;
    } else if keys.just_pressed(KeyCode::Digit8) {
        *method = Tonemapping::BlenderFilmic;
    }
}

---

Changelog

Fix the bug which led to the crash when user uses any tonemapper without hdr for rendering sprites and 2d meshes.

[arcashka]

this bindings are defined in the separate file because of the bevyengine/naga_oil#90

[kristoff3r]

Maybe link that issue in the file or where it is imported? I couldn't figure out from skimming the issue if you feel like a cleaner solution is possible later.

[arcashka]

Rebased due to the conflcts

[jnhyatt]

Generally looks good, just looks like half the diff is miscellaneous cleanup? Not a problem, just want to make sure I'm not missing something. Couple of unimportant style things. Otherwise lgtm

[arcashka]

From what I read in the Bevy Discord, the common consensus is that it's better to use explicit indices instead of sequential

[jnhyatt]

This diff is pretty gnarly but it looks like it's just a (unrelated?) refactor of this system into two, right?

[arcashka]

It's not unrelated. These systems are split into two because one of them does per-sprite setup, prepare_sprite_image_bind_groups, and the other one does per-view setup, prepare_sprite_view_bind_groups.

Before this PR, it was done in one system because earlier view bind groups were always the same, even for different views. Now, since different views (cameras) can have different tonemapping settings, we need to be able to provide different LUTs for different views.
It's pretty close to what is currently done for

• 3d meshes: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/view/mod.rs#L788
• 2d meshes: https://github.com/bevyengine/bevy/blob/main/crates/bevy_sprite/src/mesh2d/mesh.rs#L587

[arcashka]

Generally looks good, just looks like half the diff is miscellaneous cleanup? Not a problem, just want to make sure I'm not missing something. Couple of unimportant style things. Otherwise lgtm

@jnhyatt thank you for the review ❤️
I rechecked the changes, most of them are not unrelated.
There are bunch of shader reorganization changes, but they are required. Because lut bindings are defined like this

@group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>;
@group(0) @binding(#TONEMAPPING_LUT_SAMPLER_BINDING_INDEX) var dt_lut_sampler: sampler;

tonemapping_shared.wgsl can not be imported unless TONEMAPPING_LUT_TEXTURE_BINDING_INDEX and TONEMAPPING_LUT_SAMPLER_BINDING_INDEX are defined.
So I had to move pow_safe from tonemapping_shared because it's being used outside of tonemapping in some cases.

[kristoff3r]

LGTM, also tested tonemapping example on wasm

[kristoff3r]

Maybe link that issue in the file or where it is imported? I couldn't figure out from skimming the issue if you feel like a cleaner solution is possible later.

[alice-i-cecile]

@arcashka once merge conflicts are resolve I'll merge this in :)

[arcashka]

Maybe link that issue in the file or where it is imported? I couldn't figure out from skimming the issue if you feel like a cleaner solution is possible later.

I thought about it but decided against it because having bindings in a separate module is pretty common within the Bevy source code.

I don't think having lut bindings in the separate file is something which can cause questions/needs explaining. It's a standard practive and by accident also a way to bypass this weird naga-oil issue.

[arcashka]

@arcashka once merge conflicts are resolve I'll merge this in :)

Rebased and resolved merge conflicts :)