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

[Merged by Bors] - Add From<EntityMut> for EntityRef (fixes #5459) #5461

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Add From<EntityMut> for EntityRef (fixes #5459) #5461

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b55ae4
Add From<EntityMut> for EntityRef (fixes #5459)
Moulberry Jul 26, 2022
0163bdd
Use EntityRef::new instead of struct syntax
Moulberry Jul 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/bevy_ecs/src/world/entity_ref.rs
View file
Open in desktop
Expand Up @@ -128,6 +128,16 @@ impl<'w> EntityRef<'w> {
}
}

impl<'w> From<EntityMut<'w>> for EntityRef<'w> {
fn from(entity_mut: EntityMut<'w>) -> EntityRef<'w> {
EntityRef {
world: entity_mut.world,
entity: entity_mut.entity,
location: entity_mut.location,
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd much rather that this use the safe path.

let entity = self.entity;
self.world().get_entity_or_whatever_the_method_is(entity)

In that case, I think we should have it as a method instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify? Are you asking to use EntityRef::new, or something else entirely?
It seems as though the alternative "safe path" is just doing another lookup for no reason. Why is it "safe" and the proposed change is not?

Copy link
Member

Choose a reason for hiding this comment

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

We definitely should use EntityRef::new (which also should probably be marked unsafe, since the Location being 'valid' is "relied" upon for safety).

Looking more closely (not on mobile), I do think we can get away with avoiding the other lookup.

I'm still not sure it should be a From impl either really, given that it should still be valid so long as it takes an exclusive refernece to self.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please provide justifications for your proposals

Copy link
Member

Choose a reason for hiding this comment

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

Yeah; I'm not sure I agree with you here. The invariants of EntityMut and EntityRef will always be tied closely together; converting into the immutable form should always be straightforward and safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DJMcNab can this be marked as resolved?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't like that it uses the struct syntax, but I can see that others disagree. I do agree that this entire module has insufficient safety comments, but I hardly see how that's justification to make it worse.
But like I say, that seems to be fighting against the tide.

Fwiw, I'm still not convinced that the owned to owned path is the most useful/idiomatic, or that it should be a From impl. I know it's not quite the same, but std::sync::Weak doesn't have a From<std::sync::Arc> impl.

But again, I've been overruled.

I guess we can resolve it, although I can't say I'm super pleased.

Copy link
Member

@mockersf mockersf Jul 29, 2022 •
edited

Choose a reason for hiding this comment

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

I agree on using EntityRef::new rather than building the struct manually. It would help to not miss when adding controls/safety to the new later. Building the struct here is only possible because the two are in the same module, but they should be considered to be in different module for privacy/access to fields

Copy link
Member

Choose a reason for hiding this comment

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

I'm convinced; let's go with EntityRef::new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0163bdd

}
}

/// A mutable reference to a particular [`Entity`] and all of its components
pub struct EntityMut<'w> {
world: &'w mut World,
Expand Down