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

Cold Specialization #17567

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

Conversation

tychedelia
Copy link
Contributor

Cold Specialization

Objective

An ongoing part of our quest to retain everything in the render world, cold-specialization aims to cache pipeline specialization so that pipeline IDs can be recomputed only when necessary, rather than every frame. This approach reduces redundant work in stable scenes, while still accommodating scenarios in which materials, views, or visibility might change, as well as unlocking future optimization work like retaining render bins.

Solution

Queue systems are split into a specialization system and queue system, the former of which only runs when necessary to compute a new pipeline id. Pipelines are invalidated using a combination of change detection and ECS ticks.

The difficulty with change detection

Detecting “what changed” can be tricky because pipeline specialization depends not only on the entity’s components (e.g., mesh, material, etc.) but also on which view (camera) it is rendering in. In other words, the cache key for a given pipeline id is a view entity/render entity pair. As such, it's not sufficient simply to react to change detection in order to specialize -- an entity could currently be out of view or could be rendered in the future in camera that is currently disabled or hasn't spawned yet.

Why ticks?

Ticks allow us to ensure correctness by allowing us to compare the last time a view or entity was updated compared to the cached pipeline id. This ensures that even if an entity was out of view or has never been seen in a given camera before we can still correctly determine whether it needs to be re-specialized or not.

Testing

TODO: Tested a bunch of different examples, need to test more.

Migration Guide

TODO

  • AssetEvents has been moved into the PostUpdate schedule.

@tychedelia tychedelia added A-Rendering Drawing game state to the screen D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 27, 2025
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

A couple of comments to start with. I'll have more later when I take a closer look.

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Show resolved Hide resolved
.render_lightmaps
.get(&entity)
.map(|lightmap| lightmap.slab_index);
self.shared.lightmap_slab_index = lightmap_slab_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

Check pass ordering here. Are we sure that render_lightmaps is populated before this runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will check. I also think that Lightmap needs to be added to the invalidation logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're good, as RenderLightmaps is populated during the extraction phase, and RenderMeshInstanceGpuBuilder::update is called during the collection subphase of the render phase.

@alice-i-cecile alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Release-Note Work that should be called out in the blog due to impact labels Jan 27, 2025
Copy link
Contributor

@pcwalton pcwalton left a comment

Choose a reason for hiding this comment

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

This is a more thorough review. I mostly just had some simplification suggestions that you can do if you want. This is great stuff.

crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/material.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/prepass/mod.rs Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/material.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/material.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/material.rs Outdated Show resolved Hide resolved
crates/bevy_sprite/src/mesh2d/mesh.rs Outdated Show resolved Hide resolved
.render_lightmaps
.get(&entity)
.map(|lightmap| lightmap.slab_index);
self.shared.lightmap_slab_index = lightmap_slab_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're good, as RenderLightmaps is populated during the extraction phase, and RenderMeshInstanceGpuBuilder::update is called during the collection subphase of the render phase.

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-Performance A change motivated by improving speed, memory usage or compile times D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants