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

Change light defaults & fix light examples #11581

Merged
merged 23 commits into from
Feb 14, 2024
Merged

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Jan 28, 2024

Objective

Fix #11577.

Solution

Fix the examples, add a few constants to make setting light values easier, and change the default lighting settings to be more realistic. (Now designed for an overcast day instead of an indoor environment)


I did not include any example-related changes in here.

Changelogs (not including breaking changes)

bevy_pbr

  • Added light_consts module (included in prelude), which contains common lux and lumen values for lights.
  • Added AmbientLight::NONE constant, which is an ambient light with a brightness of 0.
  • Added non-EV100 variants for ExposureSettings's EV100 constants, which allow easier construction of an ExposureSettings from a EV100 constant.

Breaking changes

bevy_pbr

The several default lighting values were changed:

  • PointLight's default intensity is now 2000.0
  • SpotLight's default intensity is now 2000.0
  • DirectionalLight's default illuminance is now light_consts::lux::OVERCAST_DAY (1000.)
  • AmbientLight's default brightness is now 20.0

@mockersf
Copy link
Member

to fix #11577, you should also change the defaults so that the default values for lights work with the default values for exposure

@doonv
Copy link
Contributor Author

doonv commented Jan 28, 2024

you should also change the defaults so that the default values for lights work with the default values for exposure

I can't really, the default camera's exposure is designed for full sunlight while the point lights act similarly to real lightbulbs. And because of that the point lights are really dim. Theres nothing I can really do to make everything look good without compromising on realism or visual quality.

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Jan 28, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I really like the addition of the human-scale constants. I can give these examples a look before the release to give you the final approval if needed too,

@mockersf
Copy link
Member

mockersf commented Jan 28, 2024

you should also change the defaults so that the default values for lights work with the default values for exposure

I can't really, the default camera's exposure is designed for full sunlight while the point lights act similarly to real lightbulbs. And because of that the point lights are really dim. Theres nothing I can really do to make everything look good without compromising on realism or visual quality.

The default ExposureSettings is EV100_INDOOR, is that full sunlight?

impl Default for ExposureSettings {
fn default() -> Self {
Self {
ev100: Self::EV100_INDOOR,
}
}
}

If the default exposure is for the full sunlight, then the default directional light should be full sunlight, and the one used in examples. otherwise we should change the default exposure to be indoor so that it works correctly with point light, and use point lights in the examples

the default for exposure and for lights should make sense together, and be used in examples

@doonv
Copy link
Contributor Author

doonv commented Jan 28, 2024

ah my bad, I just assumed it would be for full sunlight (why else would the point lights be that bright).

@doonv
Copy link
Contributor Author

doonv commented Jan 28, 2024

I do think we should use EV100_OVERCAST instead, as it's more a middleground between indoor lighting and outdoor lighting

@mockersf
Copy link
Member

I do think we should use EV100_OVERCAST instead, as it's more a middleground between indoor lighting and outdoor lighting

no strong opinion on my side, as long as the defaults make sense together 🙂

@alice-i-cecile
Copy link
Member

I like the overcast as a default, and agreed that we should ensure all of our default values look sensible together.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Examples An addition or correction to our examples C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 28, 2024
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.

@doonv doonv changed the title Fix light examples Change light defaults & fix light examples Jan 29, 2024
@doonv doonv marked this pull request as ready for review January 29, 2024 13:58
Copy link
Contributor

@GitGhillie GitGhillie left a comment

Choose a reason for hiding this comment

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

I don't have time to fully review this PR right now (the consts seem like a good addition though) but I do know some examples still need adjusting. Here's a list of all the examples with lights that I know of:

Examples list

Category Name
3d 3d_gizmos
3d 3d_scene
3d 3d_shapes
3d 3d_viewport_to_world
3d anti_aliasing
3d atmospheric_fog
3d blend_modes
3d bloom_3d
3d deferred_rendering
3d fog
3d generate_custom_mesh
3d lighting
3d lines
3d load_gltf
3d orthographic
3d parallax_mapping
3d parenting
3d pbr
3d render_to_texture
3d shadow_biases
3d shadow_caster_receiver
3d skybox
3d spherical_area_lights
3d split_screen
3d spotlight
3d ssao
3d tonemapping
3d transmission
3d transparency_3d
3d two_passes
3d update_gltf_scene
3d vertex_colors
3d wireframe
animation animated_fox
animation animated_transform
animation cubic_curve
animation custom_skinned_mesh
animation gltf_skinned_mesh
animation morph_targets
asset asset_loading
asset hot_asset_reloading
audio spatial_audio_3d
ecs iter_combinations
games alien_cake_addict
mobile bevy_mobile_example
shader array_texture
shader extended_material
shader post_processing
shader shader_material_screenspace_texture
shader shader_prepass
stress_tests many_cubes
stress_tests many_foxes
stress_tests many_lights
tools scene_viewer
ui render_to_texture
transforms 3d_rotation
transforms scale
transforms transform
transforms translation
window low_power
window multiple_windows
window screenshot

@GitGhillie
Copy link
Contributor

GitGhillie commented Jan 29, 2024

For those who haven't seen it this was the reasoning behind changing the default exposure to indoors: #8407 (comment)
But in the end the default exposure is pretty much arbitrary + small effort to add ExposureSettings::INDOOR so I don't have a strong opinion there

@doonv
Copy link
Contributor Author

doonv commented Jan 30, 2024

For those who haven't seen it this was the reasoning behind changing the default exposure to indoors: #8407 (comment) But in the end the default exposure is pretty much arbitrary + small effort to add ExposureSettings::INDOOR so I don't have a strong opinion there

I like the reasoning used there, so I think I'll revert the change and use INDOOR again.

@GitGhillie
Copy link
Contributor

GitGhillie commented Feb 2, 2024

While I think this PR is a step in the right direction if I follow the steps listed here I'm not sure it actually fixes #11577. Maybe there is a deeper issue? Edit: Or the distances to the pointlights before this PR were just large

@doonv
Copy link
Contributor Author

doonv commented Feb 2, 2024

The deeper issue is that the examples are trying to use point lights as a sun, instead of a DirectionalLight. And obviously your average bulb light isn't as powerful as the sun.

@JMS55
Copy link
Contributor

JMS55 commented Feb 5, 2024

What's the current state of this PR?

@doonv
Copy link
Contributor Author

doonv commented Feb 5, 2024

What's the current state of this PR?

Ready to merge I think. But I might have missed examples outside of the 3d examples. I'll check the other examples later.

Copy link
Contributor

@GitGhillie GitGhillie 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 to me overall, just some minor notes here and there.

I do feel like the directional lights make the examples look less appealing in general. See for example cubic_curve where the ground plane turns from a nice gradient to a single color:
image
Not sure how I feel about it yet. Not a blocker though!

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
examples/3d/bloom_3d.rs Outdated Show resolved Hide resolved
examples/3d/fog.rs Outdated Show resolved Hide resolved
examples/3d/lightmaps.rs Outdated Show resolved Hide resolved
examples/3d/spherical_area_lights.rs Outdated Show resolved Hide resolved
examples/3d/transmission.rs Show resolved Hide resolved
examples/3d/transparency_3d.rs Show resolved Hide resolved
examples/3d/wireframe.rs Outdated Show resolved Hide resolved
pub const INDOOR: Self = Self {
ev100: Self::EV100_INDOOR,
};

pub const EV100_SUNLIGHT: f32 = 15.0;
pub const EV100_OVERCAST: f32 = 12.0;
pub const EV100_INDOOR: f32 = 7.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth creating several constants for indoor lighting. The linked wikipedia article says:

Sports events, stage shows, and the like = 8–9
Offices and work areas = 7–8
Home interiors = 5–7

Copy link
Member

Choose a reason for hiding this comment

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

I kind of like the current granularity. Something like "office" feels too specific to be correct. All of these will be +- 2 or 3. Anyone manually setting exposure will likely be doing it artistically / without exact constants anyway.

@cart cart added this pull request to the merge queue Feb 14, 2024
Merged via the queue into bevyengine:main with commit dc9b486 Feb 14, 2024
23 checks passed
@cart
Copy link
Member

cart commented Feb 14, 2024

I do feel like the directional lights make the examples look less appealing in general. See for example cubic_curve where the ground plane turns from a nice gradient to a single color:

I do agree with this. I think (for now) I'll revert these in most cases in the interest of render quality. Both the shadows and shading take a quality / interest-level hit.

github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
# Objective

After adding configurable exposure, we set the default ev100 value to
`7` (indoor). This brought us out of sync with Blender's configuration
and defaults. This PR changes the default to `9.7` (bright indoor or
very overcast outdoors), as I calibrated in #11577. This feels like a
very reasonable default.

The other changes generally center around tweaking Bevy's lighting
defaults and examples to play nicely with this number, alongside a few
other tweaks and improvements.

Note that for artistic reasons I have reverted some examples, which
changed to directional lights in #11581, back to point lights.
 
Fixes #11577 

---

## Changelog

- Changed `Exposure::ev100` from `7` to `9.7` to better match Blender
- Renamed `ExposureSettings` to `Exposure`
- `Camera3dBundle` now includes `Exposure` for discoverability
- Bumped `FULL_DAYLIGHT ` and `DIRECT_SUNLIGHT` to represent the
middle-to-top of those ranges instead of near the bottom
- Added new `AMBIENT_DAYLIGHT` constant and set that as the new
`DirectionalLight` default illuminance.
- `PointLight` and `SpotLight` now have a default `intensity` of
1,000,000 lumens. This makes them actually useful in the context of the
new "semi-outdoor" exposure and puts them in the "cinema lighting"
category instead of the "common household light" category. They are also
reasonably close to the Blender default.
- `AmbientLight` default has been bumped from `20` to `80`.

## Migration Guide

- The increased `Exposure::ev100` means that all existing 3D lighting
will need to be adjusted to match (DirectionalLights, PointLights,
SpotLights, EnvironmentMapLights, etc). Or alternatively, you can adjust
the `Exposure::ev100` on your cameras to work nicely with your current
lighting values. If you are currently relying on default intensity
values, you might need to change the intensity to achieve the same
effect. Note that in Bevy 0.12, point/spot lights had a different hard
coded ev100 value than directional lights. In Bevy 0.13, they use the
same ev100, so if you have both in your scene, the _scale_ between these
light types has changed and you will likely need to adjust one or both
of them.
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-Examples An addition or correction to our examples C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default point lights seems wrong after exposure
9 participants