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

Upstream bevy_command_non_send #12795

Open
BD103 opened this issue Mar 30, 2024 · 3 comments
Open

Upstream bevy_command_non_send #12795

BD103 opened this issue Mar 30, 2024 · 3 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes

Comments

@BD103
Copy link
Member

BD103 commented Mar 30, 2024

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

The only current way to insert or remove a non-send resource while the App is running is to create a system that takes &mut World. &mut World systems have exclusive access to the world, which reduces the amount of parallelism that can be achieved.

For comparison, Resource has Commands::insert_resource, which allows deferring all resource operations to occur at the same time.

I believe the historical reason non-send operations were excluded from Commands is because Command has a Send bound.

What solution would you like?

I recently created a library named bevy_command_non_send, which adds methods for interacting with non-send resources using commands. I believe it would be useful to others and worth upstreaming into the main Commands implementation. This would involve introducing 3 new methods:

impl Commands<'_, '_> {
    fn init_non_send_resource<R: FromWorld + 'static>(&mut self);
    fn insert_non_send_resource<F, R>(&mut self, func: F)
       where F: FnOnce() -> R + Send + 'static,
             R: 'static;
    fn remove_non_send_resource<R: 'static>(&mut self);
}

The exact implementation can be viewed here. init_non_send_resource and remove_non_send_resource are simple enough because the system never gains access to the non-send resource, so there are no issues with the data crossing between threads. insert_non_send_resource is the only real issue, since it allows the system to construct the non-send resource before inserting it into the World.

My solution to this was to defer creating the resource until it can be made on the main thread. insert_non_send_resource will take a closure that is Send, but can return a non-send resource. Calling it would look like this:

struct MyNonSend(*const u8);

fn create_my_non_send(mut commands: Commands) {
    commands.insert_non_send_resource(|| {
        MyNonSend(std::ptr::null())
    });
}

What alternative(s) have you considered?

I believe at least init_non_send_resource and remove_non_send_resource should be implemented. I can understand if insert_non_send_resource requires further consideration, since it is the most complicated of proposed API.

I think non-send resources are a bit of a sore spot right now, but supporting them with Commands will make them more on-pace with the universal Resource. I cannot think of any clear alternatives, but feel free to add your thoughts in a comment if I missed anything.

Additional context

For your convenience, I have published the documentation for bevy_command_non_send here. It comes complete with rigorous documentation and a test suite, which you can view by browsing the source.

Though I say "upstream" in this feature request, I really mean taking heavy inspiration. I don't intend on the functions to be made public in bevy_ecs, since other commands that are built-in are not as well. Instead, I want the functionality of CommandsExt to be directly implemented for Commands.

I originally came up with this idea while working with integrating cpal with Bevy. I was trying to get a microphone working, which required keeping a non-send Stream type alive for the course of the app. This would happen during Startup, but I did not want to claim the entire &mut World for myself. Commands is a great solution, but lacking in this particular area. :)

@BD103 BD103 added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 30, 2024
@james7132
Copy link
Member

james7132 commented Mar 30, 2024

There are plans on removing !Send resources from the World. See #6552 and @maniwani's work on #9202 and #9122. I'm not sure how useful this would be long term if that's the direction we want to move in.

@BD103
Copy link
Member Author

BD103 commented Mar 30, 2024

There are plans on removing !Send resources from the World. See #6552 and @maniwani's work on #9202 and #9122. I'm not sure how useful this would be long term if that's the direction we want to move in.

It looks like the linked PRs have been open for quite a while. It may be worth implementing non-send commands until those PRs are merged. @maniwani, what are your thoughts? Do you think the non-send changes suggested in #6552 will be implemented for 0.14?

@BD103 BD103 added the S-Blocked This cannot move forward until something else changes label Apr 10, 2024
@BD103
Copy link
Member Author

BD103 commented Apr 10, 2024

Marking this as blocked until #9122 is merged. While technically we can add it now, as I tried in #12819, I think it is best to wait until non-sends are removed from World.

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-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants