Add LUTs generation on fly if ktx2 is not enabled

[bugsweeper]

Add LUTs generation on fly if ktx2 is not enabled

Objective

• Partially addresses In 0.14.0-rc.2, minimal app with bevy_core_pipeline crashes without ktx2, zstd, and bevy_pbr #13728 .
• If default features are disabled some subcrates has implicit but not explicit dependencies.
  For example bevy_core_pipeline adds SmaaPlugin which wants ktx2 textures, ktx2 uses zstd for supercompression

Solution

• Added feature smaa_luts. If enabled SMAA uses LUTs from ktx2 files, if disabled SMAA uses LUTs generated on fly. Generators are based on on Jorge Jimenez code for AreaTex and SearchTex

Testing

• I ran reproducing example from In 0.14.0-rc.2, minimal app with bevy_core_pipeline crashes without ktx2, zstd, and bevy_pbr #13728 with enabled bevy_pbr and got no panics
• I made comparison app (which compares data from ktx2 luts and from generators) that runs with no panics

fn main() {
    let mut app = App::new();
    app.add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, check)
        .run();
}

#[derive(Resource, Default)]
struct Luts {
    search: Handle<Image>,
    area: Handle<Image>,
}

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.insert_resource(Luts {
        search: asset_server.load("/some/path/to/bevy_core_pipeline/src/smaa/SMAASearchLUT.ktx2"),
        area: asset_server.load("/some/path/to/bevy_core_pipeline/src/smaa/SMAAAreaLUT.ktx2"),
    })
}

fn check(
    mut processed: Local<u8>,
    luts: Res<Luts>,
    images: Res<Assets<Image>>,
    mut events: EventReader<AssetEvent<Image>>,
    mut app_exit_writer: EventWriter<AppExit>,
) {
    for event in events.read() {
        if event.is_loaded_with_dependencies(luts.search.id()) {
            if let Some(search) = images.get(luts.search.id()) {
                let generated = search_data()
                    .into_iter()
                    .map(|color| {
                        (Srgba::gamma_function(color as f32 / 255.) * 255.).floor()
                            as u8
                    })
                    .collect::<Vec<_>>();
                // Search look up tables are exactly the same
                assert_eq!(search.data, generated);
                *processed += 1;

                continue;
            }
        }
        if event.is_loaded_with_dependencies(luts.area.id()) {
            if let Some(area) = images.get(luts.area.id()) {
                let generated = area_data()
                    .into_iter()
                    .map(|color| {
                        (Srgba::gamma_function(color as f32 / 255.) * 255.).floor()
                            as u8
                    })
                    .collect::<Vec<_>>();
                assert_eq!(
                    area.data
                        .iter()
                        .zip(generated.iter())
                        .fold(0, |sum, (&goal, &generated)| {
                            let diff = goal.abs_diff(generated);
                            // Some pixels differs in color by 1-2 color points
                            assert!(diff < 3);
                            sum + diff as usize
                        }),
                    // Sum of diffs is only 582 from 179200 values
                    // diffs are mostly on borders of subsamples
                    // because compression algorithms usualy don't like high contrast
                    582
                );

                for (goal, &generated) in area.data.iter().zip(generated.iter()) {
                    // Some pixels differs in color by 1-2 color points
                    assert!(goal.abs_diff(generated) < 3);
                }

                *processed += 1;
            }
        }
    }

    if *processed == 2 {
        app_exit_writer.send(AppExit::Success);
    }
}

[alice-i-cecile]

I don't like pulling in bevy_pbr here. What's causing the failure?

[alice-i-cecile]

which wants bevy_pbr for Opaque3dPrepass bindings initialization

[alice-i-cecile]

Let me pull in rendering folks, but I think the correct solution is to actually move that code.

[bugsweeper]

I said about this initialization in description

[bugsweeper]

I think the correct solution is to actually move that code.

Which code are you proposing to move? Is this Core3dPlugin from bevy_core_pipeline to bevy_pbr?

[mockersf]

ktx2 should not enable zstd, and bevy_core_pipeline should not enable bevy_pbr or ktx2

[bugsweeper]

ktx2 should not enable zstd, and bevy_core_pipeline should not enable bevy_pbr or ktx2

As I mentioned in PR there are dependencies, should SmaaPlugin, Core3dPlugin become optional, or everyone must enable all necessary features manualy?

[alice-i-cecile]

ktx2 should not enable zstd, and bevy_core_pipeline should not enable bevy_pbr or ktx2

As I mentioned in PR there are dependencies, should SmaaPlugin, Core3dPlugin become optional, or everyone must enable all necessary features manualy?

I'm pretty sure we should move the offending code out of bevy_pbr and into bevy_core_pipeline.

[mockersf]

for ktx2 / ztsd features, like tonemapping LUTs being behind a feature tonemapping_luts, SMAA LUTs should be behind a new feature smaa_luts

[bugsweeper]

added S-Needs-Review and removed S-Waiting-on-Author

There is only SMAASearchLUT (SMAA search look up table) recreated, SMAAAreaLUT is not ready yet, and I didn't do anything about

we should move the offending code out of bevy_pbr and into bevy_core_pipeline

[bugsweeper]

Generator for search LUT generates exactly the same table, generator of area LUT generates table which absolutely differs up to 2 for some pixel colors. I think it's due to compression loss in ktx2 (Generators are even more precise)

[bugsweeper]

At this moment only bevy_core_pipeline dependences from ktx2/zstd are fixed

[bugsweeper]

we should move the offending code out of bevy_pbr and into bevy_core_pipeline

@alice-i-cecile Sorry, I tried to move out code from bevy_pbr to bevy_core_pipeline, that code appeared to be very dependent. As I mentioned earlier bevy_pbr adds plugins, which are needed for bevy_core_pipeline:

BinnedRenderPhasePlugin::<Opaque3dPrepass, MeshPipeline>::default(),
BinnedRenderPhasePlugin::<AlphaMask3dPrepass, MeshPipeline>::default(),

The journey started from MeshPipeline, and one by one in a chain it forced me to move MeshPipelineViewLayouts, MeshLayouts, MeshPipelineViewLayout, MeshUniform and so on, as far I was going as more I was feeling that I'm doing something wrong.
I propose to make this change focused on SMAA LUTs and drop connection to issue #13728 , because fix is not complete and we should not close issue automaticaly

[alice-i-cecile]

Sounds good, let's land something more scoped.

[bugsweeper]

added S-Waiting-on-Author and removed S-Needs-Review

Changes to replace loading SMAA LUT from ktx2 files are done. This PR is no longer waiting for the author.

[bugsweeper]

Should I make unit/integration test (maybe ignored by default) from test code in PR description?

[alice-i-cecile]

I'm on the fence about an integration test. On the one hand, this is hard to avoid breaking. On the other hand, I worry about the test being brittle. I'd prefer to set up integration tests for feature flags in a separate PR, since they're quite complex to design.

[JMS55]

My 2cents: Generating LUTs is a lot of code for no good reason. SMAA just shouldn't be usable without ktx2.

Furthermore, we don't need to put SMAA behind a new smaa cfg. We can leave the majority of the code exposed without a feature flag, and simply put the LUT load behind a cfg for ktx2, and panic if ktx2 is disabled.

Alternatively, we can include the raw bytes of the image within bevy's binary directly, without parsing it as a ktx2.

[alice-i-cecile]

The original purpose of this has been superseded by #14020, and I agree with Jasmine's comments above.