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] - Add From<EntityMut> for EntityRef (fixes #5459) #5461

Closed
wants to merge 2 commits into from

Conversation

Moulberry
Copy link
Contributor

@Moulberry Moulberry commented Jul 26, 2022

Add From for EntityRef (fixes #5459)

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I strongly dislike having a different place where EntityRef is constructed using the struct syntax.

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

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.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 27, 2022
@alice-i-cecile alice-i-cecile 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 Jul 30, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 3, 2022
Add From<EntityMut> for EntityRef (fixes #5459)
@bors bors bot changed the title Add From<EntityMut> for EntityRef (fixes #5459) [Merged by Bors] - Add From<EntityMut> for EntityRef (fixes #5459) Sep 3, 2022
@bors bors bot closed this Sep 3, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
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-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Development

Successfully merging this pull request may close these issues.

Conversion of EntityMut into EntityRef
5 participants