Repository navigation
Conversation
BenjaminBrienen
added
A-ECS
Carter0
left a comment
Carter0
left a comment
There was a problem hiding this comment.
This broadly makes sense to me. Someone with more knowledge of our ECS internals to take a look tho.
| self.location = unsafe { self.remove_bundle(bundle_info, caller) }; | ||
| // SAFETY: the `BundleInfo` for this `bundle_id` is initialized above | ||
| unsafe { | ||
| self.remove_bundle(bundle_id, caller); |
There was a problem hiding this comment.
Not a big deal, but if you put the ; outside the unsafe block then rustfmt might put it all on one line. (Here and below.)
There was a problem hiding this comment.
Slightly different semantics though; semicolon-outside is used to return something from the block to a variable.
I don't know how much we actually care about that, but we seem to follow it in this file at least.
| /// Moves the entity to a new archetype, where the new archetype | ||
| /// was a result of removing components from the entity's current archetype. | ||
| /// | ||
| /// When `DROP` is `true`, removed components will be dropped. |
There was a problem hiding this comment.
The notes about DROP may have been in the "Safety" section because it would be unsound to drop the values if they've already been moved out.
On the other hand, having the component data be valid is the normal state. The fact that this doesn't drop when false is just an extra behavior used to justify the safety requirements for the take_component call.
So, yup, moving the notes on DROP out of the "safety" section is an improvement! (Sorry for the noise; the safety logic here is a little subtle and I wanted to write down my train of thought when reviewing.)
|
@alice-i-cecile, think this is small enough to slip into 0.16? Also, I'd like to formally request the powers of labelling, if you wouldn't mind. |
alice-i-cecile
added
S-Ready-For-Final-Review
|
The power of labelling has been bequeathed onto you (once you accept the invite) 👸🏽 As for 0.16 vs 0.17, I don't think this has any end user impact, so I'd like to leave this until 0.17 to avoid introducing unforeseen last-minute bugs <3 It'll get merged in my round-up once we start that though! |
|
This looks good to me, and I'm on board with the change, but if we plan to merge #18521, this would be a moot point. (That pr removes In other words, if we merge this, fixing merge conflicts with #18521 would just be undoing this. I'd be happy to do that if we're still undecided on that PR, but in the end, I think this is an either or situation. I don't think we can do both IIUC. Up to y'all which one you want to do. |
|
I prefer #18521, so I'm going to close this out :) |
Objective
Fixes #17345
The "probably more safety stuff" was probably referring to the
EntityLocationneeding to be valid for the givenEntityor something like that, but that's not necessary if we just give it the actualEntityWorldMut.Solution
&mut selftomove_entity_from_removeparameters.self.locationfield of theEntityWorldMut, which wasn't necessarily the case before.EntityWorldMut::takepassed in a mutable reference to the field, so it already worked like that.EntityWorldMut::remove_bundlepassed in a mutable reference to a local variable, which it then returned so that its caller could use it to update thelocationfield. This seems unnecessarily convoluted.EntityWorldMut::remove_bundleno longer returns the entity's location, as it is now updated internally.unsafeblocks to some calls that had a safety note with no block.