Revert "Have EntityCommands methods consume self for easier chaining" #15523
Conversation
| @@ -1405,8 +1407,8 @@ impl EntityCommands<'_> { | |||
| /// # bevy_ecs::system::assert_is_system(remove_character_system); | |||
| /// ``` | |||
| #[track_caller] | |||
| pub fn despawn(self) -> Self { | |||
There was a problem hiding this comment.
#14897 mistakenly made despawn return self
There was a problem hiding this comment.
For context, this wasn't a mistake, it was something acknowledged in the PR: #14897 (comment)
| .with_scale(Vec3::new(1.0, 1.0, 0.5)), | ||
| ..default() | ||
| .insert_if(CameraTracked, || i == 0) | ||
| .with_children(|parent| { |
There was a problem hiding this comment.
Only indentation/whitespace changes below this comment in this file :)
gits diffs do a surprisingly bad job with indentation changes
|
Could you provide details as to why this revert should be merged in your PR description? |
MrGVSV
added
A-ECS
| self.spawn_empty().insert(bundle) | ||
| let mut entity = self.spawn_empty(); | ||
| entity.insert(bundle); | ||
| entity |
There was a problem hiding this comment.
IMO the only clear win for consuming self, but that's because the spawn methods return EntityCommands by value either way
Apologies, I forgot to link to the relevant issue. Added to the PR description. |
tim-blackbird
added
X-Blessed
|
I'm hoping the issue in question gets a little more time to discuss before this goes through. |
|
Yep, I intend to leave this discussion open in the linked issue until the release candidate is due to be cut. Given the level of controversy and breakage, I don't think it should have been merged without more discussion, so the default stance here is "revert". |
There was a problem hiding this comment.
I compared the diff to the one from #14897, and this does revert the &mut self to self changes while preserving the insert_if changes. (The changes to EntityEntryCommands aren't exactly a revert, but seem to be in the same spirit.)
| pub fn and_modify(mut self, modify: impl FnOnce(Mut<T>) + Send + Sync + 'static) -> Self { | ||
| self.entity_commands = self | ||
| .entity_commands | ||
| pub fn and_modify(&mut self, modify: impl FnOnce(Mut<T>) + Send + Sync + 'static) -> &mut Self { |
There was a problem hiding this comment.
I think this makes sense, but I want to highlight that the changes to EntityEntryCommands aren't exactly a revert. This type didn't exist at the time of the original change, and it could make sense for it to work differently from EntityCommands.
alice-i-cecile
added
S-Ready-For-Final-Review
|
@tim-blackbird I'll merge this once you resolve the merge conflicts :) |
As discussed in #15521
selfinsert_ifmethod is keptThe migration guide of #14897 should be removed
Closes #15521