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

Add method to remove component and all required components for removed component #15026

Merged
merged 31 commits into from
Oct 3, 2024

Conversation

rewin123
Copy link
Contributor

@rewin123 rewin123 commented Sep 3, 2024

Objective

The new Required Components feature (#14791) in Bevy allows spawning a fixed set of components with a single method with cool require macro. However, there's currently no corresponding method to remove all those components together. This makes it challenging to keep insertion and removal code in sync, especially for simple using cases.

#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
struct Y;

world.entity_mut(e).insert(X); // Spawns both X and Y
world.entity_mut(e).remove::<X>(); 
world.entity_mut(e).remove::<Y>(); // We need to manually remove dependencies without any sync with the `require` macro

Solution

Simplifies component management by providing operations for removal required components.
This PR introduces simple 'footgun' methods to removes all components of this bundle and its required components.

Two new methods are introduced:
For Commands:

commands.entity(e).remove_with_requires::<B>();

For World:

world.entity_mut(e).remove_with_requires::<B>();

For performance I created new field in Bundels struct. This new field "contributed_bundle_ids" contains cached ids for dynamic bundles constructed from bundle_info.cintributed_components()

Testing

The PR includes three test cases:

  1. Removing a single component with requirements using World.
  2. Removing a bundle with requirements using World.
  3. Removing a single component with requirements using Commands.
  4. Removing a single component with runtime requirements using Commands

These tests ensure the feature works as expected across different scenarios.

Showcase

Example:

use bevy_ecs::prelude::*;

#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
#[require(Z)]
struct Y;

#[derive(Component, Default)]
struct Z;

#[derive(Component)]
struct W;

let mut world = World::new();

// Spawn an entity with X, Y, Z, and W components
let entity = world.spawn((X, W)).id();

assert!(world.entity(entity).contains::<X>());
assert!(world.entity(entity).contains::<Y>());
assert!(world.entity(entity).contains::<Z>());
assert!(world.entity(entity).contains::<W>());

// Remove X and required components Y, Z
world.entity_mut(entity).remove_with_requires::<X>();

assert!(!world.entity(entity).contains::<X>());
assert!(!world.entity(entity).contains::<Y>());
assert!(!world.entity(entity).contains::<Z>());

assert!(world.entity(entity).contains::<W>());

Motivation for PR

#15580

Performance

I made simple benchmark

let mut world = World::default();
let entity = world.spawn_empty().id();

let steps = 100_000_000;

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove::<(X, Y, Z, W)>();
}
let end = std::time::Instant::now();
println!("normal remove: {:?} ", (end - start).as_secs_f32());
println!("one remove: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove_with_requires::<X>();
}
let end = std::time::Instant::now();
println!("remove_with_requires: {:?} ", (end - start).as_secs_f32());
println!("one remove_with_requires: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);

Output:

CPU: Amd Ryzen 7 2700x

normal remove: 17.36135 
one remove: 0.17361348299999999 micros
remove_with_requires: 17.534006 
one remove_with_requires: 0.17534005400000002 micros

NOTE: I didn't find any tests or mechanism in the repository to update BundleInfo after creating new runtime requirements with an existing BundleInfo. So this PR also does not contain such logic.

Future work (outside this PR)

Create cache system for fast removing components in "safe" mode, where "safe" mode is remove only required components that will be no longer required after removing root component.

@TrialDragon TrialDragon added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events 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 D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 3, 2024
Copy link
Contributor

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

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

Simplifies component management by providing symmetrical operations for removal required components.

These are not entirely symmetrical, since if X requires Y and Y is already present then inserting X will not replace Y, but later removing X will remove Y as well.

In other words .insert(X).remove_with_required::<X>() is not a guaranteed noop.

I think this is fine, since there's probably no way to avoid this behaviour, but I think it should be mentioned in the documentation for remove_with_required as a potential pitfall.

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
@tim-blackbird
Copy link
Contributor

I'm concerned this might be too foot-gunny in the case of X and Y both requiring Z and then X is removed, leaving Y without Z.
It would be great if we could keep the required components if they were still required by something else.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through X-Controversial There is active debate or serious implications around merging this PR and removed X-Contentious There are nontrivial implications that should be thought through labels Sep 3, 2024
@alice-i-cecile
Copy link
Member

I agree with @tim-blackbird; there's a serious risk that this ends up removing e.g. Transform by accident.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 3, 2024
@rewin123
Copy link
Contributor Author

rewin123 commented Sep 3, 2024

Thank you all. I have read the comments and agree with all of them. I will then try to re-create the method to take into account the dependency tree of components on each other. And improve the documentation.

@rewin123 rewin123 marked this pull request as draft September 3, 2024 14:48
@rewin123 rewin123 marked this pull request as ready for review September 4, 2024 04:00
@rewin123 rewin123 requested a review from SkiFire13 September 4, 2024 04:13
@rewin123
Copy link
Contributor Author

rewin123 commented Sep 4, 2024

@alice-i-cecile @tim-blackbird @SkiFire13

I created component require safe version of remove_with_required function, updated PR description and improved docs. I would appreciate it if you could look at the PR again.

@rewin123 rewin123 removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 4, 2024
@rewin123 rewin123 marked this pull request as ready for review October 2, 2024 14:47
@rewin123 rewin123 requested a review from SkiFire13 October 2, 2024 14:47
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.

Can you try using this PR to clean up the mess left behind in #15572 (comment) ? :)

@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers and removed X-Controversial There is active debate or serious implications around merging this PR labels Oct 2, 2024
@rewin123
Copy link
Contributor Author

rewin123 commented Oct 2, 2024

Can you try using this PR to clean up the mess left behind in #15572 (comment) ? :)

Sure! I can do it in another PR after merging this one, right?)

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I'm on board for this approach, but I have a couple of optimization suggestions

crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/bundle.rs Outdated Show resolved Hide resolved
@rewin123
Copy link
Contributor Author

rewin123 commented Oct 2, 2024

I'm on board for this approach, but I have a couple of optimization suggestions

Thanks for the review! All remarks were justified and fixed

@cart cart added this pull request to the merge queue Oct 3, 2024
Merged via the queue into bevyengine:main with commit 8bf5d99 Oct 3, 2024
26 checks passed
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Oct 4, 2024
…d component (bevyengine#15026)

## Objective
The new Required Components feature (bevyengine#14791) in Bevy allows spawning a
fixed set of components with a single method with cool require macro.
However, there's currently no corresponding method to remove all those
components together. This makes it challenging to keep insertion and
removal code in sync, especially for simple using cases.
```rust
#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
struct Y;

world.entity_mut(e).insert(X); // Spawns both X and Y
world.entity_mut(e).remove::<X>(); 
world.entity_mut(e).remove::<Y>(); // We need to manually remove dependencies without any sync with the `require` macro
```
## Solution
Simplifies component management by providing operations for removal
required components.
This PR introduces simple 'footgun' methods to removes all components of
this bundle and its required components.

Two new methods are introduced:
For Commands:
```rust
commands.entity(e).remove_with_requires::<B>();
```
For World:
```rust
world.entity_mut(e).remove_with_requires::<B>();
```

For performance I created new field in Bundels struct. This new field
"contributed_bundle_ids" contains cached ids for dynamic bundles
constructed from bundle_info.cintributed_components()

## Testing
The PR includes three test cases:

1. Removing a single component with requirements using World.
2. Removing a bundle with requirements using World.
3. Removing a single component with requirements using Commands.
4. Removing a single component with **runtime** requirements using
Commands

These tests ensure the feature works as expected across different
scenarios.

## Showcase
Example:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
#[require(Z)]
struct Y;

#[derive(Component, Default)]
struct Z;

#[derive(Component)]
struct W;

let mut world = World::new();

// Spawn an entity with X, Y, Z, and W components
let entity = world.spawn((X, W)).id();

assert!(world.entity(entity).contains::<X>());
assert!(world.entity(entity).contains::<Y>());
assert!(world.entity(entity).contains::<Z>());
assert!(world.entity(entity).contains::<W>());

// Remove X and required components Y, Z
world.entity_mut(entity).remove_with_requires::<X>();

assert!(!world.entity(entity).contains::<X>());
assert!(!world.entity(entity).contains::<Y>());
assert!(!world.entity(entity).contains::<Z>());

assert!(world.entity(entity).contains::<W>());
```

## Motivation for PR
bevyengine#15580 

## Performance

I made simple benchmark
```rust
let mut world = World::default();
let entity = world.spawn_empty().id();

let steps = 100_000_000;

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove::<(X, Y, Z, W)>();
}
let end = std::time::Instant::now();
println!("normal remove: {:?} ", (end - start).as_secs_f32());
println!("one remove: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove_with_requires::<X>();
}
let end = std::time::Instant::now();
println!("remove_with_requires: {:?} ", (end - start).as_secs_f32());
println!("one remove_with_requires: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);
```

Output:

CPU: Amd Ryzen 7 2700x

```bash
normal remove: 17.36135 
one remove: 0.17361348299999999 micros
remove_with_requires: 17.534006 
one remove_with_requires: 0.17534005400000002 micros
```

NOTE: I didn't find any tests or mechanism in the repository to update
BundleInfo after creating new runtime requirements with an existing
BundleInfo. So this PR also does not contain such logic.

## Future work (outside this PR)

Create cache system for fast removing components in "safe" mode, where
"safe" mode is remove only required components that will be no longer
required after removing root component.

---------

Co-authored-by: a.yamaev <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
…d component (bevyengine#15026)

## Objective
The new Required Components feature (bevyengine#14791) in Bevy allows spawning a
fixed set of components with a single method with cool require macro.
However, there's currently no corresponding method to remove all those
components together. This makes it challenging to keep insertion and
removal code in sync, especially for simple using cases.
```rust
#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
struct Y;

world.entity_mut(e).insert(X); // Spawns both X and Y
world.entity_mut(e).remove::<X>(); 
world.entity_mut(e).remove::<Y>(); // We need to manually remove dependencies without any sync with the `require` macro
```
## Solution
Simplifies component management by providing operations for removal
required components.
This PR introduces simple 'footgun' methods to removes all components of
this bundle and its required components.

Two new methods are introduced:
For Commands:
```rust
commands.entity(e).remove_with_requires::<B>();
```
For World:
```rust
world.entity_mut(e).remove_with_requires::<B>();
```

For performance I created new field in Bundels struct. This new field
"contributed_bundle_ids" contains cached ids for dynamic bundles
constructed from bundle_info.cintributed_components()

## Testing
The PR includes three test cases:

1. Removing a single component with requirements using World.
2. Removing a bundle with requirements using World.
3. Removing a single component with requirements using Commands.
4. Removing a single component with **runtime** requirements using
Commands

These tests ensure the feature works as expected across different
scenarios.

## Showcase
Example:
```rust
use bevy_ecs::prelude::*;

#[derive(Component)]
#[require(Y)]
struct X;

#[derive(Component, Default)]
#[require(Z)]
struct Y;

#[derive(Component, Default)]
struct Z;

#[derive(Component)]
struct W;

let mut world = World::new();

// Spawn an entity with X, Y, Z, and W components
let entity = world.spawn((X, W)).id();

assert!(world.entity(entity).contains::<X>());
assert!(world.entity(entity).contains::<Y>());
assert!(world.entity(entity).contains::<Z>());
assert!(world.entity(entity).contains::<W>());

// Remove X and required components Y, Z
world.entity_mut(entity).remove_with_requires::<X>();

assert!(!world.entity(entity).contains::<X>());
assert!(!world.entity(entity).contains::<Y>());
assert!(!world.entity(entity).contains::<Z>());

assert!(world.entity(entity).contains::<W>());
```

## Motivation for PR
bevyengine#15580 

## Performance

I made simple benchmark
```rust
let mut world = World::default();
let entity = world.spawn_empty().id();

let steps = 100_000_000;

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove::<(X, Y, Z, W)>();
}
let end = std::time::Instant::now();
println!("normal remove: {:?} ", (end - start).as_secs_f32());
println!("one remove: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);

let start = std::time::Instant::now();
for _ in 0..steps {
    world.entity_mut(entity).insert(X);
    world.entity_mut(entity).remove_with_requires::<X>();
}
let end = std::time::Instant::now();
println!("remove_with_requires: {:?} ", (end - start).as_secs_f32());
println!("one remove_with_requires: {:?} micros", (end - start).as_secs_f64() / steps as f64 * 1_000_000.0);
```

Output:

CPU: Amd Ryzen 7 2700x

```bash
normal remove: 17.36135 
one remove: 0.17361348299999999 micros
remove_with_requires: 17.534006 
one remove_with_requires: 0.17534005400000002 micros
```

NOTE: I didn't find any tests or mechanism in the repository to update
BundleInfo after creating new runtime requirements with an existing
BundleInfo. So this PR also does not contain such logic.

## Future work (outside this PR)

Create cache system for fast removing components in "safe" mode, where
"safe" mode is remove only required components that will be no longer
required after removing root component.

---------

Co-authored-by: a.yamaev <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@HackerFoo
Copy link
Contributor

HackerFoo commented Dec 20, 2024

I wrote logic to handle enabling and disabling components with dependencies which may be useful as a reference, based on how package systems typically work:

  • When enabling something, all dependencies are automatically enabled.
  • When disabling something, anything depending on it is also disabled, as well as any dependencies that are only needed to support it.

The logic is in dtask_requested. It relies on statically computed sets of (transitive) dependencies and dependents. Basically, you need to track the set of explicitly enabled/added components, and then from that compute the true set of components, and diff that with the current state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible 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 D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants