Replace Ambient Lights with Environment Map Lights #17482

SparkyPotato · 2025-01-21T22:51:40Z

Objective

Transparently uses simple EnvironmentMapLights to mimic AmbientLights. Implements the first part of #17468, but I can implement hemispherical lights in this PR too if needed.

Solution

A function EnvironmentMapLight::solid_color(&mut Assets<Image>, Color) is provided to make an environment light with a solid color.
A new system is added to SimulationLightSystems that maps AmbientLights on views or the world to a corresponding EnvironmentMapLight.

I have never worked with (or on) Bevy before, so nitpicky comments on how I did things are appreciated :).

Testing

Testing was done on a modified version of the 3d/lighting example, where I removed all lights except the ambient light. I have not included the example, but can if required.

Migration

bevy_pbr::AmbientLight has been deprecated, so all usages of it should be replaced by a bevy_pbr::EnvironmentMapLight created with EnvironmentMapLight::solid_color placed on the camera. There is no alternative to ambient lights as resources.

github-actions · 2025-01-21T22:51:51Z

Welcome, new contributor!

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

IceSentry · 2025-01-21T22:55:35Z

crates/bevy_pbr/src/light/mod.rs

@@ -522,6 +523,26 @@ pub enum SimulationLightSystems {
    CheckLightVisibility,
 }

+pub fn map_ambient_lights(


I think we may want to add a deprecation notice to the AmbientLight component and remove it after 0.16. For this PR I think this is fine as is though.

+1 to adding a deprecated attribute to ambient light, and then removing this system in 0.17.

EDIT: I mean adding the attribute in this PR, and then opening an issue to remind us to delete this system, and have put it in the 0.17 milestone.

IceSentry

This is great, thank you!

IceSentry · 2025-01-21T23:06:52Z

I can implement hemispherical lights in this PR too if needed

The PR is simple enough that you could add this in too I think. Up to you, but I would approve it either way.

SparkyPotato · 2025-01-21T23:46:06Z

I can implement hemispherical lights in this PR too if needed

The PR is simple enough that you could add this in too I think. Up to you, but I would approve it either way.

Might as well add it in then.

robtfm · 2025-01-21T23:47:39Z

code looks good.

simplifying the shaders is nice, but is the cost of binding an extra texture per view and looking up for every pixel really worth the saving of 7-8 lines of shader code?

SparkyPotato · 2025-01-22T00:58:50Z

Found a bug with user-provided environment maps being overridden by the default ambient light, so I had to complicate the sync system a bit. Not sure if what I did was the most optimal.

code looks good.

simplifying the shaders is nice, but is the cost of binding an extra texture per view and looking up for every pixel really worth the saving of 7-8 lines of shader code?

The lighting example does go from about 0.51 ms to 0.55 ms on my machine (and enabling deferred makes that 0.46 ms to 0.49 ms), but I do doubt the usage of ambient light will be widespread enough that this matters, especially after the new procedural sky rolls around.

IceSentry · 2025-01-22T02:16:15Z

simplifying the shaders is nice, but is the cost of binding an extra texture per view and looking up for every pixel really worth the saving of 7-8 lines of shader code?

I guess what we could do is keep the constructor thing in this PR and do the ambient light thing separately? I can see why that part could be a bit more controversial and that people might still want that around.

pcwalton · 2025-01-22T02:17:57Z

It's not just about simplifying the shader code: it's that the interaction between ambient and environment map light never made any sense. They should be considered basically mutually exclusive (strictly speaking, ambient should be considered a type of light probe, which this PR moves us toward).

mate-h · 2025-01-22T09:21:24Z

crates/bevy_pbr/src/light_probe/environment_map.rs

+                .into_iter()
+                .flat_map(Srgba::to_u8_array)
+                .collect(),
+                TextureFormat::Rgba8UnormSrgb,


This does not need to be a HDR texture right, since the user is defining the color in sRGB and an intensity?

Yeah, it's an 8-bit sRGB texture.

robtfm · 2025-01-22T12:22:14Z

The lighting example does go from about 0.51 ms to 0.55 ms on my machine (and enabling deferred makes that 0.46 ms to 0.49 ms)

is that in release? if so then i'd argue a ~10% fps reduction is not justified, but it's surprising how large the impact is.

They should be considered basically mutually exclusive (strictly speaking, ambient should be considered a type of light probe, which this PR moves us toward).

makes sense. if the impact is really this big then perhaps we need some investigation into env map perf before going further though?

SparkyPotato · 2025-01-22T14:33:52Z

The lighting example does go from about 0.51 ms to 0.55 ms on my machine (and enabling deferred makes that 0.46 ms to 0.49 ms)

is that in release? if so then i'd argue a ~10% fps reduction is not justified, but it's surprising how large the impact is.

In release, the difference is 0.49 ms vs 0.53 ms, but the deferred timings don't change.

They should be considered basically mutually exclusive (strictly speaking, ambient should be considered a type of light probe, which this PR moves us toward).

makes sense. if the impact is really this big then perhaps we need some investigation into env map perf before going further though?

I'd think ambient lights are usually used only in simple scenes where we'd be CPU bound anyways. For anything more complex, users would probably switch to an environment map or one baked by the procedural sky.

SparkyPotato · 2025-01-23T01:50:17Z

I decided to profile the case without an ambient light or envmap, which takes 0.47 ms on this branch.

This means we'll actually be slightly faster in scenes with an envmap after this PR. For example:

the deferred rendering example goes from 1.16 to 1.12 ms
SSR goes from 0.92 to 0.89 ms
post processing goes from 1.02 to 0.96 ms
meshlet goes from 1.68 to 1.67 ms
color grading goes from 0.88 to 0.87 ms

Note that these are all overall frametimes and not individual pass timings like that of the lighting example (for lighting, the overall frame went from 0.98 to 0.96 ms without any ambient or env light).

JMS55

Code looks fine. Needs some docs, as well as a changelog and migration guide in the PR description. Ask on discord how to do that, I think we changed our process recently and I forget what the new system is.

JMS55 · 2025-01-26T22:41:34Z

crates/bevy_pbr/src/light_probe/environment_map.rs

@@ -114,6 +116,80 @@ pub struct EnvironmentMapLight {
    pub affects_lightmapped_mesh_diffuse: bool,
 }

+impl EnvironmentMapLight {


Needs docs for the new functions. Esp since EnviornmentMapLight is supposed to have diffuse/specular components that get run through a bunch of sampling and multiscattering stuff, having a single "color" is a bit confusing. Please add some docs and I guess note that it's a bit of a hack? Something along these lines.

JMS55 · 2025-01-26T22:43:14Z

crates/bevy_pbr/src/light/mod.rs

@@ -522,6 +523,26 @@ pub enum SimulationLightSystems {
    CheckLightVisibility,
 }

+pub fn map_ambient_lights(


+1 to adding a deprecated attribute to ambient light, and then removing this system in 0.17.

EDIT: I mean adding the attribute in this PR, and then opening an issue to remind us to delete this system, and have put it in the 0.17 milestone.

SparkyPotato added 2 commits January 21, 2025 22:43

replace ambient light with environment lights

cffc7cc

fix ambient light change detection

fce2d88

IceSentry reviewed Jan 21, 2025

View reviewed changes

IceSentry approved these changes Jan 21, 2025

View reviewed changes

IceSentry added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 21, 2025

IceSentry requested review from pcwalton, superdump and robtfm January 21, 2025 23:03

fix accidental env map override and add hemispherical map

69ec194

appease clippy

e767ce5

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jan 22, 2025

BenjaminBrienen added D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Shaders This code uses GPU shader languages labels Jan 22, 2025

mate-h reviewed Jan 22, 2025

View reviewed changes

JMS55 suggested changes Jan 26, 2025

View reviewed changes

SparkyPotato added 2 commits January 26, 2025 23:17

deprecate AmbientLight and add docs

77532e3

replace allow with expect

7f8f0b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Ambient Lights with Environment Map Lights #17482

Replace Ambient Lights with Environment Map Lights #17482

SparkyPotato commented Jan 21, 2025 •

edited

Loading

github-actions bot commented Jan 21, 2025

IceSentry Jan 21, 2025

JMS55 Jan 26, 2025 •

edited

Loading

IceSentry left a comment

IceSentry commented Jan 21, 2025

SparkyPotato commented Jan 21, 2025

robtfm commented Jan 21, 2025

SparkyPotato commented Jan 22, 2025

IceSentry commented Jan 22, 2025

pcwalton commented Jan 22, 2025

mate-h Jan 22, 2025

SparkyPotato Jan 22, 2025

robtfm commented Jan 22, 2025

SparkyPotato commented Jan 22, 2025 •

edited

Loading

SparkyPotato commented Jan 23, 2025

JMS55 left a comment

JMS55 Jan 26, 2025

JMS55 Jan 26, 2025 •

edited

Loading

Replace Ambient Lights with Environment Map Lights #17482

Are you sure you want to change the base?

Replace Ambient Lights with Environment Map Lights #17482

Conversation

SparkyPotato commented Jan 21, 2025 • edited Loading

Objective

Solution

Testing

Migration

github-actions bot commented Jan 21, 2025

IceSentry Jan 21, 2025

Choose a reason for hiding this comment

JMS55 Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

IceSentry left a comment

Choose a reason for hiding this comment

IceSentry commented Jan 21, 2025

SparkyPotato commented Jan 21, 2025

robtfm commented Jan 21, 2025

SparkyPotato commented Jan 22, 2025

IceSentry commented Jan 22, 2025

pcwalton commented Jan 22, 2025

mate-h Jan 22, 2025

Choose a reason for hiding this comment

SparkyPotato Jan 22, 2025

Choose a reason for hiding this comment

robtfm commented Jan 22, 2025

SparkyPotato commented Jan 22, 2025 • edited Loading

SparkyPotato commented Jan 23, 2025

JMS55 left a comment

Choose a reason for hiding this comment

JMS55 Jan 26, 2025

Choose a reason for hiding this comment

JMS55 Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

SparkyPotato commented Jan 21, 2025 •

edited

Loading

JMS55 Jan 26, 2025 •

edited

Loading

SparkyPotato commented Jan 22, 2025 •

edited

Loading

JMS55 Jan 26, 2025 •

edited

Loading