Skip to content

#12502 Remove limit on RenderLayers.#13317

Merged
alice-i-cecile merged 24 commits into
bevyengine:mainfrom
tychedelia:12502-render-layers
May 16, 2024
Merged

#12502 Remove limit on RenderLayers.#13317
alice-i-cecile merged 24 commits into
bevyengine:mainfrom
tychedelia:12502-render-layers

Conversation

@tychedelia

Copy link
Copy Markdown
Member

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.

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels May 10, 2024
@tychedelia

Copy link
Copy Markdown
Member Author

@james7132 I think I know how to use tracy, would you mind pointing me in the direction of what I should benchmark or any documentation for this relative to rendering prs?

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 10, 2024
Comment thread crates/bevy_dev_tools/src/ui_debug_overlay/mod.rs
Comment thread examples/3d/render_to_texture.rs Outdated
Comment thread crates/bevy_pbr/src/light/mod.rs Outdated
Comment thread crates/bevy_pbr/src/render/mesh_view_types.wgsl Outdated
Comment thread crates/bevy_render/src/view/visibility/render_layers.rs Outdated
@tychedelia
tychedelia requested a review from robtfm May 11, 2024 19:51
@robtfm

robtfm commented May 14, 2024

Copy link
Copy Markdown
Contributor

Curious why I didn't hit it on my Mac.

I ran cargo run --release --example many_cubes -- --benchmark

Will check its fixed shortly

@robtfm

robtfm commented May 14, 2024

Copy link
Copy Markdown
Contributor

works for me too now.

@tychedelia

tychedelia commented May 14, 2024

Copy link
Copy Markdown
Member Author

Don't want to claim to be a perf expert, but the numbers seem pretty reasonable to me? I'm sure there are scenarios we haven't covered yet.

If the performance hit is acceptable, I don't think the additional maintence burden is worth it, but I will just say that I was able to implement a feature flag like so that "just works":

#[cfg(feature = "unlimited_render_layers")]
#[path = "unlimited_render_layers.rs"]
mod render_layers;
#[cfg(not(feature = "unlimited_render_layers"))]
mod render_layers;

@alice-i-cecile

alice-i-cecile commented May 14, 2024

Copy link
Copy Markdown
Member

I don't think feature flagging this is worth it. That's a nasty bit of maintenance burden, and IMO the flexibility here is worth the performance cost.

@alice-i-cecile alice-i-cecile added S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 14, 2024
@Brezak

Brezak commented May 16, 2024

Copy link
Copy Markdown
Contributor

I know this is pretty far into development but have we considered a tagged pointer to a slice with it's size stored inline?

The render layer struct would be a single pointer whose address would define what it contains:

  • 0bXX...XX00 A address to 4 byte aligned bitset with it's size stored in the allocation.
  • 0bXX...XX01 A 62/30 bit bitset stored in the address bits.
  • 0b11...1111 A full bitset.

A intersection would be performed as follows:

  • If neither bitset is allocated on the heap do a bitwise and of the addresses and return the result.
  • If one bitset is heap allocated and the other is full return a clone of the allocated one.
  • If both bitsets are heap allocated return a heap allocated bitset.

If most our users only use the lower render layers or all render layers intersections and clones would operate without allocating.

Pointer provenance shouldn't be an issue since we only ever dereference pointers whose addresses we haven't modified in any way.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-SME This is currently waiting for an SME to resolve something controversial labels May 16, 2024
@alice-i-cecile

Copy link
Copy Markdown
Member

The powers that SME have decided that we should merge this as is, and tackle perf follow-ups in future PRs if they become meaningful (or someone wants to get nerd-sniped).

Merging!

@alice-i-cecile
alice-i-cecile added this pull request to the merge queue May 16, 2024
Merged via the queue into bevyengine:main with commit 4c3b767 May 16, 2024
github-merge-queue Bot pushed a commit that referenced this pull request May 16, 2024
# Objective

- in example `render_to_texture`, #13317 changed the comment on the
existing light saying lights don't work on multiple layers, then add a
light on multiple layers explaining that it will work. it's confusing

## Solution

- Keep the original light, with the updated comment

## Testing

- Run example `render_to_texture`, lighting is correct
@bugsweeper

bugsweeper commented May 31, 2024

Copy link
Copy Markdown
Contributor

@alice-i-cecile doc for RenderLayers still contains mention of TOTAL_LAYERS, which doesn't exist

@alice-i-cecile

Copy link
Copy Markdown
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1316 if you'd like to help out.

zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jun 24, 2024
zhaop added a commit to zhaop/bevy_editor_pls that referenced this pull request Jun 24, 2024
jakobhellermann pushed a commit to jakobhellermann/bevy_editor_pls that referenced this pull request Aug 11, 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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants