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

Honor mesh-light interaction mediation for “low” render layers #15042

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

coreh
Copy link
Contributor

@coreh coreh commented Sep 4, 2024

Based on this Discord conversation.

Objective

Allow mesh-light interactions to be mediated by (a subset of) render layers, with minimal changes to existing shaders and light/mesh extraction systems.

This addresses the limitation originally described on #10742:

potential issue: when mesh/light layers are smaller than the view layers weird results can occur. e.g:
camera = layers 1+2
light = layers 1
mesh = layers 2

the mesh does not cast shadows wrt the light as (1 & 2) == 0.
the light affects the view as (1+2 & 1) != 0.
the view renders the mesh as (1+2 & 2) != 0.

so the mesh is rendered and lit, but does not cast a shadow.

Solution

  • “Low” render layers (from 0 to RenderLayers::MESH_LIGHT_INTERACTION_MAX) are treated specially, in that they now mediate mesh-light interaction in addition to mesh-camera and light-camera interaction
  • "High" render layers (from RenderLayers::MESH_LIGHT_INTERACTION_MAX + 1 onwards) retain the current behavior of mediating only light-camera and mesh-camera interactions.
  • A bits_compact_lossy() method is introduced to lossily pack the unlimited render layers into a single u16
    • Layers 0–14 are sent verbatim, as a the lower 15 bits of the u16
    • The presence of any layers higher than 14 is signalled by the highest bit of the u16
image
  • On the shader side, we reuse the higher half of the light flags, as well as one of the pad values in the mesh uniform, so that no changes to existing sizes take place
  • Set mesh/light layer is verified by bitwise and operations

This is a compromise solution that:

  • Preserves the ability to have unlimited layers, without regressing with current capabilities (especially for configurable, multi-viewport editor scenarios)
  • Allows us to have mesh-light interactions mediated by (at least some) render layers
  • Doesn't make the shaders overly complex, or take up additional space in the uniforms
  • Produces more intuitive/useful behavior for hand crafted render layer scenarios (see my comment below)

Testing

  • Added a unit test to enforce RenderLayers::MESH_LIGHT_INTERACTION_MAX stays in sync with the return type of bits_compact_lossy()
  • Added a render_layers example so that we can visually validate the effect. (See video below)
  • Verified that the first_person_view_model example is unaffected by this change

Showcase

before.mov
after.mov

Migration Guide

  • RenderLayers from 0 to RenderLayers::MESH_LIGHT_INTERACTION_MAX now also mediate mesh-light interactions. For many common scenarios, including first person view models, this should not produce any changes in existing behavior. If your scene is affected, and you'd like to retain the old behavior of all lights affecting all meshes, regardless of layers, you can add a layer value of RenderLayers::MESH_LIGHT_INTERACTION_MAX + 1 or higher to all affected lights/meshes.

@coreh
Copy link
Contributor Author

coreh commented Sep 4, 2024

Here's an example from my game, where I'm using manually configured render layers to prevent light leaks from indoor to outdoor environments, even for lights that cast no shadows:

Before

image

After

image

Diagram

image

@coreh coreh changed the title Honor mesh-light interactions for “low” render layers Honor mesh-light interaction mediation for “low” render layers Sep 4, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 4, 2024
@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 Sep 4, 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.

Good quality code, a nice example, and clearly explains the limitations in the docs. Just what I was looking for.

Copy link
Contributor

github-actions bot commented Sep 4, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Thanks for the absolutely fantastic pull request description. As mentioned before, the solution is a bit messy (or special-case-y, rather) but the actual implementation looks great. @alice-i-cecile I don't think this is actually controversial, but I'd like to get SME sign off as Mesh uniform space is at a premium, so there could be opportunity cost.

@@ -20,6 +20,36 @@ pub type Layer = usize;
/// An entity with this component without any layers is invisible.
///
/// Entities without this component belong to layer `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 feel like this might almost be too much documentation relative to the feature but the quality here is really good, if anything we should just expand the docs just above this in another PR so it doesn't feel unbalanced.

@@ -770,7 +797,9 @@ pub fn prepare_lights(
color: Vec4::from_slice(&light.color.to_f32_array()) * light.illuminance,
// direction is negated to be ready for N.L
dir_to_light: light.transform.back().into(),
flags: flags.bits(),
flags: flags.bits()
| (light.render_layers.bits_compact_lossy() as u32)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the code is pretty self evidence but it might be nice to leave a breadcrumb here for future readers (e.g., something like "these aren't flags, but are using the extra space").

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Needs-SME Decision or review from an SME is required and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 5, 2024
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-Bug An unexpected or incorrect behavior M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-SME Decision or review from an SME is required X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants