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

Manipulate non-send resources with Commands #12819

Closed
wants to merge 8 commits into from

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Apr 1, 2024

Objective

Solution

  • Implements init_non_send_resource, insert_non_send_resource, and remove_non_send_resource methods for Commands.
    • Since Commands is accessible across threads, insert_non_send_resource's implementation takes a closure that creates the non-send resource on the main thread, instead of straight-up taking the value. This can be a bit confusing, so please check my documentation closely to make sure I explained it well!
  • I additionally moved remove_resource in 13e0f17, which is why the diff looks weird. I recommend looking at 314391c for the actual new content.

Changelog

  • Added methods for manipulating non-send resources in Commands.

Migration Guide

This PR is not actually a breaking change, but I feel a migration guide is useful. Comment if you think I should remove it. :)

It is now possible to insert and remove non-send resources using Commands. If you have any systems that take &mut World like this:

fn insert_non_send(world: &mut World) {
    world.insert_non_send_resource(MyNonSend::new());
}

You can replace it with the following, which may yield performance improvements:

fn insert_non_send(mut commands: Commands) {
    commands.insert_non_send_resource(|| {
        MyNonSend::new()
    });
}

This also applies to the methods init_non_send_resource and remove_non_send_resource. Note that Commands::insert_non_send_resource takes a closure, not MyNonSend directly. This closure's returned value will be inserted, after it is run on the main thread.

Be warned that this may change the order of inserting your resource! Commands works by delaying work until apply_deferred is called. If you depend on a resource being inserted / removed within a schedule, using Commands will break your code.

@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 Apr 1, 2024
@NthTensor
Copy link
Contributor

Tentatively marking this as controversial, as 9122 is supposed to be out of draft soon and seems to have broad support.

@SkiFire13 asks on discord:

12819 says it is incompatible with 9122 but is that really the case? Couldn't the commands in 12819 be implemented using the TLS after 9122 is merged?

@NthTensor NthTensor added the X-Controversial There is active debate or serious implications around merging this PR label Apr 1, 2024
This makes the declaration order consistent with the surrounding methods of `Commands::remove_resource`. This is an opinionated change that I would be fine with reverting.
I incorrectly assumed that bevy was added as a dev-dependency. :)
Also remove the `is_in_fact_not_being_sent` test.
@BD103
Copy link
Member Author

BD103 commented Apr 10, 2024

Closing this until #9122 is merged, where we can then reconsider it.

@BD103 BD103 closed this Apr 10, 2024
@BD103 BD103 deleted the non-send-commands branch April 10, 2024 13:20
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 X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upstream bevy_command_non_send
2 participants