-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Do not incorrectly impl Send for World
#3519
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1142,6 +1142,28 @@ impl World { | |
self.archetypes.clear_entities(); | ||
self.entities.clear(); | ||
} | ||
|
||
/// Create a version of this [`World`] which can be sent to another thread | ||
/// | ||
/// # Panics | ||
/// | ||
/// If `self` contains any [`!Send`] resources, e.g. from calls to [`World::insert_non_send`] | ||
/// | ||
/// [`!Send`]: Send | ||
|
||
pub fn turtle(self) -> Turtle { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I wonder if this method or a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has the potential to be a relatively expensive operation, so I don't want it to be so implicit. |
||
let non_send = self.components().non_send_components().collect::<Vec<_>>(); | ||
for id in non_send { | ||
assert!( | ||
self.get_populated_resource_column(id).is_none(), | ||
"Tried to create a Turtle from a World containing a !Send resource" | ||
); | ||
} | ||
// Safety: this world does not contain anything !Send, as confirmed by the check above | ||
// In practise this method is used for GLTF loading, which does not add any resources to the given world | ||
// (i.e. the above check is trivially true) | ||
Turtle { world: self } | ||
} | ||
} | ||
|
||
impl fmt::Debug for World { | ||
|
@@ -1159,9 +1181,52 @@ impl fmt::Debug for World { | |
} | ||
} | ||
|
||
unsafe impl Send for World {} | ||
unsafe impl Sync for World {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just so I understand: why do we still want the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The entire idea of our world abstraction is that it is accessed in a shared way across multiple threads. |
||
|
||
/// A world which does not contain any [`!Send`] resources, and therefore | ||
/// can be safely sent between threads. | ||
/// | ||
/// The name turtle is derived from this being something which is moving a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 I... don't hate it? Very cute. Not sure it's terribly descriptive. What about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is quite a niche type, to be fair. I hoped I'd be able to sneak a bit of whimsy through. (I will claim I didn't just create this PR for this joke; whether you believe that is your choice) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is turtles all the way down There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to leave a comment in support of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. amazing |
||
/// [`World`] (between threads) | ||
/// | ||
/// [`!Send`]: Send | ||
#[derive(Debug)] | ||
pub struct Turtle { | ||
// Safety: does not have any !Send resources | ||
world: World, | ||
} | ||
|
||
// I mean I guess clippy, that's the point. There's a reason we called it `unsafe` | ||
#[allow(clippy::non_send_fields_in_send_ty)] | ||
// Safety: The contained world does not contain anything which is !Send | ||
unsafe impl Send for Turtle {} | ||
|
||
impl Turtle { | ||
/// The [`World`] this [`Turtle`] was created from. | ||
/// | ||
/// The returned [`World`] does not contain any [`!Send`] resources | ||
/// | ||
/// [`!Send`]: Send | ||
pub fn world(self) -> World { | ||
self.world | ||
} | ||
|
||
/// A view on this [`Turtle`]'s [`World`], which contains no [`!Send`] resources | ||
/// | ||
/// [`!Send`]: Send | ||
// Safety: NonSend resources cannot be added using a shared reference | ||
// to the world, so this cannot break our invariants | ||
pub fn world_ref(&self) -> &World { | ||
&self.world | ||
} | ||
} | ||
|
||
impl From<Turtle> for World { | ||
fn from(turtle: Turtle) -> Self { | ||
turtle.world() | ||
} | ||
} | ||
|
||
/// Creates `Self` using data from the given [World] | ||
pub trait FromWorld { | ||
/// Creates `Self` using data from the given [World] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,8 @@ | ||
use bevy_ecs::world::World; | ||
use bevy_ecs::world::Turtle; | ||
use bevy_reflect::TypeUuid; | ||
|
||
#[derive(Debug, TypeUuid)] | ||
#[uuid = "c156503c-edd9-4ec7-8d33-dab392df03cd"] | ||
pub struct Scene { | ||
pub world: World, | ||
} | ||
|
||
impl Scene { | ||
pub fn new(world: World) -> Self { | ||
Self { world } | ||
} | ||
pub turtle: Turtle, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This will be useful if we want to relax the bounds on
Component
later too.