[Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) #6384

coreh · 2022-10-27T05:40:46Z

Objective

This PR fixes #5789, by enabling movable (and scalable) directional light shadow volumes.

Solution

This PR changes ExtractedDirectionalLight to hold a copy of the DirectionalLight entity's GlobalTransform, instead of just a direction vector. This allows the shadow map volume (as defined by the light's shadow_projection field) to be transformed honoring translation and scale transforms, and not just rotation.

It also augments the texel size calculation (used to determine the shadow_normal_bias) so that it now takes into account the upper bound of the x/y/z scale of the GlobalTransform.

This change makes the directional light extraction code more consistent with point and spot lights (that already use transform), and allows easily moving and scaling the shadow volume along with a player entity based on camera distance/angle, immediately enabling more real world use cases until we have a more sophisticated adaptive implementation, such as the one described in #3629.

Note: While it was previously possible to update the projection achieving a similar effect, depending on the light direction and distance to the origin, the fact that the shadow map camera was always positioned at the origin with a hardcoded Vec3::Y up value meant you would get sub-optimal or inconsistent/incorrect results.

Changelog

Changed

DirectionalLight shadow volumes now honor translation and scale transforms

Migration Guide

If your directional lights were positioned at the origin and not scaled (the default, most common scenario) no changes are needed on your part; it just works as before;
If you previously had a system for dynamically updating directional light shadow projections, you might now be able to simplify your code by updating the directional light entity's transform instead;
In the unlikely scenario that a scene with directional lights that previously rendered shadows correctly has missing shadows, make sure your directional lights are positioned at (0, 0, 0) and are not scaled to a size that's too large or too small.

… and scaling shadow volume

…ting texel size

HackerFoo · 2022-10-27T10:32:59Z

crates/bevy_pbr/src/render/light.rs

@@ -560,15 +560,15 @@ pub fn extract_lights(
                directional_light.shadow_projection.top
                    - directional_light.shadow_projection.bottom,
            );
-        let directional_light_texel_size =
-            largest_dimension / directional_light_shadow_map.size as f32;
+        let directional_light_texel_size = transform.radius_vec3a(Vec3A::ONE) * largest_dimension


Shouldn't the largest dimension be calculated after the transform?

let directional_light_texel_size = transform.radius_vec3a(Vec3A::new( directional_light.shadow_projection.right - directional_light.shadow_projection.left, directional_light.shadow_projection.top - directional_light.shadow_projection.bottom, 0. )).abs().max_element() / directional_light_shadow_map.size as f32;

Nice catch. Updated to use this approach, should give more accurate results.

rparrett · 2022-10-27T14:37:44Z

I can't review the math, but I've been testing with a modified lighting.rs that moves the whole scene around and this seems to work.

It would be great if these new behaviors were documented on DirectionalLight.

I do agree that even if this isn't the ultimate solution, it would be a huge win over the status quo.

rparrett · 2022-10-27T14:43:00Z

Pinging @ramirezmike

…te results

coreh · 2022-10-28T03:16:27Z

It would be great if these new behaviors were documented on DirectionalLight.

Added a section on directional light shadows, documenting how the volume (and shadow maps) work.

rparrett

I have been testing in this little "directional light sandbox" and this seems to be working well.

An interesting note is that prior to this PR, you can modify the translation of a DirectionalLight, and even the lighting example uses a directional light in a non-zero position. But it's not clear at all what the behavior is supposed to be. When you move the light, the shadow projection is affected, but not in any way that would seem to make sense. There may be a bug there. If so, this seems to fix it.

I'm really happy about the new docs as well.

ramirezmike · 2022-11-01T03:34:23Z

I checked out this PR locally and updated one of my games to use it and it works like a charm. My game has a system to update the camera's transform based on the player's transform and I just added DirectionalLight to that query filter and now the shadows work regardless of how far I'm from the origin.

I also can't comment too much on the math involved but the parts I grasp make sense and the docs are great 👍

I'll continue testing this out but pretty happy with it so far.

rparrett · 2022-11-01T14:22:22Z

I tested a bit more in the playground above with modifying the shadow projection directly and it seems like the behavior is the same before/after this PR. (when the scale is 1.)

alice-i-cecile

Very nice new docs, fixes an important limitation, reviewers seem to have thoroughly tested this. Added to the milestone.

crates/bevy_pbr/src/light.rs

crates/bevy_pbr/src/render/light.rs

Co-authored-by: Robert Swain <robert.swain@gmail.com>

rparrett · 2022-11-03T02:41:16Z

@superdump Do we need to adjust DEFAULT_SHADOW_NORMAL_BIAS now? Playing in the shadow_biases example, it seems like an equivalent value to the current default would now be ~0.9

cart

Code looks good, but I'm conflicted on this one. This is a useful intermediate step and solves a real problem, but it is also training people to think about DirectionalLights in a non-standard way (and write code that operates under that assumption). Everywhere else in the ecosystem (ex: Godot, Blender, etc), DirectionalLight position has no bearing on how light / shadows work. I would have preferred a solution that maintains that mental model, but I also understand that the approach in this PR is way more straightforward to implement.

That being said, theres enough positive sentiment here to get me on board. This just increases the pressure to get shadow cascades / view frustum fitting working, which are sorely needed already!

superdump · 2022-11-03T09:14:21Z

@cart - the approach that I implemented a while back was to create an oriented bounding box around the view frustum of the camera and adjust the light's shadow projection based on that. I abandoned it because the shadow map quality quickly degrades with the size of the frustum.

coreh · 2022-11-04T02:12:12Z

Merged and fixed the conflicts. The system now seems to split ShadowView and GpuDirectionalLight extraction in two separate loops, I believe from #6066. That actually paired nicely with this, the code is overall a bit cleaner.

I tested it and things seem to be working properly with multiple directional lights.

This is a useful intermediate step and solves a real problem, but it is also training people to think about DirectionalLights in a non-standard way (and write code that operates under that assumption).

😕 Yeah. Please let me know if you'd like me to add a note to the docs explicitly stating that things might change in the future.

Do we need to adjust DEFAULT_SHADOW_NORMAL_BIAS now? Playing in the shadow_biases example, it seems like an equivalent value to the current default would now be ~0.9

Just tried out the example, is it really supposed to look like this?

Tried it out in main and it looks the same, so it probably isn't a result of this PR:

Or is that what you mean? That we need to tweak the values somehow so that it doesn't look like this?

rparrett · 2022-11-04T02:19:43Z

As far as I know, that example was used to determine the current values for the default shadow bias constants.

I believe the idea is that you use the keys to adjust the biases individually until the shadows don't look bad and then use that value for the default.

But that's what I was seeking clarity from superdump about.

superdump · 2022-11-04T03:26:17Z

@superdump Do we need to adjust DEFAULT_SHADOW_NORMAL_BIAS now? Playing in the shadow_biases example, it seems like an equivalent value to the current default would now be ~0.9

Why does the normal bias need changing? What changed to need to change it? 🙂 I need to look more closely...

rparrett · 2022-11-04T03:28:04Z

Why does the normal bias need changing? What changed to need to change it? 🙂 I need to look more closely...

The sqrt business, I think.

superdump · 2022-11-04T03:28:11Z

@coreh yeah the shadow biases example is meant to start off looking bad with lots of shadow artifacts so you know what they look like and can learn how the biases impact them.

superdump · 2022-11-04T03:35:08Z

Why does the normal bias need changing? What changed to need to change it? 🙂 I need to look more closely...

The sqrt business, I think.

We changed from sqrt(2) * max(x, y) to sqrt(x^2 + y^2) practically I think. The sqrt(2) came from assuming square fragment aspect ratio so e.g. x == y and then sqrt(x^2 + x^2) == sqrt(2 * x^2) == sqrt(2) * x == sqrt(2) * max(x, x). I feel like that shouldn't have had a significant impact in default cases. Perhaps it or something else I missed does though.

rparrett · 2022-11-04T15:26:04Z

the shadow projection in shadow_biases is very not square.

rparrett · 2022-11-04T15:45:52Z

The current 0.6 default still seems like a good value in the lighting example. Is it expected for users to need to tune these biases when the projection shape changes?

I just noticed the different behavior in shadow_biases where it seemed likely the current defaults were determined (because pre-this-PR the current defaults perfectly eliminate artifacts there) and thought they might need to be updated.

Really don't want to derail this if it's very unlikely to affect normal users though.

cart · 2022-11-04T20:12:04Z

I'm down to merge this. Changing shadow bias behavior (for a given value) should be avoided unless absolutely necessary as artists will carefully tune the value to their specific scenes. It seems like doing it here was necessary though. And changing the default value is less destructive than changing the algorithm, so I'm happy to change that later if we later find a better value.

cart · 2022-11-04T20:12:09Z

bors r+

… shadow map volume (not just direction) (#6384) # Objective This PR fixes #5789, by enabling movable (and scalable) directional light shadow volumes. ## Solution This PR changes `ExtractedDirectionalLight` to hold a copy of the `DirectionalLight` entity's `GlobalTransform`, instead of just a `direction` vector. This allows the shadow map volume (as defined by the light's `shadow_projection` field) to be transformed honoring translation _and_ scale transforms, and not just rotation. It also augments the texel size calculation (used to determine the `shadow_normal_bias`) so that it now takes into account the upper bound of the x/y/z scale of the `GlobalTransform`. This change makes the directional light extraction code more consistent with point and spot lights (that already use `transform`), and allows easily moving and scaling the shadow volume along with a player entity based on camera distance/angle, immediately enabling more real world use cases until we have a more sophisticated adaptive implementation, such as the one described in #3629. **Note:** While it was previously possible to update the projection achieving a similar effect, depending on the light direction and distance to the origin, the fact that the shadow map camera was always positioned at the origin with a hardcoded `Vec3::Y` up value meant you would get sub-optimal or inconsistent/incorrect results. --- ## Changelog ### Changed - `DirectionalLight` shadow volumes now honor translation and scale transforms ## Migration Guide - If your directional lights were positioned at the origin and not scaled (the default, most common scenario) no changes are needed on your part; it just works as before; - If you previously had a system for dynamically updating directional light shadow projections, you might now be able to simplify your code by updating the directional light entity's transform instead; - In the unlikely scenario that a scene with directional lights that previously rendered shadows correctly has missing shadows, make sure your directional lights are positioned at (0, 0, 0) and are not scaled to a size that's too large or too small.

bors · 2022-11-04T20:41:58Z

Pull request successfully merged into main.

Build succeeded:

… shadow map volume (not just direction) (bevyengine#6384) # Objective This PR fixes bevyengine#5789, by enabling movable (and scalable) directional light shadow volumes. ## Solution This PR changes `ExtractedDirectionalLight` to hold a copy of the `DirectionalLight` entity's `GlobalTransform`, instead of just a `direction` vector. This allows the shadow map volume (as defined by the light's `shadow_projection` field) to be transformed honoring translation _and_ scale transforms, and not just rotation. It also augments the texel size calculation (used to determine the `shadow_normal_bias`) so that it now takes into account the upper bound of the x/y/z scale of the `GlobalTransform`. This change makes the directional light extraction code more consistent with point and spot lights (that already use `transform`), and allows easily moving and scaling the shadow volume along with a player entity based on camera distance/angle, immediately enabling more real world use cases until we have a more sophisticated adaptive implementation, such as the one described in bevyengine#3629. **Note:** While it was previously possible to update the projection achieving a similar effect, depending on the light direction and distance to the origin, the fact that the shadow map camera was always positioned at the origin with a hardcoded `Vec3::Y` up value meant you would get sub-optimal or inconsistent/incorrect results. --- ## Changelog ### Changed - `DirectionalLight` shadow volumes now honor translation and scale transforms ## Migration Guide - If your directional lights were positioned at the origin and not scaled (the default, most common scenario) no changes are needed on your part; it just works as before; - If you previously had a system for dynamically updating directional light shadow projections, you might now be able to simplify your code by updating the directional light entity's transform instead; - In the unlikely scenario that a scene with directional lights that previously rendered shadows correctly has missing shadows, make sure your directional lights are positioned at (0, 0, 0) and are not scaled to a size that's too large or too small.

coreh added 2 commits October 27, 2022 01:56

Use GlobalTransform in ExtractedDirectionalLight to allow translating…

558c667

… and scaling shadow volume

Correctly account for affine transform scale upper bound when calcula…

339546d

…ting texel size

HackerFoo reviewed Oct 27, 2022

View reviewed changes

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Oct 27, 2022

alice-i-cecile requested a review from superdump October 27, 2022 12:45

coreh added 2 commits October 27, 2022 22:35

Consolidate largest dimension and radius calculations for more accura…

455c2b9

…te results

Add documentation on DirectionalLight shadows

54a76fb

coreh added 2 commits October 28, 2022 00:28

Fix example code that's failing on CI

8b349e5

Fix typo

5fffc35

rparrett approved these changes Oct 28, 2022

View reviewed changes

mockersf approved these changes Nov 1, 2022

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 2, 2022

alice-i-cecile approved these changes Nov 2, 2022

View reviewed changes

alice-i-cecile added this to the Bevy 0.9 milestone Nov 2, 2022

superdump requested changes Nov 2, 2022

View reviewed changes

coreh and others added 2 commits November 2, 2022 23:15

Tweak documentation comments as per code review suggestions

5e56855

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Remove redundant SQRT_2 multiplication

40661ce

cart approved these changes Nov 3, 2022

View reviewed changes

coreh added 2 commits November 3, 2022 22:34

Merge branch 'main' into directional-light-transform

ff8e3fe

Fix code to work with changes to enable multiple directional lights

d404d7f

bors bot changed the title ~~Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction)~~ [Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) Nov 4, 2022

bors bot closed this Nov 4, 2022

mockersf mentioned this pull request Nov 10, 2022

amd gpu make shadows are misplaced on a very large plane #6542

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) #6384

[Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) #6384

coreh commented Oct 27, 2022 •

edited

Loading

HackerFoo Oct 27, 2022

coreh Oct 28, 2022

rparrett commented Oct 27, 2022

rparrett commented Oct 27, 2022

coreh commented Oct 28, 2022

rparrett left a comment •

edited

Loading

ramirezmike commented Nov 1, 2022

rparrett commented Nov 1, 2022

alice-i-cecile left a comment

rparrett commented Nov 3, 2022

cart left a comment

superdump commented Nov 3, 2022

coreh commented Nov 4, 2022

rparrett commented Nov 4, 2022 •

edited

Loading

superdump commented Nov 4, 2022

rparrett commented Nov 4, 2022

superdump commented Nov 4, 2022

superdump commented Nov 4, 2022

rparrett commented Nov 4, 2022

rparrett commented Nov 4, 2022

cart commented Nov 4, 2022

cart commented Nov 4, 2022

bors bot commented Nov 4, 2022

[Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) #6384

[Merged by Bors] - Take DirectionalLight's GlobalTransform into account when calculating shadow map volume (not just direction) #6384

Conversation

coreh commented Oct 27, 2022 • edited Loading

Objective

Solution

Changelog

Changed

Migration Guide

HackerFoo Oct 27, 2022

Choose a reason for hiding this comment

coreh Oct 28, 2022

Choose a reason for hiding this comment

rparrett commented Oct 27, 2022

rparrett commented Oct 27, 2022

coreh commented Oct 28, 2022

rparrett left a comment • edited Loading

Choose a reason for hiding this comment

ramirezmike commented Nov 1, 2022

rparrett commented Nov 1, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

rparrett commented Nov 3, 2022

cart left a comment

Choose a reason for hiding this comment

superdump commented Nov 3, 2022

coreh commented Nov 4, 2022

rparrett commented Nov 4, 2022 • edited Loading

superdump commented Nov 4, 2022

rparrett commented Nov 4, 2022

superdump commented Nov 4, 2022

superdump commented Nov 4, 2022

rparrett commented Nov 4, 2022

rparrett commented Nov 4, 2022

cart commented Nov 4, 2022

cart commented Nov 4, 2022

bors bot commented Nov 4, 2022

coreh commented Oct 27, 2022 •

edited

Loading

rparrett left a comment •

edited

Loading

rparrett commented Nov 4, 2022 •

edited

Loading