Add LUTs generation on fly if ktx2 is not enabled by bugsweeper · Pull Request #13734 · bevyengine/bevy

bugsweeper · 2024-06-07T11:26:00Z

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 · 2024-06-07T12:50:43Z

Cargo.toml

  "bevy_internal/bevy_core_pipeline",
  "bevy_asset",
  "bevy_render",
+  "bevy_pbr",


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

which wants bevy_pbr for Opaque3dPrepass bindings initialization

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

I said about this initialization in description

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 · 2024-06-07T14:59:59Z

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

bugsweeper · 2024-06-07T15:24:41Z

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 · 2024-06-07T20:47:25Z

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 · 2024-06-07T23:47:43Z

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

…s disabled

bugsweeper · 2024-06-14T09:31:36Z

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 · 2024-06-17T15:55:02Z

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 · 2024-06-17T16:46:15Z

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

bugsweeper · 2024-06-18T15:32:44Z

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

…disabled

alice-i-cecile · 2024-06-18T16:18:03Z

Sounds good, let's land something more scoped.

bugsweeper · 2024-06-19T09:45:08Z

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 · 2024-06-21T10:04:10Z

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

alice-i-cecile · 2024-06-21T13:09:53Z

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 · 2024-06-25T21:11:22Z

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 · 2024-06-25T22:14:28Z

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

alice-i-cecile added this to the 0.14 milestone Jun 7, 2024

alice-i-cecile reviewed Jun 7, 2024

View reviewed changes

alice-i-cecile added A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Needs-SME This type of work requires an SME to approve it. S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 7, 2024

bugsweeper marked this pull request as draft June 7, 2024 15:19

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 9, 2024

bugsweeper force-pushed the fix_ktx2_pbr_dependencies branch 3 times, most recently from 179be1d to ec1d77d Compare June 13, 2024 12:31

SMAA generates search look up table on fly, if loading from ktx2 file…

43b9848

…s disabled

bugsweeper force-pushed the fix_ktx2_pbr_dependencies branch from ec1d77d to 43b9848 Compare June 13, 2024 13:27

jgayfer added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 14, 2024

jgayfer added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 14, 2024

bugsweeper force-pushed the fix_ktx2_pbr_dependencies branch from 1ed5bb7 to 7edff96 Compare June 17, 2024 15:52

bugsweeper marked this pull request as ready for review June 18, 2024 15:37

SMAA generates area look up table on fly, if loading from ktx2 files …

acea9e2

…disabled

bugsweeper force-pushed the fix_ktx2_pbr_dependencies branch from 7edff96 to acea9e2 Compare June 18, 2024 15:40

jgayfer added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 19, 2024

Azorlogh mentioned this pull request Jun 20, 2024

Update to bevy 0.14 rparrett/bevy_prototype_lyon#247

Merged

alice-i-cecile changed the title ~~Fix bevy_core_pipeline and ktx2 dependencies~~ Add LUTs generation on fly if ktx2 is not enabled Jun 25, 2024

alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes C-Bug An unexpected or incorrect behavior and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jun 25, 2024

alice-i-cecile closed this Jun 25, 2024

Uh oh!

Conversation

bugsweeper commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

alice-i-cecile Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

bugsweeper Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

bugsweeper Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mockersf commented Jun 7, 2024

Uh oh!

bugsweeper commented Jun 7, 2024

Uh oh!

alice-i-cecile commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mockersf commented Jun 7, 2024

Uh oh!

bugsweeper commented Jun 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bugsweeper commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bugsweeper commented Jun 17, 2024

Uh oh!

bugsweeper commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jun 18, 2024

Uh oh!

bugsweeper commented Jun 19, 2024

Uh oh!

bugsweeper commented Jun 21, 2024

Uh oh!

alice-i-cecile commented Jun 21, 2024

Uh oh!

JMS55 commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Jun 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bugsweeper commented Jun 7, 2024 •

edited

Loading

bugsweeper Jun 7, 2024 •

edited

Loading

alice-i-cecile commented Jun 7, 2024 •

edited

Loading

bugsweeper commented Jun 14, 2024 •

edited

Loading

bugsweeper commented Jun 17, 2024 •

edited

Loading

bugsweeper commented Jun 18, 2024 •

edited

Loading

JMS55 commented Jun 25, 2024 •

edited

Loading