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

Allow mix of hdr and non-hdr cameras to same render target #13419

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

tychedelia
Copy link
Contributor

Changes:

  • Track whether an output texture has been written to yet and only clear it on the first write.
  • Use ClearColorConfig on CameraOutputMode instead of a raw LoadOp.
  • Track whether a output texture has been seen when specializing the upscaling pipeline and use alpha blending for extra cameras rendering to that texture that do not specify an explicit blend mode.

Fixes #6754

Testing

Tested against provided test case in issue:
image


Changelog

  • Allow cameras rendering to the same output texture with mixed hdr to work correctly.

Migration Guide

    • Change CameraOutputMode to use ClearColorConfig instead of LoadOp.

Changes:
- Track whether an output texture has been written to yet and only
clear it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing
the upscaling pipeline and use alpha blending for extra cameras
rendering to that texture that do not specify an explicit blend
mode.

Fixes bevyengine#6754
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels May 18, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 18, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 19, 2024
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall I think, although I'd reconsider the getter-mutator pattern which might cause confusion and future issues (and limit ability to test).

@arcashka
Copy link
Contributor

I checked the split_screen example, setting one of the cameras to hdr results in different issues based on which camera is set to be hdr
camera 0:
Foxes 2 and 3 (from camera 1 and 2) leave a trace on rotating
image

camera 1 and 2:
corresponding foxes (and ui) are not showing up
image

camera 3:
fox 4 (camera 3) leaves a trace on rotation
image

my system:

2024-05-20T12:12:44.069461Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux 23.10 Ubuntu", kernel: "6.5.0-35-generic", cpu: "12th Gen Intel(R) Core(TM) i7-12800H", core_count: "14", memory: "31.0 GiB" }
2024-05-20T12:12:44.078237Z  INFO bevy_winit::system: Creating new window "App" (Entity { index: 0, generation: 1 })
2024-05-20T12:12:44.078435Z  INFO log: Guessed window scale factor: 1
2024-05-20T12:12:44.118479Z  INFO bevy_render::renderer: AdapterInfo { name: "Intel(R) Graphics (ADL GT2)", vendor: 32902, device: 17958, device_type: IntegratedGpu, driver: "Intel open-source Mesa driver", driver_info: "Mesa 23.2.1-1ubuntu3.1", backend: Vulkan }

commit: 8bdaffc

Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking based on last comment, for clarity. Apparently this is not fixed.

@tychedelia
Copy link
Contributor Author

I checked the split_screen example, setting one of the cameras to hdr results in different issues based on which camera is...

Can reproduce, thanks.

@tychedelia
Copy link
Contributor Author

So for split_screen, this behavior is kind of hurting my head, and there's a question here in terms of what the desired behavior actually even is. There's at least a few things going on.

  1. Most importantly per the example's logic, if you set a camera after 0 to hdr: true, then you must also set a ClearColorConfig for the camera at that index. Missing this will break a lot of the permutations I tested.
  2. The blend mode really matters. For example, if I set camera 1 and 2 to hdr: true, by default I get traces:
    image

but if I change the blend_state to something custom (i.e. not ALPHA_BLENDING) by using CameraOutputMode everything blends out better (in the sense of, no artifacts).

image

This can explain why the UI errors happen here, since they are painted before the first resize event fires.

  1. Turning off MSAA affects things a lot. Here's the same corrected blend as above but with MSAA off.

image

I guess part of my confusion here is that I don't think using mixed HDR cameras makes any sense in the scenario where you are also setting the viewport, because we will apply a blend state to the entire screen that makes it difficult to get the correct look within a single viewport.

In other words, I don't think it's possible to make this "just work" given the current state of the renderer. We make a best guess for the blend state, but more complex scenarios with interlaced cameras require a bit more intentional thought here about what the desired look is.

There may be other stuff going on here that I'm missing.

@arcashka
Copy link
Contributor

Then maybe the current setup is correct?

If a user wants a configuration with multiple cameras or targets with different settings, it's their responsibility to provide the appropriate settings for blending and clearing behavior. The user can also specify the camera order, which should suffice for most cases, if not all.

@tychedelia, do you have an example where it's not possible to achieve the correct result using only client-side settings without your changes?

Example from this comment #6754 (comment) works fine on main with

output_mode: CameraOutputMode::Write {
    blend_state: Some(BlendState::ALPHA_BLENDING),
    color_attachment_load_op: LoadOp::Load,
},

on hdr camera

I tried to investigate the same issue some time ago, there are few messages in discord about this https://discord.com/channels/691052431525675048/743663924229963868/1235125809707225098
But I gave up because I wasn't sure what should be the correct behavior 😆

@tychedelia
Copy link
Contributor Author

Okay, there's definitely still some strange interaction with MSAA enabled that I don't totally understand. Still looking into it.

@tychedelia
Copy link
Contributor Author

tychedelia commented May 21, 2024

@arcashka Okay, I was getting totally turned around around because the blend state is indeed really important. But the traces were coming from a weird interaction with MSAA I just fixed. Please try again and let me know if this solves your problems.

@JMS55 19b3d4c was a real doozy to track down. The issue here was that the MSAA writeback was being triggered purely on the basis of the view target. In a scenario with mixed hdr, this meant that, e.g., a hdr camera with order 1 could have it's texture marked as clear when post_process_write is called when it actually hasn't been cleared yet because it hasn't even been written to yet. This means that the first call logic doesn't work and we always load. I fixed this by tracking hdr on ExtractedCamera and using that to trigger the writeback, which I think works??

@tychedelia
Copy link
Contributor Author

Here's the blend state I'm using for reference:

                            CameraOutputMode::Write {
                                blend_state: Some(BlendState {
                                    color: BlendComponent {
                                        src_factor: BlendFactor::SrcAlpha,
                                        dst_factor: BlendFactor::OneMinusSrcAlpha,
                                        operation: BlendOperation::Min,
                                    },
                                    alpha: BlendComponent::OVER,
                                }),
                                clear_color: ClearColorConfig::None,
                            }

@arcashka
Copy link
Contributor

@tychedelia I've checked the example from #6754 (comment) and now it's not working, only green cube from hdr camera shows up. I tried different blend modes and clear_color: ClearColorConfig::None

I've managed to make it work locally by reverting 19b3d4c, but that would probably bring back the issue with traces :(

@tychedelia
Copy link
Contributor Author

@tychedelia I've checked the example from #6754 (comment) and now it's not working, only green cube from hdr camera shows up. I tried different blend modes and clear_color: ClearColorConfig::None

I've managed to make it work locally by reverting 19b3d4c, but that would probably bring back the issue with traces :(

Just checked and this is still working for me on 18640c7?

image

Here's the archive just to confirm:
hdr_issue_fixed.zip

@arcashka
Copy link
Contributor

@tychedelia ah, I missed clear_color on the camera itself, was setting it only for output_mode. Thank you, now this example works.
And regarding split_screen example, have you managed to make it work with your changes? For me it's always either hdr fox is not showing up or the rest of them leave traces. I tried a lot of different combinations of clear colors and blending modes, none worked for me

@tychedelia
Copy link
Contributor Author

tychedelia commented May 22, 2024

And regarding split_screen example, have you managed to make it work with your changes? For me it's always either hdr fox is not showing up or the rest of them leave traces. I tried a lot of different combinations of clear colors and blending modes, none worked for me

I'll post my example when I get home for split screen, but traces shouldn't be there anymore unless the clear config is set to none. The traces you were originally seeing were definitely an effect of the MSAA bug I discovered, but getting the config right can be a pain. Apologies, I should have just posted my reference split screen for others to verify. Thanks for bearing with me here.

@tychedelia
Copy link
Contributor Author

(true, false, false, false)

image

//! Renders two cameras to the same window to accomplish "split screen".

use std::f32::consts::PI;

use bevy::render::camera::CameraOutputMode;
use bevy::render::render_resource::{BlendComponent, BlendFactor, BlendOperation, BlendState};
use bevy::{
    pbr::CascadeShadowConfigBuilder, prelude::*, render::camera::Viewport, window::WindowResized,
};

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

/// set up a simple 3D scene
fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    // plane
    commands.spawn(PbrBundle {
        mesh: meshes.add(Plane3d::default().mesh().size(100.0, 100.0)),
        material: materials.add(Color::srgb(0.3, 0.5, 0.3)),
        ..default()
    });

    commands.spawn(SceneBundle {
        scene: asset_server.load("models/animated/Fox.glb#Scene0"),
        ..default()
    });

    // Light
    commands.spawn(DirectionalLightBundle {
        transform: Transform::from_rotation(Quat::from_euler(EulerRot::ZYX, 0.0, 1.0, -PI / 4.)),
        directional_light: DirectionalLight {
            shadows_enabled: true,
            ..default()
        },
        cascade_shadow_config: CascadeShadowConfigBuilder {
            num_cascades: 2,
            first_cascade_far_bound: 200.0,
            maximum_distance: 280.0,
            ..default()
        }
        .into(),
        ..default()
    });

    // Cameras and their dedicated UI
    for (index, (camera_name, camera_pos, hdr)) in [
        ("Player 1", Vec3::new(0.0, 200.0, -150.0), true),
        ("Player 2", Vec3::new(150.0, 150., 50.0), false),
        ("Player 3", Vec3::new(100.0, 150., -150.0), false),
        ("Player 4", Vec3::new(-100.0, 80., 150.0), false),
    ]
    .iter()
    .enumerate()
    {
        let camera = commands
            .spawn((
                Camera3dBundle {
                    transform: Transform::from_translation(*camera_pos)
                        .looking_at(Vec3::ZERO, Vec3::Y),
                    camera: Camera {
                        hdr: *hdr,
                        // Renders cameras with different priorities to prevent ambiguities
                        order: index as isize,
                        // Don't clear after the first camera because the first camera already cleared the entire window
                        clear_color: if index > 1 {
                            ClearColorConfig::None
                        } else {
                            ClearColorConfig::default()
                        },
                        output_mode: if index > 0 {
                            CameraOutputMode::Write {
                                blend_state: Some(BlendState {
                                    color: BlendComponent {
                                        src_factor: BlendFactor::SrcAlpha,
                                        dst_factor: BlendFactor::OneMinusSrcAlpha,
                                        operation: BlendOperation::Max,
                                    },
                                    alpha: BlendComponent::OVER,
                                }),
                                clear_color: ClearColorConfig::None,
                            }
                        } else {
                            CameraOutputMode::Write {
                                blend_state: Some(BlendState::ALPHA_BLENDING),
                                clear_color: ClearColorConfig::None,
                            }
                        },
                        ..default()
                    },
                    ..default()
                },
                CameraPosition {
                    pos: UVec2::new((index % 2) as u32, (index / 2) as u32),
                },
            ))
            .id();

        // Set up UI
        commands
            .spawn((
                TargetCamera(camera),
                NodeBundle {
                    style: Style {
                        width: Val::Percent(100.),
                        height: Val::Percent(100.),
                        padding: UiRect::all(Val::Px(20.)),
                        ..default()
                    },
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    *camera_name,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
                buttons_panel(parent);
            });
    }

    fn buttons_panel(parent: &mut ChildBuilder) {
        parent
            .spawn(NodeBundle {
                style: Style {
                    position_type: PositionType::Absolute,
                    width: Val::Percent(100.),
                    height: Val::Percent(100.),
                    display: Display::Flex,
                    flex_direction: FlexDirection::Row,
                    justify_content: JustifyContent::SpaceBetween,
                    align_items: AlignItems::Center,
                    padding: UiRect::all(Val::Px(20.)),
                    ..default()
                },
                ..default()
            })
            .with_children(|parent| {
                rotate_button(parent, "<", Direction::Left);
                rotate_button(parent, ">", Direction::Right);
            });
    }

    fn rotate_button(parent: &mut ChildBuilder, caption: &str, direction: Direction) {
        parent
            .spawn((
                RotateCamera(direction),
                ButtonBundle {
                    style: Style {
                        width: Val::Px(40.),
                        height: Val::Px(40.),
                        border: UiRect::all(Val::Px(2.)),
                        justify_content: JustifyContent::Center,
                        align_items: AlignItems::Center,
                        ..default()
                    },
                    border_color: Color::WHITE.into(),
                    image: UiImage::default().with_color(Color::srgb(0.25, 0.25, 0.25)),
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    caption,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
            });
    }
}

#[derive(Component)]
struct CameraPosition {
    pos: UVec2,
}

#[derive(Component)]
struct RotateCamera(Direction);

enum Direction {
    Left,
    Right,
}

fn set_camera_viewports(
    windows: Query<&Window>,
    mut resize_events: EventReader<WindowResized>,
    mut query: Query<(&CameraPosition, &mut Camera)>,
) {
    // We need to dynamically resize the camera's viewports whenever the window size changes
    // so then each camera always takes up half the screen.
    // A resize_event is sent when the window is first created, allowing us to reuse this system for initial setup.
    for resize_event in resize_events.read() {
        let window = windows.get(resize_event.window).unwrap();
        let size = window.physical_size() / 2;

        for (camera_position, mut camera) in &mut query {
            camera.viewport = Some(Viewport {
                physical_position: camera_position.pos * size,
                physical_size: size,
                ..default()
            });
        }
    }
}

#[allow(clippy::type_complexity)]
fn button_system(
    interaction_query: Query<
        (&Interaction, &TargetCamera, &RotateCamera),
        (Changed<Interaction>, With<Button>),
    >,
    mut camera_query: Query<&mut Transform, With<Camera>>,
) {
    for (interaction, target_camera, RotateCamera(direction)) in &interaction_query {
        if let Interaction::Pressed = *interaction {
            // Since TargetCamera propagates to the children, we can use it to find
            // which side of the screen the button is on.
            if let Ok(mut camera_transform) = camera_query.get_mut(target_camera.entity()) {
                let angle = match direction {
                    Direction::Left => -0.1,
                    Direction::Right => 0.1,
                };
                camera_transform.rotate_around(Vec3::ZERO, Quat::from_axis_angle(Vec3::Y, angle));
            }
        }
    }
}

(true, true, false, false)

image

//! Renders two cameras to the same window to accomplish "split screen".

use std::f32::consts::PI;

use bevy::render::camera::CameraOutputMode;
use bevy::render::render_resource::{BlendComponent, BlendFactor, BlendOperation, BlendState};
use bevy::{
    pbr::CascadeShadowConfigBuilder, prelude::*, render::camera::Viewport, window::WindowResized,
};

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

/// set up a simple 3D scene
fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    // plane
    commands.spawn(PbrBundle {
        mesh: meshes.add(Plane3d::default().mesh().size(100.0, 100.0)),
        material: materials.add(Color::srgb(0.3, 0.5, 0.3)),
        ..default()
    });

    commands.spawn(SceneBundle {
        scene: asset_server.load("models/animated/Fox.glb#Scene0"),
        ..default()
    });

    // Light
    commands.spawn(DirectionalLightBundle {
        transform: Transform::from_rotation(Quat::from_euler(EulerRot::ZYX, 0.0, 1.0, -PI / 4.)),
        directional_light: DirectionalLight {
            shadows_enabled: true,
            ..default()
        },
        cascade_shadow_config: CascadeShadowConfigBuilder {
            num_cascades: 2,
            first_cascade_far_bound: 200.0,
            maximum_distance: 280.0,
            ..default()
        }
        .into(),
        ..default()
    });

    // Cameras and their dedicated UI
    for (index, (camera_name, camera_pos, hdr)) in [
        ("Player 1", Vec3::new(0.0, 200.0, -150.0), true),
        ("Player 2", Vec3::new(150.0, 150., 50.0), true),
        ("Player 3", Vec3::new(100.0, 150., -150.0), false),
        ("Player 4", Vec3::new(-100.0, 80., 150.0), false),
    ]
    .iter()
    .enumerate()
    {
        let camera = commands
            .spawn((
                Camera3dBundle {
                    transform: Transform::from_translation(*camera_pos)
                        .looking_at(Vec3::ZERO, Vec3::Y),
                    camera: Camera {
                        hdr: *hdr,
                        // Renders cameras with different priorities to prevent ambiguities
                        order: index as isize,
                        // Don't clear after the first camera because the first camera already cleared the entire window
                        clear_color: if index == 1 || index == 3 {
                            ClearColorConfig::None
                        } else {
                            ClearColorConfig::default()
                        },
                        output_mode: CameraOutputMode::Write {
                            blend_state: Some(BlendState {
                                color: BlendComponent {
                                    src_factor: BlendFactor::SrcAlpha,
                                    dst_factor: BlendFactor::OneMinusSrcAlpha,
                                    operation: BlendOperation::Max,
                                },
                                alpha: BlendComponent::OVER,
                            }),
                            clear_color: ClearColorConfig::None,
                        },
                        ..default()
                    },
                    ..default()
                },
                CameraPosition {
                    pos: UVec2::new((index % 2) as u32, (index / 2) as u32),
                },
            ))
            .id();

        // Set up UI
        commands
            .spawn((
                TargetCamera(camera),
                NodeBundle {
                    style: Style {
                        width: Val::Percent(100.),
                        height: Val::Percent(100.),
                        padding: UiRect::all(Val::Px(20.)),
                        ..default()
                    },
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    *camera_name,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
                buttons_panel(parent);
            });
    }

    fn buttons_panel(parent: &mut ChildBuilder) {
        parent
            .spawn(NodeBundle {
                style: Style {
                    position_type: PositionType::Absolute,
                    width: Val::Percent(100.),
                    height: Val::Percent(100.),
                    display: Display::Flex,
                    flex_direction: FlexDirection::Row,
                    justify_content: JustifyContent::SpaceBetween,
                    align_items: AlignItems::Center,
                    padding: UiRect::all(Val::Px(20.)),
                    ..default()
                },
                ..default()
            })
            .with_children(|parent| {
                rotate_button(parent, "<", Direction::Left);
                rotate_button(parent, ">", Direction::Right);
            });
    }

    fn rotate_button(parent: &mut ChildBuilder, caption: &str, direction: Direction) {
        parent
            .spawn((
                RotateCamera(direction),
                ButtonBundle {
                    style: Style {
                        width: Val::Px(40.),
                        height: Val::Px(40.),
                        border: UiRect::all(Val::Px(2.)),
                        justify_content: JustifyContent::Center,
                        align_items: AlignItems::Center,
                        ..default()
                    },
                    border_color: Color::WHITE.into(),
                    image: UiImage::default().with_color(Color::srgb(0.25, 0.25, 0.25)),
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    caption,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
            });
    }
}

#[derive(Component)]
struct CameraPosition {
    pos: UVec2,
}

#[derive(Component)]
struct RotateCamera(Direction);

enum Direction {
    Left,
    Right,
}

fn set_camera_viewports(
    windows: Query<&Window>,
    mut resize_events: EventReader<WindowResized>,
    mut query: Query<(&CameraPosition, &mut Camera)>,
) {
    // We need to dynamically resize the camera's viewports whenever the window size changes
    // so then each camera always takes up half the screen.
    // A resize_event is sent when the window is first created, allowing us to reuse this system for initial setup.
    for resize_event in resize_events.read() {
        let window = windows.get(resize_event.window).unwrap();
        let size = window.physical_size() / 2;

        for (camera_position, mut camera) in &mut query {
            camera.viewport = Some(Viewport {
                physical_position: camera_position.pos * size,
                physical_size: size,
                ..default()
            });
        }
    }
}

#[allow(clippy::type_complexity)]
fn button_system(
    interaction_query: Query<
        (&Interaction, &TargetCamera, &RotateCamera),
        (Changed<Interaction>, With<Button>),
    >,
    mut camera_query: Query<&mut Transform, With<Camera>>,
) {
    for (interaction, target_camera, RotateCamera(direction)) in &interaction_query {
        if let Interaction::Pressed = *interaction {
            // Since TargetCamera propagates to the children, we can use it to find
            // which side of the screen the button is on.
            if let Ok(mut camera_transform) = camera_query.get_mut(target_camera.entity()) {
                let angle = match direction {
                    Direction::Left => -0.1,
                    Direction::Right => 0.1,
                };
                camera_transform.rotate_around(Vec3::ZERO, Quat::from_axis_angle(Vec3::Y, angle));
            }
        }
    }
}

(false, false, true, false)

image

//! Renders two cameras to the same window to accomplish "split screen".

use std::f32::consts::PI;

use bevy::render::camera::CameraOutputMode;
use bevy::render::render_resource::{BlendComponent, BlendFactor, BlendOperation, BlendState};
use bevy::{
    pbr::CascadeShadowConfigBuilder, prelude::*, render::camera::Viewport, window::WindowResized,
};

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

/// set up a simple 3D scene
fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    // plane
    commands.spawn(PbrBundle {
        mesh: meshes.add(Plane3d::default().mesh().size(100.0, 100.0)),
        material: materials.add(Color::srgb(0.3, 0.5, 0.3)),
        ..default()
    });

    commands.spawn(SceneBundle {
        scene: asset_server.load("models/animated/Fox.glb#Scene0"),
        ..default()
    });

    // Light
    commands.spawn(DirectionalLightBundle {
        transform: Transform::from_rotation(Quat::from_euler(EulerRot::ZYX, 0.0, 1.0, -PI / 4.)),
        directional_light: DirectionalLight {
            shadows_enabled: true,
            ..default()
        },
        cascade_shadow_config: CascadeShadowConfigBuilder {
            num_cascades: 2,
            first_cascade_far_bound: 200.0,
            maximum_distance: 280.0,
            ..default()
        }
        .into(),
        ..default()
    });

    // Cameras and their dedicated UI
    for (index, (camera_name, camera_pos, hdr)) in [
        ("Player 1", Vec3::new(0.0, 200.0, -150.0), false),
        ("Player 2", Vec3::new(150.0, 150., 50.0), false),
        ("Player 3", Vec3::new(100.0, 150., -150.0), true),
        ("Player 4", Vec3::new(-100.0, 80., 150.0), false),
    ]
    .iter()
    .enumerate()
    {
        let camera = commands
            .spawn((
                Camera3dBundle {
                    transform: Transform::from_translation(*camera_pos)
                        .looking_at(Vec3::ZERO, Vec3::Y),
                    camera: Camera {
                        hdr: *hdr,
                        // Renders cameras with different priorities to prevent ambiguities
                        order: index as isize,
                        // Don't clear after the first camera because the first camera already cleared the entire window
                        clear_color: if index == 0 || index == 2 {
                            ClearColorConfig::default()
                        } else {
                            ClearColorConfig::None
                        },
                        output_mode: CameraOutputMode::Write {
                            blend_state: Some(BlendState {
                                color: BlendComponent {
                                    src_factor: BlendFactor::SrcAlpha,
                                    dst_factor: BlendFactor::OneMinusSrcAlpha,
                                    operation: BlendOperation::Max,
                                },
                                alpha: BlendComponent::OVER,
                            }),
                            clear_color: ClearColorConfig::None,
                        },
                        ..default()
                    },
                    ..default()
                },
                CameraPosition {
                    pos: UVec2::new((index % 2) as u32, (index / 2) as u32),
                },
            ))
            .id();

        // Set up UI
        commands
            .spawn((
                TargetCamera(camera),
                NodeBundle {
                    style: Style {
                        width: Val::Percent(100.),
                        height: Val::Percent(100.),
                        padding: UiRect::all(Val::Px(20.)),
                        ..default()
                    },
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    *camera_name,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
                buttons_panel(parent);
            });
    }

    fn buttons_panel(parent: &mut ChildBuilder) {
        parent
            .spawn(NodeBundle {
                style: Style {
                    position_type: PositionType::Absolute,
                    width: Val::Percent(100.),
                    height: Val::Percent(100.),
                    display: Display::Flex,
                    flex_direction: FlexDirection::Row,
                    justify_content: JustifyContent::SpaceBetween,
                    align_items: AlignItems::Center,
                    padding: UiRect::all(Val::Px(20.)),
                    ..default()
                },
                ..default()
            })
            .with_children(|parent| {
                rotate_button(parent, "<", Direction::Left);
                rotate_button(parent, ">", Direction::Right);
            });
    }

    fn rotate_button(parent: &mut ChildBuilder, caption: &str, direction: Direction) {
        parent
            .spawn((
                RotateCamera(direction),
                ButtonBundle {
                    style: Style {
                        width: Val::Px(40.),
                        height: Val::Px(40.),
                        border: UiRect::all(Val::Px(2.)),
                        justify_content: JustifyContent::Center,
                        align_items: AlignItems::Center,
                        ..default()
                    },
                    border_color: Color::WHITE.into(),
                    image: UiImage::default().with_color(Color::srgb(0.25, 0.25, 0.25)),
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn(TextBundle::from_section(
                    caption,
                    TextStyle {
                        font_size: 20.,
                        ..default()
                    },
                ));
            });
    }
}

#[derive(Component)]
struct CameraPosition {
    pos: UVec2,
}

#[derive(Component)]
struct RotateCamera(Direction);

enum Direction {
    Left,
    Right,
}

fn set_camera_viewports(
    windows: Query<&Window>,
    mut resize_events: EventReader<WindowResized>,
    mut query: Query<(&CameraPosition, &mut Camera)>,
) {
    // We need to dynamically resize the camera's viewports whenever the window size changes
    // so then each camera always takes up half the screen.
    // A resize_event is sent when the window is first created, allowing us to reuse this system for initial setup.
    for resize_event in resize_events.read() {
        let window = windows.get(resize_event.window).unwrap();
        let size = window.physical_size() / 2;

        for (camera_position, mut camera) in &mut query {
            camera.viewport = Some(Viewport {
                physical_position: camera_position.pos * size,
                physical_size: size,
                ..default()
            });
        }
    }
}

#[allow(clippy::type_complexity)]
fn button_system(
    interaction_query: Query<
        (&Interaction, &TargetCamera, &RotateCamera),
        (Changed<Interaction>, With<Button>),
    >,
    mut camera_query: Query<&mut Transform, With<Camera>>,
) {
    for (interaction, target_camera, RotateCamera(direction)) in &interaction_query {
        if let Interaction::Pressed = *interaction {
            // Since TargetCamera propagates to the children, we can use it to find
            // which side of the screen the button is on.
            if let Ok(mut camera_transform) = camera_query.get_mut(target_camera.entity()) {
                let angle = match direction {
                    Direction::Left => -0.1,
                    Direction::Right => 0.1,
                };
                camera_transform.rotate_around(Vec3::ZERO, Quat::from_axis_angle(Vec3::Y, angle));
            }
        }
    }
}

@arcashka
Copy link
Contributor

hello @tychedelia, thank you for providing examples, I don't think I would've ever came up with using BlendOperation::Max myself. Still not sure how exactly that works 😆
Anyway, I checked all of your examples and they work. Then I tried different combinations of your commits, and looks like 8bdaffc is the fix here.
5e80886 doesn't makes a difference, it was the same behavior with and without it.

Do you think 5e80886 is still required? Do you have any scenarios where it's not possible to get correct output without these changes?

@tychedelia
Copy link
Contributor Author

Thanks for checking!

Do you think 5e80886 is still required? Do you have any scenarios where it's not possible to get correct output without these changes?

Yes, it's necessary for scenarios where the cameras render to the same viewport size.

@arcashka
Copy link
Contributor

I think I found another issue. I added this system to rotate cubes from https://github.com/bevyengine/bevy/files/15405152/hdr_issue_fixed.zip

fn move_mesh(mut transforms: Query<&mut Transform, With<Handle<Mesh>>>, time: Res<Time>) {
    for mut transform in &mut transforms {
        transform.rotate_y(time.delta_seconds());
    }
}

And it results in traces on 18640c7. Changing blending/clear_color on output_mode didn't help
image

@tychedelia
Copy link
Contributor Author

tychedelia commented May 27, 2024

And it results in traces on 18640c7. Changing blending/clear_color on output_mode didn't help

Here's some camera settings:

image

Camera3dBundle {
      transform: Transform::from_translation(Vec3::splat(10.0)).looking_at(Vec3::ZERO, Vec3::Y),
      camera: Camera {
        clear_color: ClearColorConfig::Custom(Color::BLACK),
        order: 1,
        hdr: true,
        output_mode: CameraOutputMode::Write {
          clear_color: ClearColorConfig::None,
          blend_state: Some(BlendState{
            color: BlendComponent {
              src_factor: BlendFactor::OneMinusSrc,
              dst_factor: BlendFactor::OneMinusDst,
              operation: BlendOperation::Max,
            },
            alpha: BlendComponent::OVER,
          }),
        },
        ..Default::default()
      },
      ..Default::default()
    }

We make it possible, but it's tricky. For example, if you clear the first camera with blue, these settings would wash out the green with the blue. In practice, mixing these cameras without really intentional purpose feels like a mistake to me? But I feel pretty confident this PR has at least made it possible where it wasn't before.

@arcashka
Copy link
Contributor

Yes these settings are working, you are right, thank you!
They are also working with only msaa writeback changes.
I agree this PR makes possible the cases which were not possible before. But I think only 19b3d4c changes are helping here. Haven't seen a single case which is fixed by 5e80886. It's always working the same with and without it.

@tychedelia
Copy link
Contributor Author

I agree this PR makes possible the cases which were not possible before. But I think only 19b3d4c changes are helping here. Haven't seen a single case which is fixed by 5e80886. It's always working the same with and without it.

Yes, 19b3d4c is the primary defect here, in the sense that previously it was making certain manually configured output_mode settings not take effect.

However, 5e80886 allows mixed hdr cameras to "just work" without having to manually configure output_mode. You can test this with the hdr_issue example I provided, if you only cherry-pick 19b3d4c it will no longer work, because the default load op configured on the camera will clear the output texture.

Also, I realized a better fix to your mesh movement in hdr_issue (and potentially some of the fox examples too) is actually just to set ClearColorConfig::Custom(Color::NONE) on the second camera, which will clear the previous frame but not add a background. The issue with adding a background is that then you have to think about how the background should blend with meshes that were rendered by previous camera. The BlendOperation::Max examples I was providing you were kind of a hack around this -- basically any color (i.e. the cube) is always going to be greater than the gray background, which means we would blend the previous mesh correctly, but this is also something that would break down quickly outside of a simple example.

Let me know if that makes sense.

@arcashka
Copy link
Contributor

I'm not fully certain yet if the ability to not set the camera's output mode to the correct values is a good thing here. I understand your point, but it's just weird to me that in the hdr_issue example, if I set the output_mode to something like:

output_mode: CameraOutputMode::Write {
    clear_color: ClearColorConfig::Custom(LinearRgba::RED.into()),
    blend_state: None,
},

the output would be two cubes on a grey background instead of one cube on a red background. I think requiring the user to be explicit is a better approach here. That being said, I think the original issue is resolved, and it's totally fine if you decide to leave the PR as it is. Thank you for bearing with me here 😃

The ClearColorConfig::Custom(Color::NONE) is a really nice find! And thank you for explanation on why BlendOperation::Max was working, I was really confused by it. Definitely learned a bunch of new stuff from this PR

@tychedelia
Copy link
Contributor Author

I'm not fully certain yet if the ability to not set the camera's output mode to the correct values is a good thing here.

I agree that it's weird, but it functions similarly to how the clear config works for a camera's non-output texture where only the first configured clear will take effect. Overall, I think this is more consistent, but maybe should be documented better across the board that the first camera for any texture is the one that controls this. We might be able to fiddle a bit and allow an explicit overwrite just by making the default ClearColorConfig::None.

Definitely learned a bunch of new stuff from this PR

Thanks for helping get it into a good place!

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 4, 2024
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review. The rest is incoming…

(Some(clear_color), true) => LoadOp::Clear(clear_color.into()),
(None, _) | (Some(_), false) => LoadOp::Load,
},
store: StoreOp::Store,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we never not store these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't in the current implementation. The intermediate textures are always store too. It might be good to create a ticket for this for future optimization?

@tychedelia tychedelia requested a review from superdump June 6, 2024 19:58
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit nervous about adding more special casing / implicit behavior to our camera configuration, but I do think this makes things more usable and consistent in this context.

@cart cart added this pull request to the merge queue Jun 6, 2024
Merged via the queue into bevyengine:main with commit 027f8e2 Jun 6, 2024
29 checks passed
mockersf pushed a commit that referenced this pull request Jun 6, 2024
Changes:
- Track whether an output texture has been written to yet and only clear
it on the first write.
- Use `ClearColorConfig` on `CameraOutputMode` instead of a raw
`LoadOp`.
- Track whether a output texture has been seen when specializing the
upscaling pipeline and use alpha blending for extra cameras rendering to
that texture that do not specify an explicit blend mode.

Fixes #6754

## Testing

Tested against provided test case in issue:

![image](https://github.com/bevyengine/bevy/assets/10366310/d066f069-87fb-4249-a4d9-b6cb1751971b)

---

## Changelog

- Allow cameras rendering to the same output texture with mixed hdr to
work correctly.

## Migration Guide

- - Change `CameraOutputMode` to use `ClearColorConfig` instead of
`LoadOp`.
@tychedelia
Copy link
Contributor Author

I'm a bit nervous about adding more special casing / implicit behavior to our camera configuration, but I do think this makes things more usable and consistent in this context.

I agree -- the current clear behavior with multiple cameras is surprising. But at least this PR makes it consistent. Not sure about how the new render graph will affect this, but it feels like all of these should be Option<ClearColorConfig> maybe so we can distinguish between explicit and implicit requests to clear.

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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mix of HDR and non-HDR cameras breaks rendering
8 participants