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 2 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
65 changes: 64 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,50 @@ 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,
}

// 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 raw reference
Copy link
Member

Choose a reason for hiding this comment

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

this is a _really_ sketchy safety comment imo, its relying on things not being possible in the current API which could definitely be added in the future. Do we actually need this function? It doesn't seem like it from the changes in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

How would having a raw reference to the world ever allow adding !Send resources.

Also how would you propose we access the world in a scene without this method?

Copy link
Member

Choose a reason for hiding this comment

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

Well resources have no need to fragment archetypes, which is the main problem with &World allowed add/removes on components.

After looking over the code again I realise that you have &Turtle not Turtle so yeah I dunno what to do here but I think this safety comment is not going to end well

Copy link
Member

Choose a reason for hiding this comment

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

Also, "raw" reference feels like poor wording, maybe immutable reference or just reference would work 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.

Yeah, not sure why I used the term raw here.

Also, in my view, the main challenge of &World allowing adds and removes is actually synchronisation; currently the synchronisation is outside of the World (for good reason). You seem to be suggesting that there are plans to add the synchronisation back into the World?

// 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