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

Exposure settings #8407

Closed
wants to merge 16 commits into from
Closed

Conversation

superdump
Copy link
Contributor

Objective

Solution


Changelog

  • Added: ExposureSettings component that can be added to an entity with a Camera to configure PBR exposure settings

Migration Guide

  • The ExposureSettings component on an Camera entity will be used to configure exposure for PBR rendering. Previously only directional lights were using hard-coded exposure, but now all light has exposure applied. This will mean that all light sources other than directional lights will be significantly darker. Adjust the exposure settings and light intensities as necessary.

@superdump superdump marked this pull request as draft April 16, 2023 20:39
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Apr 16, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 16, 2023
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 16, 2023
@alice-i-cecile
Copy link
Member

Adding to the 0.11 milestone as this change will be badly breaking and should be shipped sooner rather than later.

Comment on lines 89 to 91
aperture_f_stops: 4.0,
shutter_speed_s: 1.0 / 250.0,
sensitivity_iso: 100.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest ditching these individual settings and just passing the ev100 value directly. There's lots of sources online that provide tables of ev100 values, for instance this one from wikipedia

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, might be worth providing constants with a couple defaults. For instance, EXPOSURE_DIRECT_SUNLIGHT, EXPOSURE_INDOOR_LIGHTING, EXPOSURE_MOONLIGHT, etc.

Copy link
Contributor

@JMS55 JMS55 Apr 16, 2023

Choose a reason for hiding this comment

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

Agreed! You could also provide an Ev100::from_camera(aperture_f_stops, shutter_speed_s, etc...) function.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, unless we had other effects of these parameters they should really be collapsed down into one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea @JMS55 - I was thinking that it would be good to expose (HA!!!) the functionality to provide the parameters for those who it is more familiar. Was first thinking of an enum, but I agree the constructor approach could be nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alice-i-cecile hmm... now that you mention it... the filament docs did talk about how the different parameters affect depth of field (or focal depth - the aperture setting I think, I'm not familiar with cameras beyond physics stuff) and motion blur (shutter speed).

Copy link
Member

Choose a reason for hiding this comment

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

That's correct :) At the same time, I don't think we want those baked into our rendering engine by default: motion blur and focal depth are very specific effects that should be used artistically.

We're not simulating a real camera IMO, so we should just expose Ev100 and then provide independent control for things like motion blur and focal depth when we have them.

@fintelia
Copy link
Contributor

fintelia commented Apr 16, 2023

Skyboxes and environment lights may need changes as well, though I haven't looked into them in much detail.

Edit: Environment lights seem to be handled correctly, but the output from skyboxes still needs to be scaled by view.exposure (and probably a user-controllable scale factor so LDR skyboxes can be brighter than 1 nit).

@superdump
Copy link
Contributor Author

Skyboxes and environment lights may need changes as well, though I haven't looked into them in much detail

The filament docs said that they pre-expose IBL sources too, as an alternative to just applying exposure to accumulated light from all sources. They do the pre-exposure for the sake of being able to use f16. I may switch over to trying to do individual source pre-exposure for that reason but I wanted to get the whole thing working first.

@fintelia do you feel like trying out my branch to see if it matches your expectations? To me with the default values that Filament suggests, everything is very dark. I tried to find suggested values for cameras via random searches but they also seemed very dim. I feel I need to read through everything and probably compare to a reference scene + rendered in something else that is reliable.

@fintelia
Copy link
Contributor

Just tried out the 3d lighting example from this branch. I removed the ambient and directional light, changed all the other lights to be white, and set the exposure settings to have an EV=6.0. The result looks fairly reasonable to me for a scene with 3x 100W non-halogen incandescent bulbs:

image

@@ -207,6 +207,7 @@ fn setup(
// directional 'sun' light
commands.spawn(DirectionalLightBundle {
directional_light: DirectionalLight {
illuminance: 1000.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

The sun has an illuminance of ~100,000 lux. Though, having that bright a directional light that bright is of course going to overpower all the other lights in the scene (even stuff in shadows will mostly be lit by bounce lighting and light from the sky)

@@ -254,7 +254,7 @@ fn pbr(

// Total light
output_color = vec4<f32>(
direct_light + indirect_light + emissive_light,
view.exposure * (direct_light + indirect_light + emissive_light),
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that indirect light is already in photometric units. There may need to be a scale factor to undo any pre-exposure that's baked into the contents of the cubemaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. There was talk about an "intensity" field for environment map lighting. I decided to leave it out in my original PR because I was worried it would be abused. Maybe it's time for that now.

@JMS55 JMS55 modified the milestones: 0.11, 0.12 Jun 11, 2023
@JMS55
Copy link
Contributor

JMS55 commented Aug 22, 2023

@superdump rebased, converted ExposureSettings to hold EV100 directly, and did some misc work superdump#32.

Remaining TODO:

  • Add an exposure_correction/intensity/pre_exposure kinda field to EnvironmentMapLight
  • Add an exposure_correction/intensity/pre_exposure kinda field to Skybox
  • Apply exposure to Skybox output (can premultiply with the above field on the CPU)
  • Fix all the now broken looking examples (maybe change default point and spotlight intensities?)
  • Docs
  • Migration guide

@fintelia
Copy link
Contributor

I'd suggest against changing the default intensities for point/spot lights. An 800 lumen (i.e., 60 W incandescent brightness) light is more likely to be what a user wants compared to a 3.6 million lumen one. It means that, say, a flashlight will be completely overpowered by the light of the sun, but that's true in the real world too!

Rather, the main suggested mitigation should be to specify exposure settings corresponding to an indoor or night scene. Then making sure the scale of the scene is accurate in meters. And only once those are both calibrated should you start increasing the number/brightness of lights.

@superdump superdump marked this pull request as ready for review September 1, 2023 12:38
@superdump
Copy link
Contributor Author

I can’t approve my own PR. But, as it is my PR, it needs @robtfm ’s approval and we can merge it, or two community reviews.

@JMS55
Copy link
Contributor

JMS55 commented Sep 3, 2023

We still need to fix all the examples...

Changelog/migration guide also needs to mention the other newly added stuff like PhysicalCameraParameters and skybox/envmaplight brightness controls.

@GitGhillie
Copy link
Contributor

Just tried this PR with a scene exported from Blender (Eevee) containing one point light:
image
It's a bit dark but tons better than before. Do we want to adjust the default exposure to roughly match that of Blender (Eevee)?

Also, feel free to let me know if I can help with fixing the examples. In that case please let me know if 'calibrating the examples' means actual calibration or just try to match it by eye :)

@JMS55
Copy link
Contributor

JMS55 commented Dec 15, 2023

@GitGhillie Fixing the examples would be a huge help :). It's really the main blocker of this PR. @fintelia suggested what we should do here: #8407 (comment). We should set an exposure that makes sense for the scene based on real-world values, and then tweak the light values to make sense. Also yeah, reevaluating the default exposure/light intensities probably makes sense.

This PR itself is out of date, but I've just rebased it. Please use this branch as a base for your changes: https://github.com/JMS55/bevy/tree/exposure_jms

@fintelia
Copy link
Contributor

fintelia commented Dec 16, 2023

I did some investigation of Blender's handling of light intensity and I'm somewhat skeptical that their brightness levels are even meaningful...

Apparently, Blender uses a hard-coded conversion factor of 683 lumens/watt when exporting gltf (applied here). While the Blender documentation provides a table of common wattage values with conversion ratio varying from 227 lumens/watt to 556 lumens/watt.

The term for this conversion factor seems to be luminous efficacy. And from what I can find online, the 683 lumens/watt number seems to be a theoretical upper bound and values as low as 15 lumens/watt apply to some light sources.

Weirdly, I wasn't able to find any reference to how Blender determines how much of a light source's power is visible light. That feels like a rather relevant factor given that the whole reason there isn't only one luminous efficacy value is because different kinds of light have differently shaped spectrums

@GitGhillie
Copy link
Contributor

GitGhillie commented Dec 16, 2023

If I change the default exposure from EV100_OVERCAST: f32 = 12.0 to EV100_INDOOR: f32 = 7.0 everything does start to line up roughly with what I see in Blender. If their brightness levels aren't meaningful to reason about I think it's a good enough approximation.

Having a default exposure of 7 instead of 12 also avoids having intensity/illuminance values in the 10000s in the examples.

Here are some comparisons with an exposure of 7, with the Blender exposure and other settings at the default:
Point light 10W (Blender default).
Screenshot 2023-12-16 115422

Directional light 1W/m^2 (also Blender default. I assume they use 1W/m^2 as a default because the default exposure is for indoor scenes, and 1000W/m^2 (strength of the sun according to the Blender docs) would blow out the scene).
Screenshot 2023-12-16 115700

@JMS55 thank you! If people agree with changing the default exposure I can start adjusting the examples

@superdump
Copy link
Contributor Author

@GitGhillie sounds fine to me.

@GitGhillie
Copy link
Contributor

GitGhillie commented Dec 27, 2023

Ran into an issue while adjusting the transmission example:
When I increase the brightness of the candle to compensate for the changes in this PR the transmission effect seems to kind of disappear.
image
Same thing happens if instead I increase the exposure in the already present ColorGrading component.

If I leave everything as it was and adjust the scene using the new ExposureSettings it works fine. Is this the expected behavior?

Working on this branch btw: JMS55#16

@GitGhillie
Copy link
Contributor

Also wondering what the difference is between the ColorGrading exposure and the exposure in ExposureSettings. Aside from being able to set ISO, aperture and f-stop independently.

@coreh
Copy link
Contributor

coreh commented Dec 28, 2023

I have a hunch it might be the heuristic to prevent firefly artifacts when refracting very bright pixel values, it had some arbitrary constant values that might need tweaking.

We might even need to make it a function of the exposure value to make it look right across a wide range of exposures. I'll give this a look later today and try to figure it out.

Edit: It wasn't that, what was actually happening was that we were using an already exposure adjusted image (the background) as the input to another exposure adjustment.

@JMS55
Copy link
Contributor

JMS55 commented Dec 28, 2023

After talking to @DGriffin91 on discord, it sounds like we should keep ColorGrading exposure. We might also want to change it to take the new ExposureSettings.

@fintelia
Copy link
Contributor

The exposure from ColorGrading is an exposure value offset that is meant to be subtracted from the camera's exposure, so I don't think re-using Exposure settings makes sense for it

@coreh
Copy link
Contributor

coreh commented Dec 29, 2023

Fixed the underexposed transmission on GitGhillie#1

(It's now three staggered PRs from this one 😆 🚂🚋🚋🚋)

@GitGhillie
Copy link
Contributor

Alright folks, all examples adjusted (I count 59 examples with lights). It's not perfect but maybe a good time to get some feedback. Here are some notes:

  1. Most examples are going to be slightly brighter/darker. I did this in favor of round numbers and sometimes a better look (imo).
  2. There are some big numbers in there. I know fintelia suggested adjusting the scale of the scene but in most cases it felt like adjusting the scale or exposure would add lines of code that would distract from the example.
  3. I think I would also prefer big numbers for lights vs smaller than 0 numbers for scale.
  4. Nonetheless, the lights/exposure seem to be kind of on the dark side still? Thus requiring intensity values up to a million sometimes. I don't really have a good reference for this. Maybe it's just the consequence of a more physically based unit.
  5. I tried adjusting the scale of the spatial audio example but it seemed to lessen the spatial effect so for now I just increased the brightness.
  6. Maybe it's good to think some more about adjusting the default intensity of the point light. Currently a lot of LOC are added because the default is not sufficient.
  7. The atmospheric_fog example is broken, and I don't see a quick setting that can fix it.
    atmospheric_fog

Anyways, this exercise has taught me a lot about Bevy. Don't be afraid to give feedback that would mean I have to go through everything again, I can probably do it a lot quicker the second time around :)

@coreh
Copy link
Contributor

coreh commented Jan 5, 2024

The atmospheric_fog example is broken, and I don't see a quick setting that can fix it.

On it!

@coreh
Copy link
Contributor

coreh commented Jan 6, 2024

Fixed the atmospheric fog example: GitGhillie#2

@alice-i-cecile
Copy link
Member

Adopted! 🎉 Please review #11347

@alice-i-cecile
Copy link
Member

Merging the adopted PR now!

github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2024
Rebased and finished version of
#8407. Huge thanks to @GitGhillie
for adjusting all the examples, and the many other people who helped
write this PR (@superdump , @coreh , among others) :)

Fixes #8369

---

## Changelog
- Added a `brightness` control to `Skybox`.
- Added an `intensity` control to `EnvironmentMapLight`.
- Added `ExposureSettings` and `PhysicalCameraParameters` for
controlling exposure of 3D cameras.
- Removed the baked-in `DirectionalLight` exposure Bevy previously
hardcoded internally.

## Migration Guide
- If using a `Skybox` or `EnvironmentMapLight`, use the new `brightness`
and `intensity` controls to adjust their strength.
- All 3D scene will now have different apparent brightnesses due to Bevy
implementing proper exposure controls. You will have to adjust the
intensity of your lights and/or your camera exposure via the new
`ExposureSettings` component to compensate.

---------

Co-authored-by: Robert Swain <robert.swain@gmail.com>
Co-authored-by: GitGhillie <jillisnoordhoek@gmail.com>
Co-authored-by: Marco Buono <thecoreh@gmail.com>
Co-authored-by: vero <email@atlasdostal.com>
Co-authored-by: atlas dostal <rodol@rivalrebels.com>
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-Feature A new feature, making something new possible 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.

Directional lights are 4800x dimmer than point lights and spot lights
6 participants