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

Do not incorrectly impl Send for World #3519

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/bevy_core_pipeline/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,10 @@ impl CachedPipelinePhaseItem for Transparent3d {
}
}

pub fn extract_clear_color(clear_color: Res<ClearColor>, mut render_world: ResMut<RenderWorld>) {
pub fn extract_clear_color(
clear_color: Res<ClearColor>,
mut render_world: NonSendMut<RenderWorld>,
) {
// If the clear color has changed
if clear_color.is_changed() {
// Update the clear color resource in the render world
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,13 @@ impl Components {

ComponentId(*index)
}

pub(crate) fn non_send_components(&'_ self) -> impl Iterator<Item = ComponentId> + '_ {
Copy link
Member

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.

self.components
.iter()
.filter(|x| !x.is_send_and_sync())
.map(|x| x.id())
}
}

#[derive(Clone, Debug)]
Expand Down
12 changes: 0 additions & 12 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1193,18 +1193,6 @@ mod tests {
assert_eq!(*world.get_non_send_resource_mut::<i64>().unwrap(), 456);
}

#[test]
#[should_panic]
fn non_send_resource_panic() {
let mut world = World::default();
world.insert_non_send(0i32);
std::thread::spawn(move || {
let _ = world.get_non_send_resource_mut::<i32>();
})
.join()
.unwrap();
}

#[test]
fn trackers_query() {
let mut world = World::default();
Expand Down
67 changes: 66 additions & 1 deletion crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I wonder if this method or a From impl is better...

Copy link
Member Author

Choose a reason for hiding this comment

The 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 {
Expand All @@ -1159,9 +1181,52 @@ impl fmt::Debug for World {
}
}

unsafe impl Send for World {}
unsafe impl Sync for World {}
Copy link
Member

Choose a reason for hiding this comment

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

Just so I understand: why do we still want the Sync impl for World?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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 ThreadSafeWorld or something?

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

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

it is turtles all the way down

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to leave a comment in support of Turtles. It's personal for me, given my avatar.

Copy link
Contributor

Choose a reason for hiding this comment

The 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]
Expand Down
8 changes: 6 additions & 2 deletions crates/bevy_gltf/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,12 @@ async fn load_gltf<'a, 'b>(
if let Some(Err(err)) = err {
return Err(err);
}
let scene_handle = load_context
.set_labeled_asset(&scene_label(&scene), LoadedAsset::new(Scene::new(world)));
let scene_handle = load_context.set_labeled_asset(
&scene_label(&scene),
LoadedAsset::new(Scene {
turtle: world.turtle(),
}),
);

if let Some(name) = scene.name() {
named_scenes.insert(name.to_string(), scene_handle.clone());
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_render/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ impl Plugin for RenderPlugin {
app.insert_resource(device.clone())
.insert_resource(queue.clone())
.insert_resource(options.clone())
.init_resource::<ScratchRenderWorld>()
.init_non_send_resource::<ScratchRenderWorld>()
.register_type::<Frustum>()
.register_type::<CubemapFrusta>();
let render_pipeline_cache = RenderPipelineCache::new(device.clone());
Expand Down Expand Up @@ -303,16 +303,16 @@ fn extract(app_world: &mut World, render_app: &mut App) {
.unwrap();

// temporarily add the render world to the app world as a resource
let scratch_world = app_world.remove_resource::<ScratchRenderWorld>().unwrap();
let scratch_world = app_world.remove_non_send::<ScratchRenderWorld>().unwrap();
let render_world = std::mem::replace(&mut render_app.world, scratch_world.0);
app_world.insert_resource(RenderWorld(render_world));
app_world.insert_non_send(RenderWorld(render_world));

extract.run(app_world);

// add the render world back to the render app
let render_world = app_world.remove_resource::<RenderWorld>().unwrap();
let render_world = app_world.remove_non_send::<RenderWorld>().unwrap();
let scratch_world = std::mem::replace(&mut render_app.world, render_world.0);
app_world.insert_resource(ScratchRenderWorld(scratch_world));
app_world.insert_non_send(ScratchRenderWorld(scratch_world));

extract.apply_buffers(&mut render_app.world);
}
4 changes: 2 additions & 2 deletions crates/bevy_render/src/render_resource/pipeline_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
};
use bevy_app::EventReader;
use bevy_asset::{AssetEvent, Assets, Handle};
use bevy_ecs::system::{Res, ResMut};
use bevy_ecs::system::{NonSendMut, Res, ResMut};
use bevy_utils::{tracing::error, HashMap, HashSet};
use std::{collections::hash_map::Entry, hash::Hash, ops::Deref, sync::Arc};
use thiserror::Error;
Expand Down Expand Up @@ -389,7 +389,7 @@ impl RenderPipelineCache {
}

pub(crate) fn extract_shaders(
mut world: ResMut<RenderWorld>,
mut world: NonSendMut<RenderWorld>,
shaders: Res<Assets<Shader>>,
mut events: EventReader<AssetEvent<Shader>>,
) {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_render/src/view/window.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl DerefMut for ExtractedWindows {
}
}

fn extract_windows(mut render_world: ResMut<RenderWorld>, windows: Res<Windows>) {
fn extract_windows(mut render_world: NonSendMut<RenderWorld>, windows: Res<Windows>) {
let mut extracted_windows = render_world.get_resource_mut::<ExtractedWindows>().unwrap();
for window in windows.iter() {
let (new_width, new_height) = (
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_scene/src/dynamic_scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub struct DynamicEntity {
impl DynamicScene {
/// Create a new dynamic scene from a given scene.
pub fn from_scene(scene: &Scene, type_registry: &TypeRegistryArc) -> Self {
Self::from_world(&scene.world, type_registry)
Self::from_world(scene.turtle.world_ref(), type_registry)
}

/// Create a new dynamic scene from a given world.
Expand Down
10 changes: 2 additions & 8 deletions crates/bevy_scene/src/scene.rs
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,
}
13 changes: 4 additions & 9 deletions crates/bevy_scene/src/scene_spawner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,15 +154,15 @@ impl SceneSpawner {
handle: scene_handle.clone(),
})?;

for archetype in scene.world.archetypes().iter() {
let scene_world = scene.turtle.world_ref();
for archetype in scene_world.archetypes().iter() {
for scene_entity in archetype.entities() {
let entity = *instance_info
.entity_map
.entry(*scene_entity)
.or_insert_with(|| world.spawn().id());
for component_id in archetype.components() {
let component_info = scene
.world
let component_info = scene_world
.components()
.get_info(component_id)
.expect("component_ids in archetypes should have ComponentInfo");
Expand All @@ -179,12 +179,7 @@ impl SceneSpawner {
}
})
})?;
reflect_component.copy_component(
&scene.world,
world,
*scene_entity,
entity,
);
reflect_component.copy_component(scene_world, world, *scene_entity, entity);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_sprite/src/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ pub struct SpriteAssetEvents {
}

pub fn extract_sprite_events(
mut render_world: ResMut<RenderWorld>,
mut render_world: NonSendMut<RenderWorld>,
mut image_events: EventReader<AssetEvent<Image>>,
) {
let mut events = render_world
Expand All @@ -230,7 +230,7 @@ pub fn extract_sprite_events(
}

pub fn extract_sprites(
mut render_world: ResMut<RenderWorld>,
mut render_world: NonSendMut<RenderWorld>,
texture_atlases: Res<Assets<TextureAtlas>>,
sprite_query: Query<(&Visibility, &Sprite, &GlobalTransform, &Handle<Image>)>,
atlas_query: Query<(
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_text/src/text2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use bevy_ecs::{
bundle::Bundle,
entity::Entity,
query::{Changed, QueryState, With},
system::{Local, Query, QuerySet, Res, ResMut},
system::{Local, NonSendMut, Query, QuerySet, Res, ResMut},
};
use bevy_math::{Size, Vec3};
use bevy_render::{texture::Image, view::Visibility, RenderWorld};
Expand Down Expand Up @@ -42,7 +42,7 @@ impl Default for Text2dBundle {
}

pub fn extract_text2d_sprite(
mut render_world: ResMut<RenderWorld>,
mut render_world: NonSendMut<RenderWorld>,
texture_atlases: Res<Assets<TextureAtlas>>,
text_pipeline: Res<DefaultTextPipeline>,
windows: Res<Windows>,
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ui/src/render/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ pub struct ExtractedUiNodes {
}

pub fn extract_uinodes(
mut render_world: ResMut<RenderWorld>,
mut render_world: NonSendMut<RenderWorld>,
images: Res<Assets<Image>>,
uinode_query: Query<(
&Node,
Expand Down Expand Up @@ -173,7 +173,7 @@ pub fn extract_uinodes(
}

pub fn extract_text_uinodes(
mut render_world: ResMut<RenderWorld>,
mut render_world: NonSendMut<RenderWorld>,
texture_atlases: Res<Assets<TextureAtlas>>,
text_pipeline: Res<DefaultTextPipeline>,
windows: Res<Windows>,
Expand Down