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 fog color being inaccurate #10226

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Conversation

rafalh
Copy link
Contributor

@rafalh rafalh commented Oct 22, 2023

Objective

Fog color was passed to shaders without conversion from sRGB to linear color space. Because shaders expect colors in linear space this resulted in wrong color being used. This is most noticeable in open scenes with dark fog color and clear color set to the same color. In such case background/clear color (which is properly processed) is going to be darker than very far objects.

Example:
image
bevy-fog-color-bug.zip

Solution

Add missing conversion of fog color to linear color space.


Changelog

  • Fixed conversion of fog color

Migration Guide

  • Colors in FogSettings struct (color and directional_light_color) are now sent to the GPU in linear space. If you were using Color::rgb()/Color::rgba() and would like to retain the previous colors, you can quickly fix it by switching to Color::rgb_linear()/Color::rgba_linear().

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fog color was passed to shaders without conversion from sRGB to linear
color space. Because shaders expect colors in linear space this resulted
in wrong color being used. Fix it by adding missing conversion.
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Oct 22, 2023
@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 22, 2023
@superdump
Copy link
Contributor

I think @coreh should comment on whether fog colour is supposed to be specified as non-linear sRGB or linear. If it is meant to be specified as linear then no conversion should be done.

Copy link
Contributor

@coreh coreh left a comment

Choose a reason for hiding this comment

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

Hey, nice catch! I think what got me confused when setting this up originally is that with hdr: false the clear color is not tonemapped, only the meshes, so I could never get the fog to match it, and assumed the issue was with the clear color and not the fog. But for hdr: true it should now match perfectly.

Before we merge this, can you update the examples, so that they don't look darker with the new (correct) color format being sent to the GPU?

It should be as simple as replacing Color::rgb and Color::rgba with Color::rgb_linear and Color::rgba_linear.

examples/fog.rs

FogSettings {
    color: Color::rgba_linear(0.05, 0.05, 0.05, 1.0),
    falloff: FogFalloff::Linear {
        start: 5.0,
        end: 20.0,
    },
    ..default()
},

examples/atmospheric_fog.rs

FogSettings {
    color: Color::rgba_linear(0.1, 0.2, 0.4, 1.0),
    directional_light_color: Color::rgba_linear(1.0, 0.95, 0.75, 0.5),
    directional_light_exponent: 30.0,
    falloff: FogFalloff::from_visibility_colors(
        15.0, // distance in world units up to which objects retain visibility (>= 5% contrast)
        Color::rgb(0.35, 0.5, 0.66), // atmospheric extinction color (after light is lost due to absorption by atmospheric particles)
        Color::rgb(0.8, 0.844, 1.0), // atmospheric inscattering color (light gained due to scattering from the sun)
    ),
},

(I double checked it and in this second one, we don't need to change the values in FogFalloff since it was already correctly using linear space internally to calculate extinction and inscattering.

I believe we also need a migration guide for this. Something like: “Fog color and directional_light_color are now sent to the GPU in linear space. If you were using Color::rgb()/Color::rgba() and would like to retain the previous colors, you can quickly fix it by switching to Color::rgb_linear()/Color::rgba_linear().”

@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 22, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

Make examples look the same as before fog color conversion was fixed.
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 23, 2023
@alice-i-cecile
Copy link
Member

Great work, thank you very much :) And thanks @coreh for the high quality review!

@rafalh
Copy link
Contributor Author

rafalh commented Oct 23, 2023

I adjusted configuration in examples to match the previous look and added migration info. I agree it may be helpful.
I didn't replace Color::rgba calls to Color::rgba_linear because I believe we shouldn't encourage people to use linear space outside of shaders. It is less intuitive to set up and for me it is a technical detail that shaders do calculations in linear space. Let me know if you disagree and I can change examples to linear values.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 23, 2023
Merged via the queue into bevyengine:main with commit 51c70bc Oct 23, 2023
22 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

Fog color was passed to shaders without conversion from sRGB to linear
color space. Because shaders expect colors in linear space this resulted
in wrong color being used. This is most noticeable in open scenes with
dark fog color and clear color set to the same color. In such case
background/clear color (which is properly processed) is going to be
darker than very far objects.

Example:

![image](https://github.com/bevyengine/bevy/assets/160391/89b70d97-b2d0-4bc5-80f4-c9e8b8801c4c)

[bevy-fog-color-bug.zip](https://github.com/bevyengine/bevy/files/13063718/bevy-fog-color-bug.zip)

## Solution

Add missing conversion of fog color to linear color space.

---

## Changelog

* Fixed conversion of fog color

## Migration Guide

- Colors in `FogSettings` struct (`color` and `directional_light_color`)
are now sent to the GPU in linear space. If you were using
`Color::rgb()`/`Color::rgba()` and would like to retain the previous
colors, you can quickly fix it by switching to
`Color::rgb_linear()`/`Color::rgba_linear()`.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Fog color was passed to shaders without conversion from sRGB to linear
color space. Because shaders expect colors in linear space this resulted
in wrong color being used. This is most noticeable in open scenes with
dark fog color and clear color set to the same color. In such case
background/clear color (which is properly processed) is going to be
darker than very far objects.

Example:

![image](https://github.com/bevyengine/bevy/assets/160391/89b70d97-b2d0-4bc5-80f4-c9e8b8801c4c)

[bevy-fog-color-bug.zip](https://github.com/bevyengine/bevy/files/13063718/bevy-fog-color-bug.zip)

## Solution

Add missing conversion of fog color to linear color space.

---

## Changelog

* Fixed conversion of fog color

## Migration Guide

- Colors in `FogSettings` struct (`color` and `directional_light_color`)
are now sent to the GPU in linear space. If you were using
`Color::rgb()`/`Color::rgba()` and would like to retain the previous
colors, you can quickly fix it by switching to
`Color::rgb_linear()`/`Color::rgba_linear()`.
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants