Re-introduce "Replace Ambient Lights with Environment Map Lights (#17482)"#18207
Re-introduce "Replace Ambient Lights with Environment Map Lights (#17482)"#18207SparkyPotato wants to merge 29 commits into
Conversation
…evyengine#17482)" …" This reverts commit 54701a8.
|
Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke! If it's expected, please add the M-Deliberate-Rendering-Change label. If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it. |
alice-i-cecile
added
A-Rendering
alice-i-cecile
added
M-Deliberate-Rendering-Change
|
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? |
Can you say more about this? Has this been discussed elsewhere? I'd really like to do this, but we need clear rationale for changing how user scenes are rendered. |
|
Does that mean we'll change again how every scene renders in the next version? |
mockersf
left a comment
mockersf
left a comment
There was a problem hiding this comment.
Example code shouldn't use deprecated things, they need to be updated to use the new component.
|
this would also close #17367 |
|
Would it make sense to rename the Conceptually, it is ambient light, right? Isn't it just being implemented with an environment map? |
It was discussed on discord.
There's no plan that I'm aware of to do other changes related to this. The main goal is still to yeet the non physical ambient light but have an easy way to still emulate an ambient light with less overall shader code to maintain.
No, because the |
|
is there an explanation why some scenes are darker (https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/9021/compare/9009?screenshot=3D+Rendering/ssao.png or https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/9021/compare/9009?screenshot=3D+Rendering/pbr.png) and some are lighter (https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/9021/compare/9009?screenshot=3D+Rendering/spotlight.png or https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/9021/compare/9009?screenshot=3D+Rendering/ssao.png)? on https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/9021/compare/9009?screenshot=3D+Rendering/anisotropy.png some details completely disappear. for example the dark bar between the light filaments that should be there when looking at https://github.com/KhronosGroup/glTF-Sample-Assets/tree/main/Models/AnisotropyBarnLamp |
Certain scenes are darker since they had both an ambient light and an environment map, but now the ambient light is overridden by the environment map. For scenes that are lighter, this is because the ambient light had no effect on specular reflections, whereas the environment map does.
Examples that modified the ambient light have been updated to use environment map lights instead, but I haven't modified any that relied on the default resource. That change should probably be done when we remove |
|
I feel like we should make some effort to preserve the scenes of our users. If the new light is brighter because it is more physically correct then it seems like the default intensity should be lower. Or we at least need to tell users what happened and how to fix their scenes. Just linking for reference, the PR that set the current default ambient intensity: 11868. |
There are still examples that use ambient light, they should be fixed now |
|
I also think that if we deprecate |
This makes sense, but I'm not sure how to replace that with a default environment map, because the new constructors require an While I do think that having no lights by default is a good idea, as it forces users to be deliberate with their lighting setup, it does hurt quick prototyping. |
|
can't you create a default image on startup to use for the default environment map? |
|
bevy already has this 1x1 default images bevy/crates/bevy_render/src/texture/mod.rs Lines 24 to 30 in 3b9e2e6 other locations with images created on setup bevy/crates/bevy_core_pipeline/src/post_process/mod.rs Lines 56 to 61 in 3b9e2e6 bevy/crates/bevy_core_pipeline/src/smaa/mod.rs Lines 86 to 90 in 3b9e2e6 |
|
yup you can static embed small assets, a 1x1 cubemap shouldn't use a lot of memory |
alice-i-cecile
added
S-Ready-For-Final-Review
| App::new() | ||
| .insert_resource(AmbientLight::NONE) | ||
| .add_plugins(DefaultPlugins) | ||
| .add_plugins(DefaultPlugins.set(bevy::pbr::PbrPlugin { |
There was a problem hiding this comment.
I dislike that we need to reach in to plugin config to override a "camera default". Semantically in most cases I think people should be saying "I dont want this camera to have the default". We are composing scenes here, and Plugin settings are not scene properties. This design choice was almost certainly a product of required component behavior (which you currently can't disable for a specific spawn ... and I'm not convinced we ever want that). I would almost prefer camera.insert(EnvironmentMapLight { intensity: 0, ..default()}) for the common case. That would mean we'd be rendering environment maps when we don't need them though. Making EnvironmentMapLight an enum probably makes sense. See my next comment for more motivation.
| // environment lights' brightnesses are measured in candela per meter square, calculable as (color * intensity) | ||
| EnvironmentMapLight { | ||
| intensity: 80.0, | ||
| ..EnvironmentMapLight::solid_color(&mut images, ORANGE_RED.into()) |
There was a problem hiding this comment.
This unnecessarily regresses the UX of the (very) common case "I want colored ambient light". As others have mentioned, it also makes doing things like animating the color more challenging.
In light of this (and my comment above, and the desired EnvironmentLight rename), I think we should make EnvironmentLight an enum:
enum EnvironmentLight {
CubeMap {
diffuse_map: Handle<Image>,
intensity: f32,
/* other fields */
},
Color {
color: Color,
intensity: f32,
},
Disabled,
}I think this should be done before 0.17 to make this a "pleasant" change, ease migration, and avoid double breakage. We can then also remove all of the bevy::pbr::PbrPlugin { default_environment_map_light: false } nastiness in favor of camera.insert(EnvironmentLight::Disabled)
|
I'm cool with merging this now, provided we block 0.17 on the changes above. |
|
punting this to 0.18, will write up an issue on the path forward |
|
(We had a Discord conversation here with some context) |
alice-i-cecile
added
S-Waiting-on-Author
|
Iiuc we should probably have environment/dome light type that encapsulates an enum: Color/AmbientLight, EnvironmetTexture, SkyTexture, something else. You can configure this light with only one option but can have multiple such lights with different options in a scene. Related:
|
|
|
||
| impl EnvironmentMapLight { | ||
| /// An environment map with a uniform color, useful for uniform ambient lighting. | ||
| pub fn solid_color(assets: &mut Assets<Image>, color: Color) -> Self { |
There was a problem hiding this comment.
| pub fn solid_color(assets: &mut Assets<Image>, color: Color) -> Self { | |
| pub fn solid_color(assets: &mut Assets<Image>, color: impl Into<Color>) -> Self { |
so that e.g. tailwind::AMBER_100 works :)
| top_color: Color, | ||
| mid_color: Color, | ||
| bottom_color: Color, |
There was a problem hiding this comment.
plz turn these into impl Into<Color> too
# Objective - Add EnvironmentMapLight creation helper functions ## Solution - Rip them from #18207 ## Testing - tested in linked PR
alice-i-cecile
added
S-Adopt-Me
14 participants
#17482 was reverted due to unintentional changes in ambient light rendering.
However, after further inspection, that was due to environment maps being more physically-correct, so the changes are positive.
Closes #17367.