Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] #12502

UkoeHB · 2024-03-15T22:51:35Z

Objective

Address various ambiguities and issues with the separation between RenderLayers and TargetCamera.
Closes Unify RenderLayers and TargetCamera and provide a single way to configure which camera entities are rendered to #12468.

Solution

See changelog. This is Part 1 of a 2-part rework. Part 2 will update UI to remove TargetCamera and use affiliated cameras instead.

Part 2 planned changelog:

Remove TargetCamera.
Use PropagateRenderGroups::Camera by default on UI cameras. This will set the camera affiliation in InheritedRenderGroups equal to the UI cameras (unless an entity has a RenderGroups component with a manually-specified camera affiliation, which will take precedence).

Part 2 planned migration:

Replace TargetCamera components that you added manually to cameras with PropagateRenderGroups::Camera components. If those cameras need to see all 'default render layer' entities, then also add a CameraView::default() component.

Changelog

Introduced a unified RenderGroups component that merges the functionality of RenderLayers and TargetCamera.
- Includes RenderLayers (reworked) and an optional 'affiliated camera'.
Reworked RenderLayers to store a growable bitmask, instead of being fixed to 32 bits. It can now store up to 64 render layers on the stack (including the default render layer), and then will allocate for more bits.
Replaced Layer with RenderLayer as a user-facing abstraction for interacting with RenderLayers. A RenderLayer is a bit index, equivalent to the existing Layer type.
- Added DEFAULT_RENDER_LAYER = RenderLayer(0). By default all entities and cameras without RenderGroups or CameraView components will be in the default layer.
Added CameraView as a camera-specific component for controlling which RenderLayers are visible to the camera. Cameras automatically see any entity with a RenderGroups component that has a camera affiliation equal to the given camera.
Added a PropagateRenderGroups component for controlling how RenderGroups are propagated down hierarchies.
- Added an InheritedRenderGroups component that merges RenderGroups components with propagated RenderGroups. This component is managed automatically in PropagateRenderGroupsSet. It is used internally for rendering, and can be queried by users to e.g. debug visibility issues.
Added PropagateRenderGroupsSet, which runs in the VisibilityPropagate set.

Migration Guide

Replace Layer with RenderLayer.
Replace RenderLayers components with RenderGroups components on non-camera entities.
Replace RenderLayers components with CameraView components on camera entities.

TODOs

Complete integration.
- This PBR shader uses render layers internally (GpuDirectionalLight and ViewUniform and DirectionalLight and View). Now that it is a growable bitset, this doesn't work unless we set an upper cap on bits and pass in an array of ints for the flags.
- prepare_lights in bevy_pbr is cloning ExtractedDirectionalLight, which contains a RenderGroups. This makes it potentially-allocating due to the growable bitset. There is also a clone when extracting from the world (also for ViewUniform).
- Check pixel_grid_snap, render_to_texture examples.
Task list: Add tests, examples and benchmarks for RenderLayers and TargetCamera rework #12588

Follow-up

Part 2: Remove TargetCamera and use affiliated cameras in for UI instead.
Refactor gizmos/GizmoConfig to support displaying different gizmos in different cameras. This may require a deeper rethink of how gizmos are designed, since they are currently special-cased.

viridia · 2024-03-16T00:28:03Z

I want to point out that there's a couple other options here - not that they are necessarily better.

One option is to combine the 'inherited' status as a boolean property within the RenderGroups component, rather than making it a separate marker component. I don't know whether this is better or not, it really depends on how much overhead there is for storing separate marker components.

You could take that further and have the property be an enum:

enum RenderGroupInheritance {
  /// This render group was calculated automatically
  Implicit = 0,
  /// This render group was explicitly set on the entity, but should not cascade to descendants
  Explicit,
  /// This render group has been explicitly set on the entity, and should cascade to descendants
  ExplicitPropagating
}

This means reducing the total number of components involved on each entity. Whether that is an optimization or de-optimization, I don't know.

It also raises the question: if an explicit propagating entity has an explicit non-propagating child, what should the render groups for its grandchild be?

UkoeHB · 2024-03-16T02:44:42Z

Working on the implementation for propagating render groups now. I think I can manage the following rules without needing to change the time complexity of existing code:

Any entity with PropagateRenderGroups will propagate a RenderGroup to its children (derived from the PropagateRenderGroups variant).
If an entity with PropagateRenderGroups is in-tree, it will block up-tree propagation and 'take over'. Entities can only inherit one propagated RenderGroups, unless they have PropagateRenderGroups in which case they can't inherit.

Allowing PropagateRenderGroups to merge together as you traverse the tree would require more time-complex hierarchy traversal due to a 'dependency diamond' issue.

It is also possible, although I think probably better for a follow-up PR, to allow a component RenderGroupsInheritancePolicy with filtering rules:

Auto: Doesn't do anything extra.
Skip: Don't merge inherited groups, but pass them to children.
Block: Don't merge inherited groups, and prevent propagation to children.

EDIT: Given the complexity of the propagation algorithm for this PR, I doubt RenderGroupsInheritancePolicy can be easily integrated.

viridia · 2024-03-17T03:17:38Z

Something that would be helpful, I think, is a high-level description of the propagation algorithm. For example, the stylesheet propagator that I wrote a few months ago could be described by the following summary: "Starting with a query of all UI entities that can be styled, start by identifying the roots and then recursively visit all descendants accessible via the query. Each visited node calculates the composition of the inherited style with the style defined on that node, if any. Mutations to the state of each node are applied differentially to minimize change detection signals."

UkoeHB · 2024-03-17T23:53:40Z

Something that would be helpful, I think, is a high-level description of the propagation algorithm.

I wrote a description and notes at the top of the propagate_render_groups.rs file.

viridia · 2024-03-18T00:52:11Z

Something that would be helpful, I think, is a high-level description of the propagation algorithm.

I wrote a description and notes at the top of the propagate_render_groups.rs file.

That looks really good.

Part of the motivation for wanting this description is to be able to make an argument as to why this approach is better than alternatives (such as an exclusive system with world access, or a giant query with a bunch of optional components which are then accessed via entity id instead of sequential scanning), both of which are obvious patterns for hierarchical traversals.

tychedelia

This looks great! Thanks for tackling this, it's going to be a huge help.

Regarding your open questions:

How to put reasonable limits on RenderLayer?

I think, at best, we should warn. My use case is a little niche because I'm allowing users to spawn cameras at will, but I'd rather have them work around poor performance than be arbitrarily limited. In my case, the allocations from going about 64 layers are unlikely to matter, especially compared to whatever other lurking inefficiencies there are in the renderer with having so many cameras.

This PBR shader uses render layers internally (GpuDirectionalLight and ViewUniform and DirectionalLight and View).

I commented on the issue here #12468 (comment), but I think the preference is to remove RenderLayers from the shader code entirely and compute light visibility on the cpu as property of the view. We could include layers as a dynamic array on the view itself, but iirc there were problems including the layers on the light struct as they're already nested inside another array.

crates/bevy_render/src/view/visibility/render_groups.rs

tychedelia · 2024-03-19T02:17:57Z

crates/bevy_render/src/view/visibility/render_groups.rs

+/// A default `CameraView` will include the [`DEFAULT_RENDER_LAYER`].
+#[derive(Component, Debug, Clone, PartialEq, Reflect)]
+#[reflect(Component, Default, PartialEq)]
+pub struct CameraView {


Nit: I can't decide if "view" is too overloaded here. I totally understand the description of the component as "things I can see", but might get confused in it's relation to something like ExtractedView in render world.

Yeah it's pretty overloaded, but it feels really comfortable for this use-case.

UkoeHB · 2024-03-19T23:27:59Z

crates/bevy_pbr/src/render/light.rs

        for (light_index, &(light_entity, light)) in directional_lights
            .iter()
            .enumerate()
-            .take(directional_shadow_enabled_count)
+            .take(MAX_DIRECTIONAL_LIGHTS)
        {
+            let gpu_light = &mut gpu_lights.directional_lights[light_index];
+
+            // Check if the light intersects with the view.
+            if extracted_render_groups.intersects(&light.render_groups) {
+                gpu_light.skip = 1u32;
+                continue;
+            }
+
+            // Only deal with cascades when shadows are enabled.
+            if (gpu_light.flags & DirectionalLightFlags::SHADOWS_ENABLED.bits()) == 0u32 {
+                continue;
+            }


Changing this may invalidate directional_depth_texture_array_index, someone who understands this needs to check.

UkoeHB · 2024-03-20T00:21:31Z

I commented on the issue here #12468 (comment), but I think the preference is to remove RenderLayers from the shader code entirely and compute light visibility on the cpu as property of the view. We could include layers as a dynamic array on the view itself, but iirc there were problems including the layers on the light struct as they're already nested inside another array.

Ok the check has been moved to the CPU code. I added a skip field to the GpuDirectionalLight struct, which allows the shader to skip directional lights not visible to a particular view. It's kind of a hack, but hard to see a better option that doesn't involve a major refactor of this dense code.

UkoeHB · 2024-03-20T00:36:40Z

I think, at best, we should warn.

Added a warning at 1024 layers, and a panic failsafe if you exceed 1mill, which would be 24MB per RenderLayers struct.

viridia · 2024-03-20T18:10:58Z

@alice-i-cecile @UkoeHB I tried to make an example, but I couldn't get it to work - possibly I am doing something wrong. Here's what I did:

Start with the example update_gltf_scene which spawns two copies of the flight helmet model.
Add RenderGroups::default().add(1).clone() to the first model spawn. (I'm not sure this is the best way to create a RenderGroups with a given layer).
Add RenderGroups::default().add(2).clone() to the second model spawn.
Finally, add RenderGroups::default().add(1).clone() to the camera spawn.

At this point I would have expected the second model not to be visible. However, both models were visible.

UkoeHB · 2024-03-20T18:34:00Z

At this point I would have expected the second model not to be visible. However, both models were visible.

A default RenderGroups starts with the default render layer. Use RenderGroups::from() instead. Also, the camera uses CameraView, not RenderGroups.

viridia · 2024-03-20T18:43:49Z

If I try RenderGroups::from(RenderLayers(2)) I get the following error:

error[E0391]: cycle detected when getting the resolver for lowering
  |
  = note: ...which requires normalizing `bevy_render::view::visibility::render_groups::RenderLayers::iter_layers::{opaque#0}`...
  = note: ...which requires looking up limits...
  = note: ...which requires getting the crate HIR...
  = note: ...which again requires getting the resolver for lowering, completing the cycle
  = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

viridia · 2024-03-20T18:47:31Z

I changed the camera to CameraView, but I still see both models.

UkoeHB · 2024-03-20T18:51:48Z

I changed the camera to CameraView, but I still see both models.

Can you share your code? Maybe in a gist. If there is a bug then I will fix it ASAP.

viridia · 2024-03-20T18:56:49Z

Start with the existing update_gltf_scene example (which is relatively short), and change the setup function to the following:

fn setup(mut commands: Commands, asset_server: Res<AssetServer>) {
    commands.spawn((
        DirectionalLightBundle {
            transform: Transform::from_xyz(4.0, 25.0, 8.0).looking_at(Vec3::ZERO, Vec3::Y),
            directional_light: DirectionalLight {
                shadows_enabled: true,
                ..default()
            },
            ..default()
        },
        RenderGroups::default().add(1).clone(),
    ));
    commands.spawn((
        Camera3dBundle {
            transform: Transform::from_xyz(-0.5, 0.9, 1.5)
                .looking_at(Vec3::new(-0.5, 0.3, 0.0), Vec3::Y),
            ..default()
        },
        EnvironmentMapLight {
            diffuse_map: asset_server.load("environment_maps/pisa_diffuse_rgb9e5_zstd.ktx2"),
            specular_map: asset_server.load("environment_maps/pisa_specular_rgb9e5_zstd.ktx2"),
            intensity: 1500.0,
        },
        CameraView::default().add(1).clone(),
    ));

    // Spawn the scene as a child of this entity at the given transform
    commands.spawn((
        SceneBundle {
            transform: Transform::from_xyz(-1.0, 0.0, 0.0),
            scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
            ..default()
        },
        RenderGroups::default().add(1).clone(),
    ));

    // Spawn a second scene, and add a tag component to be able to target it later
    commands.spawn((
        SceneBundle {
            scene: asset_server.load("models/FlightHelmet/FlightHelmet.gltf#Scene0"),
            ..default()
        },
        MovedScene,
        RenderGroups::default().add(2).clone(),
    ));
}

github-actions · 2024-03-21T18:09:08Z

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.

github-actions · 2024-03-26T01:38:50Z

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

…ts can see a mesh but the camera can't see the lights

…ghts now intersect entities again but warn if they have more than one RenderLayer

UkoeHB · 2024-04-16T02:47:20Z

Changing this to ADOPT ME status since I don't have time or motivation to continue working on it.

tychedelia · 2024-05-10T18:29:05Z

Opened a PR to port removing the layer limit (which seemed less controversial) #13317

@UkoeHB

# Objective Remove the limit of `RenderLayer` by using a growable mask using `SmallVec`. Changes adopted from @UkoeHB's initial PR here #12502 that contained additional changes related to propagating render layers. Changes ## Solution The main thing needed to unblock this is removing `RenderLayers` from our shader code. This primarily affects `DirectionalLight`. We are now computing a `skip` field on the CPU that is then used to skip the light in the shader. ## Testing Checked a variety of examples and did a quick benchmark on `many_cubes`. There were some existing problems identified during the development of the original pr (see: https://discord.com/channels/691052431525675048/1220477928605749340/1221190112939872347). This PR shouldn't change any existing behavior besides removing the layer limit (sans the comment in migration about `all` layers no longer being possible). --- ## Changelog Removed the limit on `RenderLayers` by using a growable bitset that only allocates when layers greater than 64 are used. ## Migration Guide - `RenderLayers::all()` no longer exists. Entities expecting to be visible on all layers, e.g. lights, should compute the active layers that are in use. --------- Co-authored-by: robtfm <[email protected]>

bugsweeper · 2024-05-31T11:00:38Z

@tychedelia doc for RenderLayers still contains mention of TOTAL_LAYERS, which doesn't exist

Sirmadeira · 2024-09-16T21:42:15Z

@alice-i-cecile Sorry for the annoying ping, may I adopt this? I am not sure if this is still ongoing. I literally made my rtts a few x transforms away from my game. It is driving me insane the ugly code

UkoeHB · 2024-09-16T21:47:15Z

@Sirmadeira this PR originally died because one of the rendering SMEs blocked it. You'll want to get consensus/full approval from the rendering guys before putting any work in.

alice-i-cecile · 2024-09-16T21:53:18Z

Yep, I'd like to see this work in some form, but I definitely recommend building consensus first.

Sirmadeira · 2024-09-16T21:53:30Z

@UkoeHB If you dont mind me asking what was the reason for the block?

UkoeHB · 2024-09-16T22:16:10Z

@UkoeHB If you dont mind me asking what was the reason for the block?

Something about it being too complicated or 'we don't really need this'. If you can present a compelling case then it might get enough support from the SMEs.

viridia · 2024-09-16T22:24:40Z

Originally, there were three different users pushing for this, each with their own use case:

@tychedelia Wanted to expand the number of render layers, this part got merged.
@aevyrie Wanted to unify TargetCamera and RenderLayers for their own work (I believe this was related to mod_picking. I don't know how this intersects with the mod_picking upstreaming). This part got dropped.
What I wanted was an option for inheritable render layers - that is, I wanted to be able to load a GLTF model or scene, assign render layers to the root entity of that scene, and have that layer config apply to all of the different entities and meshes in the loaded scene. Currently the way you have to do this is listen for the scene to finish spawning, then traverse the scene hierarchy and add the render layers component to each descendant. This part of the work also got dropped.

viridia mentioned this pull request Mar 16, 2024

Mechanism for inheritable render layers #12461

Open

UkoeHB changed the title ~~Unify RenderLayers and TargetCamera~~ Unify RenderLayers and TargetCamera (Part 1: RenderGroups) Mar 18, 2024

tychedelia reviewed Mar 19, 2024

View reviewed changes

UkoeHB commented Mar 19, 2024

View reviewed changes

UkoeHB force-pushed the vis_rework branch from a35fb63 to 86d13db Compare March 19, 2024 23:54

UkoeHB force-pushed the vis_rework branch from bcbd9f9 to 4d1c094 Compare March 20, 2024 00:41

UkoeHB marked this pull request as ready for review March 20, 2024 02:00

UkoeHB mentioned this pull request Mar 20, 2024

Add tests, examples and benchmarks for RenderLayers and TargetCamera rework #12588

Open

10 tasks

UkoeHB force-pushed the vis_rework branch from efa8ae0 to 31bfea4 Compare March 25, 2024 23:26

UkoeHB added 2 commits April 1, 2024 07:47

add RenderGroups (WIP)

faba920

typo

756191b

UkoeHB added 10 commits April 1, 2024 07:51

add floor to example:

858b88f

fix shadows appearing erroneously for point and spot lights when ligh…

962d722

…ts can see a mesh but the camera can't see the lights

cleanup example

266b048

fmt

62c04aa

add second camera to render groups example

6b4a2d3

simplify: RenderGroups -> RenderLayers, CameraView -> CameraLayer, li…

19bacda

…ghts now intersect entities again but warn if they have more than one RenderLayer

fix examples

48dc2ee

try to fix render_to_texture example

fb9d417

fmt

9c826d8

remove unsafe blocker from bevy_pbr

6f3909c

UkoeHB force-pushed the vis_rework branch from 8cc2f4c to 6f3909c Compare April 1, 2024 13:02

musjj mentioned this pull request Apr 1, 2024

TargetCamera breaks UI mouse interaction #12836

Open

UkoeHB changed the title ~~Unify RenderLayers and TargetCamera (Part 1: RenderGroups)~~ Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] Apr 16, 2024

alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Apr 16, 2024

tychedelia added a commit to tychedelia/bevy that referenced this pull request May 10, 2024

bevyengine#12502 Remove limit on RenderLayers.

7febd5e

tychedelia added a commit to tychedelia/bevy that referenced this pull request May 10, 2024

bevyengine#12502 Remove limit on RenderLayers.

c8c3df4

tychedelia added a commit to tychedelia/bevy that referenced this pull request May 10, 2024

bevyengine#12502 Remove limit on RenderLayers.

e477571

tychedelia added a commit to tychedelia/bevy that referenced this pull request May 10, 2024

bevyengine#12502 Remove limit on RenderLayers.

5e0b61c

tychedelia added a commit to tychedelia/bevy that referenced this pull request May 10, 2024

bevyengine#12502 Remove limit on RenderLayers.

77f544b

tychedelia mentioned this pull request May 10, 2024

#12502 Remove limit on RenderLayers. #13317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] #12502

Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] #12502

UkoeHB commented Mar 15, 2024 •

edited

Loading

viridia commented Mar 16, 2024 •

edited

Loading

UkoeHB commented Mar 16, 2024 •

edited

Loading

viridia commented Mar 17, 2024 •

edited

Loading

UkoeHB commented Mar 17, 2024

viridia commented Mar 18, 2024

tychedelia left a comment

tychedelia Mar 19, 2024

UkoeHB Mar 19, 2024

UkoeHB Mar 19, 2024

UkoeHB commented Mar 20, 2024

UkoeHB commented Mar 20, 2024

viridia commented Mar 20, 2024

UkoeHB commented Mar 20, 2024 •

edited

Loading

viridia commented Mar 20, 2024

viridia commented Mar 20, 2024 •

edited

Loading

UkoeHB commented Mar 20, 2024

viridia commented Mar 20, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 26, 2024

UkoeHB commented Apr 16, 2024

tychedelia commented May 10, 2024

bugsweeper commented May 31, 2024 •

edited

Loading

Sirmadeira commented Sep 16, 2024

UkoeHB commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

Sirmadeira commented Sep 16, 2024

UkoeHB commented Sep 16, 2024

viridia commented Sep 16, 2024

Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] #12502

Are you sure you want to change the base?

Unify RenderLayers and TargetCamera (Part 1: RenderGroups) [ADOPT ME] #12502

Conversation

UkoeHB commented Mar 15, 2024 • edited Loading

Objective

Solution

Changelog

Migration Guide

TODOs

Follow-up

viridia commented Mar 16, 2024 • edited Loading

UkoeHB commented Mar 16, 2024 • edited Loading

viridia commented Mar 17, 2024 • edited Loading

UkoeHB commented Mar 17, 2024

viridia commented Mar 18, 2024

tychedelia left a comment

Choose a reason for hiding this comment

tychedelia Mar 19, 2024

Choose a reason for hiding this comment

UkoeHB Mar 19, 2024

Choose a reason for hiding this comment

UkoeHB Mar 19, 2024

Choose a reason for hiding this comment

UkoeHB commented Mar 20, 2024

UkoeHB commented Mar 20, 2024

viridia commented Mar 20, 2024

UkoeHB commented Mar 20, 2024 • edited Loading

viridia commented Mar 20, 2024

viridia commented Mar 20, 2024 • edited Loading

UkoeHB commented Mar 20, 2024

viridia commented Mar 20, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 26, 2024

UkoeHB commented Apr 16, 2024

tychedelia commented May 10, 2024

bugsweeper commented May 31, 2024 • edited Loading

Sirmadeira commented Sep 16, 2024

UkoeHB commented Sep 16, 2024

alice-i-cecile commented Sep 16, 2024

Sirmadeira commented Sep 16, 2024

UkoeHB commented Sep 16, 2024

viridia commented Sep 16, 2024

UkoeHB commented Mar 15, 2024 •

edited

Loading

viridia commented Mar 16, 2024 •

edited

Loading

UkoeHB commented Mar 16, 2024 •

edited

Loading

viridia commented Mar 17, 2024 •

edited

Loading

UkoeHB commented Mar 20, 2024 •

edited

Loading

viridia commented Mar 20, 2024 •

edited

Loading

bugsweeper commented May 31, 2024 •

edited

Loading