bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Add insert_by_id and try_insert_by_id to EntityCommands

Open SOF3 opened this issue 1 year ago • 2 comments

Objective

  • Allow queuing insertion of dynamic components to an existing entity

Solution

  • Add insert_by_id<T: Send + 'static>(commands: &mut EntityCommands, component_id: ComponentId, value: T) and the try_insert_by_id counterpart

Testing

TODO

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Alternatives

This PR is not feature-complete for dynamic components. In particular, it

  • only supports one component
  • only supports adding components with a known, sized type

These were not implemented because doing so would require enhancing CommandQueue to support pushing unsized commands (or equivalently, pushing commands with a buffer of data). Even so, the cost would not be transparent compared to the implementation in this PR, which simply captures the ComponentId and value: T into the command closure and can be easily memcpy'ed to the stack during execution. For example, to efficiently pass &[ComponentId] from the caller to the world, we would need to:

  1. Update CommandQueue.bytes from Vec<MaybeUninit<u8>> to Vec<MaybeUninit<usize>> so that it has the same alignment as ComponentId (which probably needs to be made #[repr(transparent)] too)
  2. After pushing the Command metadata, push padding bytes until the vec len is a multiple of size_of::<usize>()
  3. Store components.len() in the data
  4. memcpy the user-provided &[ComponentId] to CommandQueue.bytes
  5. During execution, round up the data pointer behind the Command to skip padding, then cast the pointer and consume it as a &[ComponentId]

The effort here seems unnecessarily high, unless someone else has such a requirement. At least for the use case I am working with, I only need a single known type, and if we need multiple components, we could always enhance this function to accept a [ComponentId; N].

I recommend enhancing the Bundle API in the long term to achieve this goal more elegantly.

SOF3 avatar Jul 11 '24 15:07 SOF3

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

github-actions[bot] avatar Jul 11 '24 15:07 github-actions[bot]

Not sure what would be appropriate for testing this PR. Any thoughts on adding an example for dynamic systems that include inserting dynamic components?

SOF3 avatar Jul 12 '24 03:07 SOF3

Any thoughts on adding an example for dynamic systems that include inserting dynamic components?

I would be very happy to see this. Keep it simple, but try and demonstrate a whole lifecycle: spawning, querying...

alice-i-cecile avatar Jul 14 '24 15:07 alice-i-cecile

Regarding the comment on ComponentId world identity, any thoughts on using something like ghost-cell brand lifetime to identify the world at compile time? Checking the world at runtime might not be heavy, but it is just very unlikely to be useful considering that most applications shouldn't have more than one world at a time [citation needed].

SOF3 avatar Jul 16 '24 09:07 SOF3