Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix some post-processing issues #7474

Closed
wants to merge 3 commits into from
Closed

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented Feb 2, 2023

Objective

fix some post-processing issues

Solution

  • make upscaling load existing color, and use Some(BlendState::ALPHA_BLENDING) when writing each camera's result to the output texture instead of just overwriting
  • also fix a minor issue with bloom where the output image's alpha was just the bloom alpha. this now needs to be maxed since we blend the output

todo:

  • maybe add a CameraBlendMode(Option<Wgpu::BlendState>) component to allow controlling the blend mode
  • maybe clear instead of loading on first upscale for a given target (wgpu docs say you must clear before loading, but it seems to work for me without ... but my ci seems to be crashing on linux examples)
  • check for performance impact

fix #7435
fix #7361
hopefully fix #5721 (again i haven't tested that)


Migration Guide

  • Using a clear color with alpha != 1 will lead to ghosting. make sure your clear color alpha is 1 on the first camera for a given target.
  • Previously where you would use ClearColor::None on secondary cameras, you will now probably want ClearColorConfig::Custom(Color::NONE), which will clear the texture to transparent
  • this may change the apparent sensitivity of bloom on secondary cameras. this is because previously the bloom was using the clear color from previous cameras, which will now not be present in the camera's texture. you can work around this by changing the sensitivity (i found turning up the knee to 4.0 seemed to be the same as a clear color of 0.1, ymmv) or re-ordering the cameras.

@robtfm robtfm changed the title Upscale blend fix some post-processing issues Feb 2, 2023
@robtfm robtfm added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Feb 2, 2023
@Keelar
Copy link

Keelar commented Feb 2, 2023

Just tested #5721 again and unlike #7460 this PR does not fix it in its current state.

@robtfm
Copy link
Contributor Author

robtfm commented Feb 2, 2023

Just tested #5721 again and unlike #7460 this PR does not fix it in its current state.

thanks for checking it. do you have a small reproduction?

@Keelar
Copy link

Keelar commented Feb 2, 2023

Sure, this is what I've been testing it with:

use bevy::{core_pipeline::clear_color::ClearColorConfig, prelude::*, render::view::RenderLayers};

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(startup)
        .run();
}

fn startup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn(Camera2dBundle {
        camera_2d: Camera2d {
            clear_color: ClearColorConfig::Custom(Color::WHITE),
        },
        ..Default::default()
    });
    commands.spawn((
        UiCameraConfig { show_ui: false },
        Camera2dBundle {
            camera_2d: Camera2d {
                clear_color: ClearColorConfig::None,
            },
            camera: Camera {
                order: 1,
                ..Default::default()
            },
            ..Default::default()
        },
        RenderLayers::layer(1),
    ));

    commands.spawn(NodeBundle {
        style: Style {
            size: Size::new(Val::Percent(100.0), Val::Percent(50.0)),
            ..Default::default()
        },
        background_color: BackgroundColor(Color::rgba(1.0, 0.0, 0.0, 0.8)),
        ..Default::default()
    });

    commands.spawn((
        SpriteBundle {
            texture: asset_server.load("branding/icon.png"),
            ..Default::default()
        },
        RenderLayers::layer(1),
    ));
}

The Node should fill up the top half of the window and be behind the Sprite, but the bug causes it to not be visible at all unless the top layer has show_ui enabled.

@robtfm
Copy link
Contributor Author

robtfm commented Feb 2, 2023

if you change the second camera's clear color to clear_color: ClearColorConfig::Custom(Color::NONE), then i think it looks like you're describing. the top half is a bit darker, but only behind the sprite.

i mentioned that in the migration guide, but i think if we decide this is the right approach it might be worth changing the behaviour of ClearColorConfig::None so that it does a transparent clear (which is what Custom(NONE) does) instead of no clear, then this would work without change.

@Keelar
Copy link

Keelar commented Feb 2, 2023

Ah, you're right. That does fix it. Sorry I missed that in the migration guide. I do agree about maybe changing the behavior of ClearColorConfig::None. Using ClearColorConfig::Custom(Color::NONE) when ClearColorConfig::None exists is very counter intuitive.

@robtfm
Copy link
Contributor Author

robtfm commented Feb 3, 2023

closing too for #7490

@robtfm robtfm closed this Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
2 participants