Attempt to remove component from render world if not extracted. by tychedelia · Pull Request #15904 · bevyengine/bevy

tychedelia · 2024-10-14T16:50:18Z

Objective

Ensure that components that are conditionally extracted do not linger in the render world when not extracted from the main world.

Solution

If the ExtractComponent returns None, we'll remove the render world component. I think this is the most sensible behavior here. In the future if there really is a use case for keeping the previous render component around, we could add a Option<Self::Out> parameter for the previous render component to the method, or something similar. I think that this follows the principle of least surprise here relative to what None would suggest and the way that render nodes are typically written. The alternative would be to add an enabled field to pretty much every camera settings component, or duplicate the extraction condition as #15856 does.

Testing

transmission no longer crashes.

Migration Guide

Components that implement ExtractComponent and return None will cause the extracted component to be removed from the render world.

alice-i-cecile

I like this fix! Simple and relatively robust.

alice-i-cecile · 2024-10-14T16:53:10Z

Does this fully fix #15871?

tychedelia · 2024-10-14T16:55:16Z

Does this fully fix #15871?

Only the ones that implement ExtractComponent (which is most of them), theres a few that are still manual extraction systems we need to audit.

crates/bevy_render/src/extract_component.rs

JMS55 · 2024-10-14T17:56:19Z

crates/bevy_render/src/extract_component.rs

        if let Some(component) = C::extract_component(query_item) {
            values.push((entity, component));
+        } else {
+            commands.entity(entity).remove::<C>();


@alice-i-cecile What is the behavior of remove() when the entity lacks the component already? No-op, or panic?

The docs don't say: https://dev-docs.bevyengine.org/bevy_ecs/system/commands/struct.EntityCommands.html#method.remove

IceSentry

I think this behaviour should be documented on the ExtractComponent trait

akimakinai · 2024-10-15T13:05:50Z

crates/bevy_render/src/extract_component.rs

        if let Some(component) = C::extract_component(query_item) {
            values.push((entity, component));
+        } else {
+            commands.entity(entity).remove::<C>();


C: ExtractComponent inserts C::Out instead of C so this might need to be remove::<C::Out>.

bevy/crates/bevy_core_pipeline/src/contrast_adaptive_sharpening/mod.rs

Lines 82 to 85 in 813c759

impl ExtractComponent for ContrastAdaptiveSharpening {

type QueryData = &'static Self;

type QueryFilter = With<Camera>;

type Out = (DenoiseCas, CasUniform);

Gosh duh. In many cases they're the same but good catch.

# Objective - `C: ExtractComponent` inserts `C::Out` instead of `C`, so we need to remove `C::Out`. cc #15904. ## Solution - `C` -> `C::Out` ## Testing - CAS has `<ContrastAdaptiveSharpening as ExtractComponent>::Out = (DenoiseCas, CasUniform)`. Setting its strength to zero correctly removes the effect after this change.

# Objective - Fixes #15871 (Camera is done in #15946) ## Solution - Do the same as #15904 for other extraction systems - Added missing `SyncComponentPlugin` for DOF, TAA, and SSAO (According to the [documentation](https://dev-docs.bevyengine.org/bevy/render/sync_component/struct.SyncComponentPlugin.html), this plugin "needs to be added for manual extraction implementations." We may need to check this is done.) ## Testing Modified example locally to add toggles if not exist. - [x] DOF - toggling DOF component and perspective in `depth_of_field` example - [x] TAA - toggling `Camera.is_active` and TAA component - [x] clusters - not entirely sure, toggling `Camera.is_active` in `many_lights` example (no crash/glitch even without this PR) - [x] previous_view - toggling `Camera.is_active` in `skybox` (no crash/glitch even without this PR) - [x] lights - toggling `Visibility` of `DirectionalLight` in `lighting` example - [x] SSAO - toggling `Camera.is_active` and SSAO component in `ssao` example - [x] default UI camera view - toggling `Camera.is_active` (nop without #15946 because UI defaults to some camera even if `DefaultCameraView` is not there) - [x] volumetric fog - toggling existence of volumetric light. Looks like optimization, no change in behavior/visuals

tychedelia 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 Oct 14, 2024

tychedelia added this to the 0.15 milestone Oct 14, 2024

alice-i-cecile requested review from IceSentry, JMS55 and Trashtalk217 October 14, 2024 16:51

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Oct 14, 2024

alice-i-cecile approved these changes Oct 14, 2024

View reviewed changes

Attempt to remove component from render world if not extracted.

9ec9c7b

tychedelia force-pushed the 15871-conditional-extraction branch from 8f57da9 to 9ec9c7b Compare October 14, 2024 16:54

akimakinai reviewed Oct 14, 2024

View reviewed changes

crates/bevy_render/src/extract_component.rs Show resolved Hide resolved

JMS55 reviewed Oct 14, 2024

View reviewed changes

IceSentry reviewed Oct 14, 2024

View reviewed changes

Correct docs.

28c19ac

BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 15, 2024

BenjaminBrienen approved these changes Oct 15, 2024

View reviewed changes

tychedelia requested a review from IceSentry October 15, 2024 03:46

Merge branch 'main' into 15871-conditional-extraction

f791bf3

JMS55 approved these changes Oct 15, 2024

View reviewed changes

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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 15, 2024

alice-i-cecile added this pull request to the merge queue Oct 15, 2024

Merged via the queue into bevyengine:main with commit acbed60 Oct 15, 2024

akimakinai reviewed Oct 15, 2024

View reviewed changes

akimakinai mentioned this pull request Oct 15, 2024

Remove ExtractComponent::Out #15926

Merged

akimakinai mentioned this pull request Oct 16, 2024

Remove components if not extracted #15948

Merged

8 tasks

tychedelia mentioned this pull request Oct 19, 2024

Add RenderComponent trait for transient render world marker components #16011

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Attempt to remove component from render world if not extracted.#15904

Attempt to remove component from render world if not extracted.#15904
alice-i-cecile merged 3 commits intobevyengine:mainfrom
tychedelia:15871-conditional-extraction

tychedelia commented Oct 14, 2024

Uh oh!

alice-i-cecile left a comment

Uh oh!

alice-i-cecile commented Oct 14, 2024

Uh oh!

tychedelia commented Oct 14, 2024

Uh oh!

Uh oh!

JMS55 Oct 14, 2024

Uh oh!

alice-i-cecile Oct 14, 2024

Uh oh!

IceSentry left a comment

Uh oh!

akimakinai Oct 15, 2024

Uh oh!

tychedelia Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	impl ExtractComponent for ContrastAdaptiveSharpening {
	type QueryData = &'static Self;
	type QueryFilter = With<Camera>;
	type Out = (DenoiseCas, CasUniform);

Uh oh!

Conversation

tychedelia commented Oct 14, 2024

Objective

Solution

Testing

Migration Guide

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 14, 2024

Uh oh!

tychedelia commented Oct 14, 2024

Uh oh!

Uh oh!

JMS55 Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

akimakinai Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

tychedelia Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants