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

Add resource_scope variant for nonsend resources #6037

Closed
jakobhellermann opened this issue Sep 20, 2022 · 6 comments
Closed

Add resource_scope variant for nonsend resources #6037

jakobhellermann opened this issue Sep 20, 2022 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@jakobhellermann
Copy link
Contributor

jakobhellermann commented Sep 20, 2022

What problem does this solve or what need does it fill?

Bevy has a resource_scope method which lets you have mutable access to a resource and the world at the same time by temporarily removing the resource:

world.resource_scope(|world, mut a: Mut<A>| {
    let b = world.get_mut::<B>(entity).unwrap();
    a.0 += b.0;
});

Is is roughly equivalent to

let a = world.remove_resource::<A>();
f(world, b);
world.insert_resource(a);

Bevy also has the concept of non-send resources; resources which do not implement the Send auto trait (and Sync?) so any systems that access them are only scheduled on the main thread. This is useful for example for some scripting language runtimes or OS operations that can only run on one thread.

There is, however, no variant of resource_scope which works for nonsend resources.

What solution would you like?

Add a non_send_resource_scope function, which is pretty much a copy of resource_scope (

pub fn resource_scope<R: Resource, U>(&mut self, f: impl FnOnce(&mut World, Mut<R>) -> U) -> U {
) but relaxes the R: Resource bound to R: 'static.
The method also needs to validate that it is being run on the main thread, #6037 (comment).

What alternative(s) have you considered?

Write

let runtime = world.remove_non_send_resource::<JsRuntime>().unwrap();
f(runtime, world);
world.insert_non_send_resource(runtime);

instead. Less ergonomic and a tiny bit less performant.

@DJMcNab
Copy link
Member

DJMcNab commented Sep 20, 2022

In theory, resource_scope should just work for NonSend resources - there's no part of that API which requires the type be Send - if you have an &mut World, you're already on the main thread (at least in theory, in practise that's not necessarily true, but we have enough information to panic in those cases).

@jakobhellermann
Copy link
Contributor Author

Hm, right. In that case it should be enough to just remove the R: Resource bound which implies Send + Sync and leave a comment explaining how this is safe without the bound.

@jakobhellermann
Copy link
Contributor Author

        let mut world = World::default();
        world.insert_non_send_resource(A(0));

        std::thread::scope(|_| {
            world.resource_scope(|world: &mut World, mut value: Mut<A>| {
                value.0 += 1;
                assert!(!world.contains_resource::<A>());
            });
            assert_eq!(world.get_non_send_resource::<A>().unwrap().0, 1);
        });

would have to be disallowed, right?
So resource_scope also needs a call to self.validate_non_send_access::<R>();.
But it should be allowed to call resource_scope on another thread for a regular resource.
We can't specialize on R: Send, so is

but we have enough information to panic in those cases)

true?

@DJMcNab
Copy link
Member

DJMcNab commented Sep 21, 2022

We store whether a resource is registered as non_send though? So we read that field to validate?

@jakobhellermann
Copy link
Contributor Author

Ah yes, forgot about that field.
So we need to add

        let component_info = self.components().get_info(component_id).unwrap();
        if !component_info.is_send_and_sync() {
            self.validate_non_send_access::<R>();
        }

@jakobhellermann jakobhellermann added the D-Trivial Nice and easy! A great choice to get started with Bevy label Sep 21, 2022
@Pietrek14
Copy link
Contributor

I would like to try it, since noone is assigned.

@bors bors bot closed this as completed in 0bf7f31 Sep 28, 2022
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 19, 2022
…vyengine#6113)

# Objective

Relaxes the trait bound for `World::resource_scope` to allow non-send resources. Fixes bevyengine#6037.

## Solution

No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread.

---

## Changelog
 - `World::resource_scope` accepts non-send resources now
 - `World::resource_scope` verifies non-send access if the resource is non-send
 - Two new tests are added, one for valid use of `World::resource_scope` with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic)

Co-authored-by: Dawid Piotrowski <41804418+Pietrek14@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this issue Oct 28, 2022
…vyengine#6113)

# Objective

Relaxes the trait bound for `World::resource_scope` to allow non-send resources. Fixes bevyengine#6037.

## Solution

No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread.

---

## Changelog
 - `World::resource_scope` accepts non-send resources now
 - `World::resource_scope` verifies non-send access if the resource is non-send
 - Two new tests are added, one for valid use of `World::resource_scope` with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic)

Co-authored-by: Dawid Piotrowski <41804418+Pietrek14@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…vyengine#6113)

# Objective

Relaxes the trait bound for `World::resource_scope` to allow non-send resources. Fixes bevyengine#6037.

## Solution

No big changes in code had to be made. Added a check so that the non-send resources won't be accessed from a different thread.

---

## Changelog
 - `World::resource_scope` accepts non-send resources now
 - `World::resource_scope` verifies non-send access if the resource is non-send
 - Two new tests are added, one for valid use of `World::resource_scope` with a non-send resource, and one for invalid use (calling it from a different thread, resulting in panic)

Co-authored-by: Dawid Piotrowski <41804418+Pietrek14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants