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

Directional lights are 4800x dimmer than point lights and spot lights #8369

Closed
fintelia opened this issue Apr 13, 2023 · 9 comments · Fixed by #11347
Closed

Directional lights are 4800x dimmer than point lights and spot lights #8369

fintelia opened this issue Apr 13, 2023 · 9 comments · Fixed by #11347
Labels
A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior
Milestone

Comments

@fintelia
Copy link
Contributor

fintelia commented Apr 13, 2023

Bevy version

2ec38d1 (but also happens on 0.10.x and probably others).

What you did

Delete the environment map lighting from the PBR example, then compare point light and directional light strengths.

The point light is sqrt(50^2 + 50^2 + 50^2) ≈ 86 meters from the origin and has a strength of 600K lumens. Which means 600,000 / (4 * PI * 86^2) ≈ 6.5 lux reaching the origin.

Point light

image

Same strength directional light

commands.spawn(DirectionalLightBundle {
    transform: Transform::from_xyz(50.0, 50.0, 50.0).looking_at(Vec3::ZERO, Vec3::Y),
    directional_light: DirectionalLight {
        illuminance: 6.5,
        ..default()
    },
    ..default()
});

image

4800x scaled directional light

commands.spawn(DirectionalLightBundle {
    transform: Transform::from_xyz(50.0, 50.0, 50.0).looking_at(Vec3::ZERO, Vec3::Y),
    directional_light: DirectionalLight {
        illuminance: 6.5 * 4800.0,
        ..default()
    },
    ..default()
});

image

Additional information

The bug is caused by this code which dims the strengths of directional lights by 4800x, but leaves point lights and spotlights unchanged:

// convert from illuminance (lux) to candelas
//
// exposure is hard coded at the moment but should be replaced
// by values coming from the camera
// see: https://google.github.io/filament/Filament.html#imagingpipeline/physicallybasedcamera/exposuresettings
const APERTURE: f32 = 4.0;
const SHUTTER_SPEED: f32 = 1.0 / 250.0;
const SENSITIVITY: f32 = 100.0;
let ev100 = f32::log2(APERTURE * APERTURE / SHUTTER_SPEED) - f32::log2(SENSITIVITY / 100.0);
let exposure = 1.0 / (f32::powf(2.0, ev100) * 1.2);
let intensity = light.illuminance * exposure;

@fintelia fintelia added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Apr 13, 2023
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Triage This issue needs to be labelled labels Apr 13, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 13, 2023
@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label Apr 13, 2023
@alice-i-cecile
Copy link
Member

This will be a silly migration. Better to fix it up sooner than later.

@benny-n
Copy link

benny-n commented Apr 13, 2023

Hi, I'd like to take this one, if possible. I think I have a rough idea of what to change to solve it.
Let me know if there's anything else I should know before getting started.

@CptPotato
Copy link
Contributor

CptPotato commented Apr 14, 2023

I think this is because of a fundamental difference between point/spot lights (which have a spatial position and a 1/d² falloff) and directional lights that are infinitely far away and have no distance attenuation.

With spot and point lights you can assign them a value like 1500 lumens that corresponds to the amount of light emitted - which does not work for directional lights. They're using illuminance (lux), which refers to the amount of light received instead. So you're essentially using completely different units for these types of lights.

If I remember correctly the PBR implementations somewhat follows the Filament docs. There is some more information on light units there.

That being said, maybe one of the people who worked on the PBR implementation can chime in and confirm that the current behavior is correct.

@fintelia
Copy link
Contributor Author

fintelia commented Apr 14, 2023

This is not caused by the inverse square falloff. In my post, I use the amount of light emitted by the point light together with the distance to calculate the amount of light received at the origin. Then I construct a directional light that produces the same illuminance.

Rather, this is caused by not following the Filament docs. Specifically, section 5.2.6 says that you have to pre-expose both directional and point/spot lights but Bevy is only pre-exposing directional lights while forgetting to do the same for point/spot lights.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Apr 15, 2023
@superdump
Copy link
Contributor

It seems like we could implement a quick fix that uses hard-coded exposure settings, but also that using per-camera defined exposure settings should be pretty straightforward to implement anyway?

@fintelia
Copy link
Contributor Author

There's currently Camera3dBundle::color_grading::exposure, but that's an exposure value offset which means that it is intended to be subtracted from the calculated automatic exposure. The main problem with using it as a fixed exposure setting directly is that it is negated (if you want a scene ev=15, then you'd set exposure=-15)

@superdump
Copy link
Contributor

I made a draft PR that I think does what is suggested, but it makes everything look very dark. There's a lot of information in the filament docs and I need to read it more carefully. I noticed that only part of all the maths is being used and I didn't have time today to understand it. If you see errors, please point them out and I'll fix them ASAP.

@benny-n
Copy link

benny-n commented Apr 16, 2023

Umm I guess I should close #8378 in favor of #8407 ?

@alice-i-cecile
Copy link
Member

Agreed, I think that's the right way forward. Thanks for looking into this!

@JMS55 JMS55 modified the milestones: 0.11, 0.12 Jun 11, 2023
@JMS55 JMS55 modified the milestones: 0.12, 0.13 Oct 24, 2023
github-merge-queue bot pushed a commit that referenced this issue 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 <[email protected]>
Co-authored-by: GitGhillie <[email protected]>
Co-authored-by: Marco Buono <[email protected]>
Co-authored-by: vero <[email protected]>
Co-authored-by: atlas dostal <[email protected]>
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior
Projects
None yet
6 participants