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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Oct 15, 2024
Merged

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

merged 3 commits into from
Oct 15, 2024

Conversation

tychedelia
Copy link
Contributor

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.

@tychedelia 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 tychedelia added this to the 0.15 milestone Oct 14, 2024
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Oct 14, 2024
alice-i-cecile
alice-i-cecile approved these changes Oct 14, 2024
View reviewed changes
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.

I like this fix! Simple and relatively robust.

@alice-i-cecile
Copy link
Member

Does this fully fix #15871?

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

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.

JMS55
JMS55 reviewed Oct 14, 2024
View reviewed changes
crates/bevy_render/src/extract_component.rs
@@ -205,6 +205,8 @@ fn extract_components<C: ExtractComponent>(
for (entity, query_item) in &query {
if let Some(component) = C::extract_component(query_item) {
values.push((entity, component));
} else {
commands.entity(entity).remove::<C>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

No-op.

IceSentry
IceSentry reviewed Oct 14, 2024
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I think this behaviour should be documented on the ExtractComponent trait

@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 15, 2024
@tychedelia tychedelia requested a review from IceSentry October 15, 2024 03:46
@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-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 15, 2024
@alice-i-cecile 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
29 checks passed
akimakinai
akimakinai reviewed Oct 15, 2024
View reviewed changes
crates/bevy_render/src/extract_component.rs
@@ -205,6 +206,8 @@ fn extract_components<C: ExtractComponent>(
for (entity, query_item) in &query {
if let Some(component) = C::extract_component(query_item) {
values.push((entity, component));
} else {
commands.entity(entity).remove::<C>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@akimakinai akimakinai mentioned this pull request Oct 15, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 16, 2024
@akimakinai
Remove ExtractComponent::Out (#15926)
c78886e 
# 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.
@akimakinai akimakinai mentioned this pull request Oct 16, 2024
Merged
8 tasks
github-merge-queue bot pushed a commit that referenced this pull request Oct 19, 2024
@akimakinai
Remove components if not extracted (#15948)
61350cd 
# 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 tychedelia mentioned this pull request Oct 19, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@akimakinai akimakinai akimakinai left review comments

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

@JMS55 JMS55 JMS55 approved these changes

@Trashtalk217 Trashtalk217 Awaiting requested review from Trashtalk217

@IceSentry IceSentry Awaiting requested review from IceSentry

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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
Rendering
Status: Done
Milestone
0.15
Development

Successfully merging this pull request may close these issues.
6 participants
@tychedelia @alice-i-cecile @IceSentry @BenjaminBrienen @JMS55 @akimakinai