Skip to content

[Merged by Bors] - EntityMut: rename remove_intersection to remove and remove to take #7810

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
Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - EntityMut: rename remove_intersection to remove and remove to take #7810

wants to merge 4 commits into from

Conversation

@maniwani
Copy link
Contributor

@maniwani maniwani commented Feb 24, 2023

Objective

  • A more intuitive distinction between the two. remove_intersection is verbose and unclear.
  • EntityMut::remove and Commands::remove should match.

Solution

  • What the title says.

Migration Guide

Before

fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).remove::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove_intersection::<Parent>();
        }
    }
}

After

fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).take::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove::<Parent>();
        }
    }
}

@maniwani maniwani changed the title Rename remove_intersection to remove and remove to take EntityMut: rename remove_intersection to remove and remove to take Feb 24, 2023
@maniwani maniwani added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 24, 2023
@maniwani maniwani requested a review from MrGVSV February 24, 2023 18:02
@maniwani
Copy link
Contributor Author

maniwani commented Feb 24, 2023 •
edited
Loading

Just want to make sure I didn't choose the wrong method in bevy_reflect.

MrGVSV
MrGVSV approved these changes Feb 24, 2023
View reviewed changes
crates/bevy_ecs/src/reflect.rs
Comment on lines -102 to -105
///
/// # Panics
///
/// Panics if there is no [`Component`] of the given type.
Copy link
Member

@MrGVSV MrGVSV Feb 24, 2023

Choose a reason for hiding this comment

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

So entity.remove::<T>() won't panic if the component doesn't exist on the entity? (Did it even panic before?)

Copy link
Contributor Author

@maniwani maniwani Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, I don't think remove ever panicked (well, ig if you call unwrap when it returns None).

But there's a subtle difference when T is a tuple/bundle.

remove (take) does not remove anything unless the entity has all the components in the bundle. remove_intersection (remove) will remove the ones it does have.

It wasn't clear to me which one bevy_reflect should use.

Copy link
Member

@MrGVSV MrGVSV Feb 24, 2023

Choose a reason for hiding this comment

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

Gotcha. Well ReflectComponent only operates on a single component anyways, so either should work fine 😄

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 24, 2023
joseph-gio
joseph-gio suggested changes Feb 24, 2023
View reviewed changes
@james7132
Copy link
Member

james7132 commented Feb 25, 2023

@maniwani mind fixing the CI failures? Will merge after that, though we may need to manually add the changes into the 0.10 migration guide.

@james7132 james7132 added this to the 0.10 milestone Feb 25, 2023
@james7132
Copy link
Member

james7132 commented Feb 25, 2023

bors r+

bors bot pushed a commit that referenced this pull request Feb 25, 2023
@maniwani
EntityMut: rename remove_intersection to remove and remove to…
aa52059 
… `take` (#7810)

# Objective

- A more intuitive distinction between the two. `remove_intersection` is verbose and unclear.
- `EntityMut::remove` and `Commands::remove` should match.


## Solution

- What the title says.

---

## Migration Guide

Before
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).remove::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove_intersection::<Parent>();
        }
    }
}
```

After
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).take::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove::<Parent>();
        }
    }
}
```
@bors
Copy link
Contributor

bors bot commented Feb 25, 2023

Build failed:

  • run-examples

@mockersf
Copy link
Member

mockersf commented Feb 26, 2023

bors r+

bors bot pushed a commit that referenced this pull request Feb 26, 2023
@maniwani
EntityMut: rename remove_intersection to remove and remove to…
be22569 
… `take` (#7810)

# Objective

- A more intuitive distinction between the two. `remove_intersection` is verbose and unclear.
- `EntityMut::remove` and `Commands::remove` should match.


## Solution

- What the title says.

---

## Migration Guide

Before
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).remove::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove_intersection::<Parent>();
        }
    }
}
```

After
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).take::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove::<Parent>();
        }
    }
}
```
@bors
Copy link
Contributor

bors bot commented Feb 26, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title EntityMut: rename remove_intersection to remove and remove to take [Merged by Bors] - EntityMut: rename remove_intersection to remove and remove to take Feb 26, 2023
@bors bors bot closed this Feb 26, 2023
@maniwani maniwani deleted the remove_and_take branch February 28, 2023 04:16
ProfLander pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@maniwani @ProfLander
EntityMut: rename remove_intersection to remove and remove to…
b5b72aa 
… `take` (bevyengine#7810)

# Objective

- A more intuitive distinction between the two. `remove_intersection` is verbose and unclear.
- `EntityMut::remove` and `Commands::remove` should match.


## Solution

- What the title says.

---

## Migration Guide

Before
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).remove::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove_intersection::<Parent>();
        }
    }
}
```

After
```rust
fn clear_children(parent: Entity, world: &mut World) {
    if let Some(children) = world.entity_mut(parent).take::<Children>() {
        for &child in &children.0 {
            world.entity_mut(child).remove::<Parent>();
        }
    }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@james7132 james7132 james7132 approved these changes

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

+2 more reviewers

@MrGVSV MrGVSV MrGVSV approved these changes

@joseph-gio joseph-gio joseph-gio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Milestone

0.10

Development

Successfully merging this pull request may close these issues.

6 participants

@maniwani @james7132 @mockersf @alice-i-cecile @joseph-gio @MrGVSV