Skip to content

Added boxable commands#4068

Closed
jamescarterbell wants to merge 13 commits intobevyengine:mainfrom
jamescarterbell:feature/commandboxed
Closed

Added boxable commands#4068
jamescarterbell wants to merge 13 commits intobevyengine:mainfrom
jamescarterbell:feature/commandboxed

Conversation

@jamescarterbell
Copy link
Contributor

Objective

Create a simple method of adding boxed commands to Commands. The cleanest method of adding Box currently isn't possible (adding Box directly), but we can do something close.

The end goal is to allow a performant method of adding commands to the CommandQueue across threads (mainly for sharing some form of Commands in a par_for_each loop).

Solution

By essentially creating and storing the CommandMetas write function alongside the Box in a specialized type, we can copy the data from the box into the CommandQueue, and move it's write function into the new CommandMeta object.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Feb 28, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 28, 2022
@IceSentry IceSentry removed the S-Needs-Triage This issue needs to be labelled label Mar 1, 2022
Copy link
Contributor

@mcobzarenco mcobzarenco left a comment

Choose a reason for hiding this comment

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

Just my thoughts - High level, I would really love to see an explanation in the docs as to why you'd want to box your commands, also honest question as I didn't know why / when this is useful

@jamescarterbell
Copy link
Contributor Author

Came back to look at this, fixed the fmt, think this could open up multithreaded command submissions.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Approved, but I want to see what the other folks working in this space have to think about it.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 16, 2022

@jamescarterbell can you resolve the comments that are addressed to make this easier for the reviewers?

Also clippy is failing :)

jamescarterbell and others added 3 commits May 16, 2022 19:33
Co-authored-by: Marius Cobzarenco <marius.cobzarenco@gmail.com>
Co-authored-by: Marius Cobzarenco <marius.cobzarenco@gmail.com>
@alice-i-cecile alice-i-cecile requested a review from bjorn3 May 17, 2022 04:11
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

I suspect for the parallel use case that #4749 probably avoids contention better than using a channel. (I haven't done any benching, so I wouldn't trust my guess too much.) Even if that is true, I expect that there are other use cases for boxable commands. If the linked PR gets merged we should maybe come up with another example for boxable commands in the docs.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 17, 2022
@alice-i-cecile
Copy link
Member

I'm happy to merge this once the typos are fixed then. Thanks for the informed opinion @hymm.

Co-authored-by: Mike <mike.hsu@gmail.com>
@jamescarterbell
Copy link
Contributor Author

I suspect for the parallel use case that #4749 probably avoids contention better than using a channel. (I haven't done any benching, so I wouldn't trust my guess too much.) Even if that is true, I expect that there are other use cases for boxable commands. If the linked PR gets merged we should maybe come up with another example for boxable commands in the docs.

Yeah I made this before knowing about that PR... Maybe some sort of example with commands in events? Hmm...

@alice-i-cecile
Copy link
Member

Yeah 🤔 I think I'd like to try and find another example for now to make this clearer; I want to get this + #4749 in for the next release and having a clear differentiation would be good.

@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone May 17, 2022
@TheRawMeatball
Copy link
Member

Personally I'm not a very big fan of this PR, considering that a Box<dyn FnOnce(&mut World)> would do pretty much everything this does just as well, and because I personally think creating commands without directly adding it to Commands is a bit of an anti-pattern with not much use.

@alice-i-cecile
Copy link
Member

@jamescarterbell, my personal feeling is that we should close this out until we have a compelling use case.

@jamescarterbell
Copy link
Contributor Author

Go for it

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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants