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

[Merged by Bors] - Fix unsound EntityMut::remove_children. Add EntityMut::world_scope #6464

Closed
wants to merge 6 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Nov 4, 2022

EntityMut::remove_children does not call self.update_location() which is unsound.
Verified by adding the following assertion, which fails when running the tests.

let before = self.location();
self.update_location();
assert_eq!(before, self.location());

I also removed incorrect messages like "parent entity is not modified" and the unhelpful "Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype" which might lead people to think that's the only thing that can change the entity's location.

Changelog

Added EntityMut::world_scope.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies P-Unsound A bug that results in undefined compiler behavior labels Nov 4, 2022
@nicopap
Copy link
Contributor

nicopap commented Nov 4, 2022

In #5845, I had added a method to update the world safely: with_world_mut, but it never got merged. This allowed removing a lot of unsafe code.

Maybe you could add it? There were some discussion around it, you can find them there: https://github.com/bevyengine/bevy/pull/5845/files

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -543,6 +543,12 @@ impl<'w> EntityMut<'w> {
self.world
}

/// Gives mutable access to this `EntityMut`'s [`World`] in a temporary scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might benefit from explaining why you'd want to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the only way of safely getting a &mut World from an EntityMut without consuming self. Which sort of speaks for itself?
I'm not sure what to add to the docs that could make that clearer (Aside from being very pedantic) :)

Copy link
Member

Choose a reason for hiding this comment

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

You could discuss update_location here, but that's very gory-internals, and likely to go out of date in dangerous ways. If people care, they can read the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking: someone reading the doc doesn't necessarily see the source code. If they encounter this method, they may ask themselves why its here. But I agree with alice, it's really a minor thing and really depends on internals.

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.

bors r+

@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Nov 4, 2022
bors bot pushed a commit that referenced this pull request Nov 4, 2022
`EntityMut::remove_children` does not call `self.update_location()` which is unsound.
Verified by adding the following assertion, which fails when running the tests.
```rust
let before = self.location();
self.update_location();
assert_eq!(before, self.location());
```

I also removed incorrect messages like "parent entity is not modified" and the unhelpful "Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype" which might lead people to think that's the *only* thing that can change the entity's location.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@alice-i-cecile
Copy link
Member

Can you add a change log item for the new world_scope API? It's a nice little public usability improvement.

@bors
Copy link
Contributor

bors bot commented Nov 4, 2022

Build failed:

@tim-blackbird tim-blackbird changed the title Fix unsound EntityMut::remove_children Fix unsound EntityMut::remove_children. Add EntityMut::world_scope Nov 4, 2022
@alice-i-cecile
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Nov 4, 2022
#6464)

`EntityMut::remove_children` does not call `self.update_location()` which is unsound.
Verified by adding the following assertion, which fails when running the tests.
```rust
let before = self.location();
self.update_location();
assert_eq!(before, self.location());
```

I also removed incorrect messages like "parent entity is not modified" and the unhelpful "Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype" which might lead people to think that's the *only* thing that can change the entity's location.

# Changelog
Added `EntityMut::world_scope`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors bors bot changed the title Fix unsound EntityMut::remove_children. Add EntityMut::world_scope [Merged by Bors] - Fix unsound EntityMut::remove_children. Add EntityMut::world_scope Nov 4, 2022
@bors bors bot closed this Nov 4, 2022
@tim-blackbird tim-blackbird deleted the uh-oh-safety branch November 6, 2022 18:22
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
bevyengine#6464)

`EntityMut::remove_children` does not call `self.update_location()` which is unsound.
Verified by adding the following assertion, which fails when running the tests.
```rust
let before = self.location();
self.update_location();
assert_eq!(before, self.location());
```

I also removed incorrect messages like "parent entity is not modified" and the unhelpful "Inserting a bundle in the children entities may change the parent entity's location if they were of the same archetype" which might lead people to think that's the *only* thing that can change the entity's location.

# Changelog
Added `EntityMut::world_scope`.

Co-authored-by: devil-ira <justthecooldude@gmail.com>
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 A-Hierarchy Parent-child entity hierarchies C-Usability A targeted quality-of-life change that makes Bevy easier to use P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants