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

Bundles with optional components #2157

Open
botiapa opened this issue May 13, 2021 · 20 comments
Open

Bundles with optional components #2157

botiapa opened this issue May 13, 2021 · 20 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Milestone

Comments

@botiapa
Copy link

botiapa commented May 13, 2021

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

Currently if a bundle has components which are wrapped in an Option<>, they are treated literally. However in my opinion it would be a good addition if we had the option to mark optional components, and they would be treated as such.

What solution would you like?

Unfortunately I don't understand the inner workings of bevy enough to propose an exact solution, but after having a small conversation on discord, it has come to my attention, that there had been a similar feature like this in the old ECS called DynamicBundle. An example might look like this:

#[derive(Bundle)]
struct BundleA {
    component_a: f32,
    #[optional]
    component_b: Option<i32>
}

// In a system
let bundle = BundleA { component_a: 0f32, component_b: None };
commands.spawn_bundle(bundle) // --> Only component_a is added to the entity;

What alternative(s) have you considered?

Manually checking every optional component and adding it manually.

Additional context

I hadn't used DynamicBundle when it was still in, so I don't know if what I'm proposing makes sense.

@botiapa botiapa added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels May 13, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels May 13, 2021
@JonahPlusPlus
Copy link
Contributor

Hmm, maybe a builder pattern would be better for this? If you have a big bundle that has a lot of optionals, then it would mean that you would have to set a lot of then to None, when perhaps you only need a few. With a builder pattern, you could just add the ones you need, then spawn all of them together. However, I only see it being useful for when you don't want to spawn something right away. I don't know what scenario it would be used for, maybe you have something in mind?

Anyways, you can already spawn custom bundles by using tuples. The Bevy Docs cover it. You can use it like commands.spawn_bundle((Component1, Component2));. But maybe I am misunderstanding something?

@botiapa
Copy link
Author

botiapa commented May 14, 2021

Okay so imagine the following scenario:

You want to synchronize an entity over the network, so you create a query like this on the server side:
Query<(&NetID, Option<&ComponentA>, Option<&ComponentB>, Option<&ComponentC>)> you could easily convert this to a custom bundle which then you could serialize and send through the network, and adding it on the client side could be as easy as: commands.spawn_bundle(deserialized_bundle);

I hope that clears it up.

@Nilirad
Copy link
Contributor

Nilirad commented May 14, 2021

If I understand correctly, when you put in a query an Option<T>, the ECS will return the components of entities that may or may not include a component T.

If that's the case, I wonder if a Component that is of type Option<T> is actually queryable. If not, there should be no need to put an #[optional] attribute, since an Option cannot be returned from a query.

I'm 99% sure that I'm missing something though... 🤔

@botiapa
Copy link
Author

botiapa commented May 14, 2021

I'm pretty sure that it's queryable, you just have to switch where you put the &: Query<&Option<ComponentA>> instead of Query<Option<&ComponentA>>

I don't know if anyones uses it like this, but it is possible AFAIK.

@mockersf
Copy link
Member

mockersf commented May 18, 2021

Thinking more about this, I actually think we shouldn't insert optional components from a bundle:

  • if the component is None, do not insert it
  • if the component is Some(v), insert v

The Option from Rust translate nicely into a component present or not on an entity in Bevy

Downside is that a bundle may not have the same components based on its value, but seems OK to me

@StarArawn
Copy link
Contributor

I ran into this when wanting to allow users to pass in a custom bundle for tiles. I'm not sure how I can let them have tags in bundles currently. Example:

struct TileBundle {
  tile: Tile,
  visible: VisibleTile, // < If this is Option<VisibleTile> that doesn't make sense for a tag, yet there is no way for a user to not include the tag in the bundle.
}

This type of code is useful for deferred spawning of entities. Where you want to build out all of your entity data first and then mass spawn all of them.

@cart
Copy link
Member

cart commented May 18, 2021

This would likely require some major changes to how we handle Bundles internally and would likely result in more expensive archetype changes. Right now, we assume a Bundle's composition is static. This allows us to make bundle add/remove operations faster by pre-computing archetype transitions in the Archetype Graph and indexing them via the BundleId (which correlates 1:1 to a Bundle's TypeId).

If we want to support this pattern, we'll probably need to find a way to treat permutations of "optional bundle components" as different BundleIds.

There might be a way to reconcile this pattern with the "Bundle Builder" abstraction we've discussed in a couple of places:

#2061 (comment)
#792

// TileBundle includes all possible TileBundle components 
commands.spawn().insert_bundle(
    TileBundle::default()
        .without::<VisibleTile>()
)

@bjorn3
Copy link
Contributor

bjorn3 commented Jul 22, 2021

Nope, the get_type_info method doesn't take &self and as such must return the same type infos independent of the actual value of the bundle. I do see value in changing this though. (especially in the context of (currently not yet supported) dynamic components)

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Dec 12, 2021
@neocturne
Copy link
Contributor

I've been thinking about this feature a bit lately (even though I eventually realized that I don't need it for my current project after all):

  • With the new unified Component/Bundle methods, this could be made very ergonomic by having impl<B: Bundle> Bundle for Option<B>
  • I think Option can't implement Component with current Bevy, so the potential ambiguity discussed in earlier comments shouldn't be an issue
  • I haven't seen discussion of what EntityCommands::remove() should do with a Bundle with optional parts. I think it would make sense to remove all optional Components, so everything that could potentially have been added by the same Bundle is covered.

Has anything changed in the last 1.5 years that would making implementing this feature easier or more difficult?

@bjorn3
Copy link
Contributor

bjorn3 commented Dec 11, 2022

impl<B: Bundle> Bundle for Option<B> isn't possible as the list of components in a bundle can't depend on the bundle value. Bundle::component_ids doesn't take a self argument, nor can it as it is called when creating a new bundle from existing components too.

@neocturne
Copy link
Contributor

impl<B: Bundle> Bundle for Option<B> isn't possible as the list of components in a bundle can't depend on the bundle value. Bundle::component_ids doesn't take a self argument, nor can it as it is called when creating a new bundle from existing components too.

Hmm, shouldn't any solution that would allow us to have optional Components in Bundles at all also allow us to impl Bundle for Option? A Bundle with a single optional Component is isomorphic to an Option of a Component after all; extending this from single Components to whole Bundles should be possible as well.

I don't know much about the inner workings of Bundles and archetype transitions though, so I'm not sure what would need to change to allow implementing this at all, and what the tradeoffs are performance-wise.

@bjorn3
Copy link
Contributor

bjorn3 commented Dec 11, 2022

I guess.

@Imberflur
Copy link

I encountered a use case where this may be relevant:

I have an existing bundle with a from_asset(/* ... */) -> Self currently assumes that the asset has been already been loaded. I'm refactoring this allow for the asset loading to not necessarily be complete. This involves having a system that polls the pending assets and updates the state of one of the components from the bundle if the asset has been loaded. I also plan to include a MyAssetPending marker component to avoid unnecessary iteration and checking for cases where the loading was already completed.

However, since it is not possible to have optional components in a bundle I will need to refactor the code calling from_asset and spawning the bundle:

let bundle = MyBundle::from_asset(/*...*/);
commands.spawn(MyBundle);

My current plan to address this is something like this:

// Note, no longer actually a Bundle
impl MyBundle {
    fn spawn<'w, 's, 'a>(self, commands: &'a mut Commands<'w, 's>) -> EntityCommands<'w, 's, 'a> {
        if let Some(pending) = self.pending {
            commands.spawn((pending, self.other_components))
        } else {
            commands.spawn(self.other_components)
        }
    }
}
let bundle = MyBundle::from_asset(/*...*/);
bundle.spawn(&mut commands);

Although while writing this I did think of another option. MyAssetPending could always be inserted and then removed later in the polling system if the asset is actually already loaded.

@ActuallyHappening
Copy link
Contributor

Another rusty solution in some cases may be to rely on a components Default implementation as a no-op depending on your use case.

@marcotoniut
Copy link

Something to consider is what should happen if an attribute is set to None for a component that's already present in the entity during insertion.

The more natural scenario would be, in my opinion, to remove the current component, but I suppose the decision should arise from recurring patterns within the community.

@notverymoe
Copy link
Contributor

notverymoe commented Mar 28, 2024

The more natural scenario would be, in my opinion, to remove the current component

At first my gut instinct was the opposite. But I suppose if a bundle represents something like a "valid configuration" of components as it were, /not/ removing them could create an invalid or unintentional state. And having to explicitly check the options to remove them in such a case sounds annoying/error-prone, but conversely if you want the preserve behaviour it's going to be the greater pain (you can't really "undo" a removal, right?)

Perhaps a flag or alternate method so both can be supported? 😅

@ActuallyHappening
Copy link
Contributor

Maybe the #[non_exhaustive] attribute could be used here to flag the behaviour?
Structs/Bundles derived without the non_exhaustive attribute would overwrite None values (and maybe tracing::warn)
but structs/bundles derived with the non_exhaustive attribute would not overwrite, such that newer fields could be added without changing previous functionality

Just a suggestion

@s-puig
Copy link
Contributor

s-puig commented Sep 18, 2024

I don't know much about bevy_ecs but couldn't this be hacked around for the time being with observers/component hooks or would that break optional queries?

Something along the lines of adding OnAdd observers/hooks to all <T: Component> Option, add the component if Some and remove itself.

@PPakalns
Copy link
Contributor

PPakalns commented Sep 26, 2024

Okay so imagine the following scenario:

You want to synchronize an entity over the network, so you create a query like this on the server side: Query<(&NetID, Option<&ComponentA>, Option<&ComponentB>, Option<&ComponentC>)> you could easily convert this to a custom bundle which then you could serialize and send through the network, and adding it on the client side could be as easy as: commands.spawn_bundle(deserialized_bundle);

I hope that clears it up.

I have the same use case.

And I am seeing bad performance upon receiving a lot of components that need to be added to entity for the first time for hundreds of entities.

Pseudocode:

for entity_data in entities_data_from_server {
     let entity = get_entity_or_spawn(...., entity_data.entity_id);
     for component in entity_data.components {
         match component {
                ComponentA(comp) => { entity.insert(comp); }     
                ComponentB(comp) => { entity.insert(comp); }         
               ....
         }
    }
}

As much as I understand:
Each new component insert (directly to world or through command) results in entity being moved from one archetype to another. It would be good if all these operations could be grouped together in dynamic ComponentSet data structure that could be inserted into entity in one operation. In this case no unneeded intermediate archetypes would be initialized, no redundant data moves of entity between different archetypes would be needed.

It looks like something similar can be done with unsafe: https://docs.rs/bevy/latest/bevy/ecs/prelude/struct.EntityWorldMut.html#method.insert_by_ids
It would be nice if there would be a safe variant.

@alice-i-cecile
Copy link
Member

I've implemented a third-party version of this in i-cant-believe-its-not-bsn here. It uses component hooks and a special Maybe<B: Bundle> component. Super easy!

If you're interested in adding this to Bevy itself, feel free to copy-paste my code directly and we can review it.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 17, 2024
@BenjaminBrienen BenjaminBrienen added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Oct 30, 2024
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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

No branches or pull requests