[Merged by Bors] - Add From<EntityMut> for EntityRef (fixes #5459) #5461
Conversation
| world: entity_mut.world, | ||
| entity: entity_mut.entity, | ||
| location: entity_mut.location, | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Please provide justifications for your proposals
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@DJMcNab can this be marked as resolved?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm convinced; let's go with EntityRef::new.
alice-i-cecile
added
A-ECS
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
…#5461) Add From<EntityMut> for EntityRef (fixes bevyengine#5459)
…#5461) Add From<EntityMut> for EntityRef (fixes bevyengine#5459)
Add From for EntityRef (fixes #5459)