-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
can spawn empty bundle from world #6424
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
Conversation
Hmm yeah. I don't love the runtime branching, but maybe it gets optimized away. Does this regress our benchmarks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a regression test for this.
I think the only real alternative to branching somewhere in this impl is removing the Bundle impl for () and adding the constraint that Bundle can't be empty. Note that this PR isn't a full fix. All empty bundles are broken: // This fails in the same way
#[derive(Bundle)]
struct EmptyBundle {}
let mut world = World::new();
world.spawn(EmptyBundle {}); |
A full (still semi-weird) fix would be to check But maybe we don't need to support empty bundles? Do they even have a purpose anymore? Only use case I can think of is that we don't have a batch spawning api for empty entities: Also: idk if we need to hack around this for 0.9. This case fails with an unhelpful error message, but it still fails and it doesn't really have a use case. Maybe better to just hold off for a full solve. |
Notably this also panics atm. |
Yeah I think I'm fine with removing empty bundles completely. I don't like any of these fixes. |
The core of the issue is that spawning empty bundles is framed as |
But yeah we should discuss removing empty bundles. |
Came up with a reasonable long term fix: #6425 |
# Objective Alternative to #6424 Fixes #6226 Fixes spawning empty bundles ## Solution Add `BundleComponentStatus` trait and implement it for `AddBundle` and a new `SpawnBundleStatus` type (which always returns an Added status). `write_components` is now generic on `BundleComponentStatus` instead of taking `AddBundle` directly. This means BundleSpawner can now avoid needing AddBundle from the Empty archetype, which means BundleSpawner no longer needs a reference to the original archetype. In theory this cuts down on the work done in `write_components` when spawning, but I'm seeing no change in the spawn benchmarks.
# Objective Alternative to #6424 Fixes #6226 Fixes spawning empty bundles ## Solution Add `BundleComponentStatus` trait and implement it for `AddBundle` and a new `SpawnBundleStatus` type (which always returns an Added status). `write_components` is now generic on `BundleComponentStatus` instead of taking `AddBundle` directly. This means BundleSpawner can now avoid needing AddBundle from the Empty archetype, which means BundleSpawner no longer needs a reference to the original archetype. In theory this cuts down on the work done in `write_components` when spawning, but I'm seeing no change in the spawn benchmarks.
# Objective Alternative to bevyengine#6424 Fixes bevyengine#6226 Fixes spawning empty bundles ## Solution Add `BundleComponentStatus` trait and implement it for `AddBundle` and a new `SpawnBundleStatus` type (which always returns an Added status). `write_components` is now generic on `BundleComponentStatus` instead of taking `AddBundle` directly. This means BundleSpawner can now avoid needing AddBundle from the Empty archetype, which means BundleSpawner no longer needs a reference to the original archetype. In theory this cuts down on the work done in `write_components` when spawning, but I'm seeing no change in the spawn benchmarks.
Objective
world.spawn(())
World.spawn(())
panics with index out of bounds #6226Solution
()
, useworld.spawn_empty()
instead