bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.06k stars 3.56k forks source link

Color Attribute Doesn't do sRGB Color Correction #5547

Open Rigidity opened 2 years ago

Rigidity commented 2 years ago

Bevy version

0.8

Relevant system information

AdapterInfo { name: "NVIDIA GeForce RTX 3070 Ti", vendor: 4318, device: 9346, device_type: DiscreteGpu, backend: Vulkan }

What you did

I used the Mesh::ATTRIBUTE_COLOR attribute to try to depict the color purple ([1.0, 0.0, 1.0, 1.0]) on a triangle, and it looks like this: image

What went wrong

It's showing a different color than purple, presumably because it's not applying sRGB color correction.

CptPotato commented 2 years ago

At least currently, it is more or less the intended behavior to use linear rgb for vertex colors: (see #4528)

Though it might be worth revisiting.

CptPotato commented 2 years ago

Actually, I don't think this can be a result of the (missing) srgb conversion because the extreme values you're using (0.0, 1.0) would be unaffected by the conversion function (i.e. #ff00ff is the same in linear rgb and srgb).

I suspect it's either tonemapping or some pbr effects are still enabled and throw the result off (specular, fresnel).

Rigidity commented 2 years ago

Yeah, it might not be sRGB specifically, but I made a Purple cube using the Material system to make a texture, and when doing it manually with a color, it's a different color. I would expect them to be the same.

rparrett commented 2 years ago

Vertex colors are multiplied by the material color.

Bevy's default material for PbrBundles without a material set is Color::rgb(1.0, 0.0, 0.5) (https://github.com/bevyengine/bevy/blob/main/crates/bevy_pbr/src/lib.rs#L197)

Give your PbrBundle a material: materials.add(StandardMaterial::default()) and you should see the expected color.

AThilenius commented 1 year ago

@rparrett this doesn't seem to be the case (v0.9)

use bevy::prelude::*;

pub fn main() {
    App::default()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    commands.spawn(Camera3dBundle {
        transform: Transform::from_xyz(5.0, 5.0, 5.0).looking_at(Vec3::ZERO, Vec3::Y),
        ..default()
    });

    commands.spawn(PointLightBundle {
        point_light: PointLight {
            intensity: 1500.0,
            shadows_enabled: true,
            ..default()
        },
        transform: Transform::from_xyz(-4.0, 8.0, 4.0),
        ..default()
    });

    let color = Color::rgb(0.8, 0.2, 0.2);

    // Uses `base_color` on the `StandardMaterial`
    commands.spawn(PbrBundle {
        mesh: meshes.add(Mesh::from(shape::Cube::default())),
        material: materials.add(color.into()),
        transform: Transform::from_translation((1.0, 0.0, 0.0).into()),
        ..Default::default()
    });

    // In theory should just set `color` on the vert, computation is here:
    // https://github.com/bevyengine/bevy/blob/v0.9.1/crates/bevy_pbr/src/render/pbr.wgsl#L20
    // And `default()` is using WHITE in sRGB space:
    // https://github.com/bevyengine/bevy/blob/v0.9.1/crates/bevy_pbr/src/pbr_material.rs#L233
    let mut vert_color_mesh = Mesh::from(shape::Cube::default());
    vert_color_mesh.insert_attribute(Mesh::ATTRIBUTE_COLOR, vec![color.as_rgba_f32(); 24]);

    commands.spawn(PbrBundle {
        mesh: meshes.add(vert_color_mesh),
        material: materials.add(StandardMaterial::default()),
        transform: Transform::from_translation((-1.0, 0.0, 0.0).into()),
        ..Default::default()
    });
}

This happens on Windows and WebGL.

image

Looking through both the shaders code and the Color conversions (as well as defaults) I cannot for the life of me figure out what's causing it. The vertex_colors example exabits this same behavior.

AThilenius commented 1 year ago

Ah, found it. I don't know enough of Bevy yet to say if this is 'intended behavior' but I will say it's quite confusing to have two default color spaces for uniform vs. vertex attributes. The default into() for Color -> [f32; 4] is also as_rgba_f32.

tim-blackbird commented 1 year ago

The default into() for Color -> [f32; 4] is also as_rgba_f32.

I noticed this as well while working on #6529. I think we should consider removing the From impl to make the choice explicit.