Skip to content

Conversation

SkiFire13
Copy link
Contributor

@SkiFire13 SkiFire13 commented Jun 4, 2025

Note: this is in the process of being split up, consider reviewing the individual PRs unless you're looking for the whole context

Objective

Solution

  • Introduce a StaticBundle (name bikesheddable) for usecases where the set of components of a bundle must be knowable without an instance of the bundle and switch functions that needed this aspect of Bundle;
  • Add &self parameters to Bundle's methods to allow them to depend on self's value;
  • (Optimization): add a way to distinguish different types of Bundles:
    • static bundles have all components statically known, like current bundles, and can be cached by their type id;
    • bounded bundles may have some optional components, but overall the set of components they can contain is bounded, allowing to distinguish different subsets by a cache key (in addition to the bundle type id);
    • dynamic bundles have no way to be cached other than looking at their ComponentIds
    • overall this is implemented through the new is_static, is_bounded and cache_key methods on Bundle.
  • Implement Bundle for Option<T: Bundle> as a bounded bundle (i.e. with is_static = false, is_bounded = true and a cache key depending on whether it is Some or None);
  • Make Bundle dyn-compatible and implement Bundle for Box<dyn Bundle>
    • this was done by marking non-dyn compatible methods as where Self: Sized and by introducing a couple of new dyn-compatible automatically-implemented supertraits, BundleDyn and BundleEffectDyn.
  • Add a #[bundle(dynamic)] attribute for the Bundle derive macro to opt-out of deriving StaticBundle and BundleFromComponents
    • BundleFromComponents could theoretically be implemented for bounded but non-static bundles, but it seems tricky and I decided to leave it out from this PR

Testing

  • each new implementation of Bundle comes with its own tests in crates/bevy_ecs/src/bundle.rs
  • benchmarks have been added to validate the performance advantages of the cache key mechanism, see this and this comments

Showcase

fn enemy_bundle(name: String, attack_power: u32, is_boss: bool) -> impl Bundle {
    (
        EnemyMarker,
        Name::new(name),
        AttackPower(attack_power),
        // You can now conditionally insert components or other bundles!
        if is_boss { Some(BossMarker) } else { None }
    )
}

// You can now convert reflected components and bundles into an actual `Bundle` you can insert
fn bundle_from_reflected_data(components: Vec<Box<dyn Reflect>>, registry: &TypeRegistry) -> impl Bundle {
    components
        .into_iter()
        .map(|data| {
            let Some(reflect_bundle) = registry.get_type_data::<ReflectBundle>(data.type_id()) else { todo!() };
            let Ok(bundle_box) = reflect_bundle.get_boxed(data) else { todo!() };
            bundle_box
        })
        .collect::<Vec<Box<dyn Bundle>>>()
}

SkiFire13 added 30 commits June 4, 2025 19:10
@NthTensor NthTensor removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged S-Needs-Help The author needs help finishing this PR. labels Jun 9, 2025
@SkiFire13
Copy link
Contributor Author

Upon seeing #19560 I wonder if StaticBundle should be a separate derive or not. Differently from BundleFromComponents, which is just used in take, StaticBundle is used in a lot of places (almost every API that uses Bundle but not for insert or spawn) so a double derive might be a lot more common. For reference, currently I have a #[bundle(dynamic)] annotation to opt-out of deriving StaticBundle and BundleFromComponents.

@alice-i-cecile
Copy link
Member

I think that making the common case pleasant is the way to go here. Most bundles will be static, so I like your current design.

github-merge-queue bot pushed a commit that referenced this pull request Jun 20, 2025
# Objective

- Splitted off from  #19491
- Make adding generated code to the `Bundle` derive macro easier
- Fix a bug when multiple fields are `#[bundle(ignore)]`

## Solution

- Instead of accumulating the code for each method in a different `Vec`,
accumulate only the names of non-ignored fields and their types, then
use `quote` to generate the code for each of them in the method body.
- To fix the bug, change the code populating the `BundleFieldKind` to
push only one of them per-field (previously each `#[bundle(ignore)]`
resulted in pushing twice, once for the correct
`BundleFieldKind::Ignore` and then again unconditionally for
`BundleFieldKind::Component`)

## Testing

- Added a regression test for the bug that was fixed
@SkiFire13 SkiFire13 marked this pull request as draft June 23, 2025 09:45
github-merge-queue bot pushed a commit that referenced this pull request Jun 30, 2025
# Objective

- Splitted off from #19491
- Add some benchmarks for spawning and inserting components. Right now
these are pretty short, but it's expected that they will be extended
when different kinds of dynamic bundles will be implemented.
Trashtalk217 pushed a commit to Trashtalk217/bevy that referenced this pull request Jul 10, 2025
# Objective

- Splitted off from  bevyengine#19491
- Make adding generated code to the `Bundle` derive macro easier
- Fix a bug when multiple fields are `#[bundle(ignore)]`

## Solution

- Instead of accumulating the code for each method in a different `Vec`,
accumulate only the names of non-ignored fields and their types, then
use `quote` to generate the code for each of them in the method body.
- To fix the bug, change the code populating the `BundleFieldKind` to
push only one of them per-field (previously each `#[bundle(ignore)]`
resulted in pushing twice, once for the correct
`BundleFieldKind::Ignore` and then again unconditionally for
`BundleFieldKind::Component`)

## Testing

- Added a regression test for the bug that was fixed
@alice-i-cecile alice-i-cecile modified the milestones: 0.17, 0.18 Jul 21, 2025
@alice-i-cecile alice-i-cecile added S-Blocked This cannot move forward until something else changes and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 21, 2025
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Blocked This cannot move forward until something else changes X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Respond (With Priority)
Status: Widget-ready
Development

Successfully merging this pull request may close these issues.

Better tools for working with dynamic collections of components Bundles with optional components
6 participants