Skip to content

[Merged by Bors] - Spawn specific entities: spawn or insert operations, refactor spawn internals, world clearing#2673

Closed
cart wants to merge 8 commits intobevyengine:mainfrom
cart:insert-or-spawn
Closed

[Merged by Bors] - Spawn specific entities: spawn or insert operations, refactor spawn internals, world clearing#2673
cart wants to merge 8 commits intobevyengine:mainfrom
cart:insert-or-spawn

Conversation

@cart
Copy link
Copy Markdown
Member

@cart cart commented Aug 17, 2021

This upstreams the code changes used by the new renderer to enable cross-app Entity reuse:

  • Spawning at specific entities
  • get_or_spawn: spawns an entity if it doesn't already exist and returns an EntityMut
  • insert_or_spawn_batch: the batched equivalent to world.get_or_spawn(entity).insert_bundle(bundle)
  • Clearing entities and storages
  • Allocating Entities with "invalid" archetypes. These entities cannot be queried / are treated as "non existent". They serve as "reserved" entities that won't show up when calling spawn(). They must be "specifically spawned at" using apis like get_or_spawn(entity).

In combination, these changes enable the "render world" to clear entities / storages each frame and reserve all "app world entities". These can then be spawned during the "render extract step".

This refactors "spawn" and "insert" code in a way that I think is a massive improvement to legibility and re-usability. It also yields marginal performance wins by reducing some duplicate lookups (less than a percentage point improvement on insertion benchmarks). There is also some potential for future unsafe reduction (by making BatchSpawner and BatchInserter generic). But for now I want to cut down generic usage to a minimum to encourage smaller binaries and faster compiles.

This is currently a draft because it needs more tests (although this code has already had some real-world testing on my custom-shaders branch).

I also fixed the benchmarks (which currently don't compile!) / added new ones to illustrate batching wins.

After these changes, Bevy ECS is basically ready to accommodate the new renderer. I think the biggest missing piece at this point is "sub apps".

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 17, 2021
@cart cart marked this pull request as draft August 17, 2021 23:12
@cart cart added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Aug 17, 2021
@cart cart added this to the Bevy 0.6 milestone Aug 17, 2021
@alice-i-cecile alice-i-cecile added the C-Performance A change motivated by improving speed, memory usage or compile times label Aug 18, 2021
@cart
Copy link
Copy Markdown
Member Author

cart commented Aug 18, 2021

Also I think we should generally discourage patterns like "spawning entities with a specific user-defined value":

  1. The entity metadata array is densely packed, so spawning at large indices will take up a lot of memory
  2. It goes against the general pattern of treating Entities as "opaque unique runtime handles", which the system generally encourages.

At the very least, the relevant methods should have docs discussing this / what use cases are acceptable. We might even want to remove the Entity::new() constructor entirely to prevent misuse (which has been discussed before).

@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented Aug 18, 2021

At the very least, the relevant methods should have docs discussing this / what use cases are acceptable. We might even want to remove the Entity::new() constructor entirely to prevent misuse (which has been discussed before).

I'm pretty heavily in favor of removing Entity::new(id): it's very misleading. I've run into several cases of users attempting to interface with other databases / coordinate data between two bevy_ecs worlds and thinking that operating on specific synchronized Entity identifiers was the way to go.

Does that make sense in this PR? I would also like to see a niche-optimized Option<Entity> as part of that general direction of changes (as reducing manual control means you won't accidentally assign to it), so it's probably worth splitting out.

@cart
Copy link
Copy Markdown
Member Author

cart commented Aug 18, 2021

I'm pretty heavily in favor of removing Entity::new(id): it's very misleading. I've run into several cases of users attempting to interface with other databases / coordinate data between two bevy_ecs worlds and thinking that operating on specific synchronized Entity identifiers was the way to go

Yeah thats a pretty small / scoped change that makes the changes in this pr more palatable. I'd probably just make it pub(crate) so we can test internally instead of removing it. The biggest hiccup is sorting out how to handle the FromWorld impls for Parent / PreviousParent (see the comments on that code for rationale).

#[derive(Default)]
struct Vec3([f32; 3]);

fn insert_commands(criterion: &mut Criterion) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't love this as a name: it reads as "the time it takes to insert Commands to the queue", not "how long does it take to insert components via commands". Perhaps batch_component_insertion?

Copy link
Copy Markdown
Member Author

@cart cart Aug 18, 2021

Choose a reason for hiding this comment

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

batch_component_insertion only applies to one of the two benchmarks in the group. It is intended to be a comparison of the various insertion approaches (non batched and batched).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm open to alternative names though (provided they describe that situation).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO this should be component_insertion then. Eventually, I'd like to have non-Commands component insertion, which would also be very nice to include here. The fact that it uses commands to do insertion is mostly incidental IMO.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Its worth calling out that this file is called "commands", which scopes it to "command benchmarks" imo. Happy to reorganize, but currently these benchmarks are all organized under a "commands" bucket.

@cart cart marked this pull request as ready for review August 18, 2021 02:10
@cart
Copy link
Copy Markdown
Member Author

cart commented Aug 19, 2021

Alrighty I think this is good to go!

@cart
Copy link
Copy Markdown
Member Author

cart commented Aug 19, 2021

(after another round of reviews / final signoff)

@cart
Copy link
Copy Markdown
Member Author

cart commented Aug 25, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 25, 2021
…nternals, world clearing (#2673)

This upstreams the code changes used by the new renderer to enable cross-app Entity reuse:

* Spawning at specific entities
* get_or_spawn: spawns an entity if it doesn't already exist and returns an EntityMut
* insert_or_spawn_batch: the batched equivalent to `world.get_or_spawn(entity).insert_bundle(bundle)`
* Clearing entities and storages
* Allocating Entities with "invalid" archetypes. These entities cannot be queried / are treated as "non existent". They serve as "reserved" entities that won't show up when calling `spawn()`. They must be "specifically spawned at" using apis like `get_or_spawn(entity)`.

In combination, these changes enable the "render world" to clear entities / storages each frame and reserve all "app world entities". These can then be spawned during the "render extract step".

This refactors "spawn" and "insert" code in a way that I think is a massive improvement to legibility and re-usability. It also yields marginal performance wins by reducing some duplicate lookups (less than a percentage point improvement on insertion benchmarks). There is also some potential for future unsafe reduction (by making BatchSpawner and BatchInserter generic). But for now I want to cut down generic usage to a minimum to encourage smaller binaries and faster compiles.

This is currently a draft because it needs more tests (although this code has already had some real-world testing on my custom-shaders branch). 

I also fixed the benchmarks (which currently don't compile!) / added new ones to illustrate batching wins.

After these changes, Bevy ECS is basically ready to accommodate the new renderer. I think the biggest missing piece at this point is "sub apps".
@bors bors bot changed the title Spawn specific entities: spawn or insert operations, refactor spawn internals, world clearing [Merged by Bors] - Spawn specific entities: spawn or insert operations, refactor spawn internals, world clearing Aug 25, 2021
@bors bors bot closed this Aug 25, 2021
bors bot pushed a commit that referenced this pull request Sep 23, 2021
This changes how render logic is composed to make it much more modular. Previously, all extraction logic was centralized for a given "type" of rendered thing. For example, we extracted meshes into a vector of ExtractedMesh, which contained the mesh and material asset handles, the transform, etc. We looked up bindings for "drawn things" using their index in the `Vec<ExtractedMesh>`. This worked fine for built in rendering, but made it hard to reuse logic for "custom" rendering. It also prevented us from reusing things like "extracted transforms" across contexts.

To make rendering more modular, I made a number of changes:

* Entities now drive rendering:
  * We extract "render components" from "app components" and store them _on_ entities. No more centralized uber lists! We now have true "ECS-driven rendering"
  * To make this perform well, I implemented #2673 in upstream Bevy for fast batch insertions into specific entities. This was merged into the `pipelined-rendering` branch here: #2815
* Reworked the `Draw` abstraction:
  * Generic `PhaseItems`: each draw phase can define its own type of "rendered thing", which can define its own "sort key"
  * Ported the 2d, 3d, and shadow phases to the new PhaseItem impl (currently Transparent2d, Transparent3d, and Shadow PhaseItems)
  * `Draw` trait and and `DrawFunctions` are now generic on PhaseItem
  * Modular / Ergonomic `DrawFunctions` via `RenderCommands`
    * RenderCommand is a trait that runs an ECS query and produces one or more RenderPass calls. Types implementing this trait can be composed to create a final DrawFunction. For example the DrawPbr DrawFunction is created from the following DrawCommand tuple. Const generics are used to set specific bind group locations:
        ```rust
         pub type DrawPbr = (
            SetPbrPipeline,
            SetMeshViewBindGroup<0>,
            SetStandardMaterialBindGroup<1>,
            SetTransformBindGroup<2>,
            DrawMesh,
        );
        ```
    * The new `custom_shader_pipelined` example illustrates how the commands above can be reused to create a custom draw function:
       ```rust
       type DrawCustom = (
           SetCustomMaterialPipeline,
           SetMeshViewBindGroup<0>,
           SetTransformBindGroup<2>,
           DrawMesh,
       );
       ``` 
* ExtractComponentPlugin and UniformComponentPlugin:
  * Simple, standardized ways to easily extract individual components and write them to GPU buffers
* Ported PBR and Sprite rendering to the new primitives above.
* Removed staging buffer from UniformVec in favor of direct Queue usage
  * Makes UniformVec much easier to use and more ergonomic. Completely removes the need for custom render graph nodes in these contexts (see the PbrNode and view Node removals and the much simpler call patterns in the relevant Prepare systems).
* Added a many_cubes_pipelined example to benchmark baseline 3d rendering performance and ensure there were no major regressions during this port. Avoiding regressions was challenging given that the old approach of extracting into centralized vectors is basically the "optimal" approach. However thanks to a various ECS optimizations and render logic rephrasing, we pretty much break even on this benchmark!
* Lifetimeless SystemParams: this will be a bit divisive, but as we continue to embrace "trait driven systems" (ex: ExtractComponentPlugin, UniformComponentPlugin, DrawCommand), the ergonomics of `(Query<'static, 'static, (&'static A, &'static B, &'static)>, Res<'static, C>)` were getting very hard to bear. As a compromise, I added "static type aliases" for the relevant SystemParams. The previous example can now be expressed like this: `(SQuery<(Read<A>, Read<B>)>, SRes<C>)`. If anyone has better ideas / conflicting opinions, please let me know!
* RunSystem trait: a way to define Systems via a trait with a SystemParam associated type. This is used to implement the various plugins mentioned above. I also added SystemParamItem and QueryItem type aliases to make "trait stye" ecs interactions nicer on the eyes (and fingers).
* RenderAsset retrying: ensures that render assets are only created when they are "ready" and allows us to create bind groups directly inside render assets (which significantly simplified the StandardMaterial code). I think ultimately we should swap this out on "asset dependency" events to wait for dependencies to load, but this will require significant asset system changes.
* Updated some built in shaders to account for missing MeshUniform fields
github-merge-queue bot pushed a commit that referenced this pull request Mar 23, 2026
## Objective

When moving an entity from one table to another, we have 3 methods
available:
- `move_to_superset_unchecked`, for moving during insertion.
- `move_to_and_drop_missing_unchecked`, for moving during normal
removal.
- `move_to_and_forget_missing_unchecked`, for moving during `take`
removal.

However, when looking at flecs (to steal ideas, naturally), I noticed it
just has one function for moving during insertion and removal:
[`flecs_table_move`](https://github.com/SanderMertens/flecs/blob/689adf8f6c1d069aa51fa059b8a2cbb4fb761f1d/src/storage/table.c#L1961).
It also handles things a bit differently internally (details on that
below).

## Solution

There are essentially 3 somewhat independent changes here:
- Combining the 3 methods into 1 called `move_row`.
- Moving the `move` method to `Tables` and taking `TableId`s (rather
than being `Table::move_to(&mut self, other: &mut Table)`).
- Changing the internals of the `move` method to be more like flecs.

### Combining the methods

The 3 methods were already very similar; `move_to_superset` just didn't
check if components were missing. They could be combined without any of
the other changes, but it's possible that insertion performance would
suffer.

### `TableId` instead of `&mut Table`

I believe the idea behind using references instead of IDs was to be able
to cache the pointers and avoid fetching the tables more than once.
However, the only callers of the `move_to` methods, `BundleInserter` and
`BundleRemover`, only used the pointers for the `move_to` methods and
didn't need them for anything else, so there isn't much point in caching
them. And being able to just use IDs makes the code nicer, since we
don't have to dance around the borrow checker.

### Internals

`flecs_table_move` requires that tables' columns are sorted (by
component ID, low-to-high). It iterates the source and destination
tables at the same time (i.e. their component IDs and corresponding
columns), but pauses one iterator when the other "falls behind" (has a
lower component ID). If the source table falls behind, the source
table's current component ID was removed, and if the destination table
falls behind, the destination table's current component ID was added.

Bevy's `move_to` methods don't rely on tables' columns being in any
certain order; they just iterate the source table and fetch each
matching column from the destination table individually.

As it turns out, Bevy's tables *are* sorted in practice, we just never
guaranteed or relied on it. A table's list of component IDs are sorted
to use as the key in a `HashMap`, and the same list is used to build the
table. So we can just make it official by `assert`ing that a table's
components are sorted when the table is finalized
(`TableBuilder::build`).

Evidently, iterating the destination table is faster than fetching from
it repeatedly:

<img width="644" height="848" alt="Screenshot_20260224_135012"
src="https://github.com/user-attachments/assets/118cb78a-3267-411e-aa8e-3d1d7ea4a274"
/>

### Misc

There's a `PERF` comment that has survived since 2021 (#2673) and I'm
not really sure what it means:

```
// PERF: store "non bundle" components in edge, then just move those to avoid
// redundant copies
```

I didn't want to remove it without saying anything, but I think it's
lived long enough.

---------

Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: eugineerd <[email protected]>
Co-authored-by: Eagster <[email protected]>
splo pushed a commit to splo/bevy that referenced this pull request Mar 31, 2026
## Objective

When moving an entity from one table to another, we have 3 methods
available:
- `move_to_superset_unchecked`, for moving during insertion.
- `move_to_and_drop_missing_unchecked`, for moving during normal
removal.
- `move_to_and_forget_missing_unchecked`, for moving during `take`
removal.

However, when looking at flecs (to steal ideas, naturally), I noticed it
just has one function for moving during insertion and removal:
[`flecs_table_move`](https://github.com/SanderMertens/flecs/blob/689adf8f6c1d069aa51fa059b8a2cbb4fb761f1d/src/storage/table.c#L1961).
It also handles things a bit differently internally (details on that
below).

## Solution

There are essentially 3 somewhat independent changes here:
- Combining the 3 methods into 1 called `move_row`.
- Moving the `move` method to `Tables` and taking `TableId`s (rather
than being `Table::move_to(&mut self, other: &mut Table)`).
- Changing the internals of the `move` method to be more like flecs.

### Combining the methods

The 3 methods were already very similar; `move_to_superset` just didn't
check if components were missing. They could be combined without any of
the other changes, but it's possible that insertion performance would
suffer.

### `TableId` instead of `&mut Table`

I believe the idea behind using references instead of IDs was to be able
to cache the pointers and avoid fetching the tables more than once.
However, the only callers of the `move_to` methods, `BundleInserter` and
`BundleRemover`, only used the pointers for the `move_to` methods and
didn't need them for anything else, so there isn't much point in caching
them. And being able to just use IDs makes the code nicer, since we
don't have to dance around the borrow checker.

### Internals

`flecs_table_move` requires that tables' columns are sorted (by
component ID, low-to-high). It iterates the source and destination
tables at the same time (i.e. their component IDs and corresponding
columns), but pauses one iterator when the other "falls behind" (has a
lower component ID). If the source table falls behind, the source
table's current component ID was removed, and if the destination table
falls behind, the destination table's current component ID was added.

Bevy's `move_to` methods don't rely on tables' columns being in any
certain order; they just iterate the source table and fetch each
matching column from the destination table individually.

As it turns out, Bevy's tables *are* sorted in practice, we just never
guaranteed or relied on it. A table's list of component IDs are sorted
to use as the key in a `HashMap`, and the same list is used to build the
table. So we can just make it official by `assert`ing that a table's
components are sorted when the table is finalized
(`TableBuilder::build`).

Evidently, iterating the destination table is faster than fetching from
it repeatedly:

<img width="644" height="848" alt="Screenshot_20260224_135012"
src="https://github.com/user-attachments/assets/118cb78a-3267-411e-aa8e-3d1d7ea4a274"
/>

### Misc

There's a `PERF` comment that has survived since 2021 (bevyengine#2673) and I'm
not really sure what it means:

```
// PERF: store "non bundle" components in edge, then just move those to avoid
// redundant copies
```

I didn't want to remove it without saying anything, but I think it's
lived long enough.

---------

Co-authored-by: Chris Russell <[email protected]>
Co-authored-by: eugineerd <[email protected]>
Co-authored-by: Eagster <[email protected]>
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 C-Performance A change motivated by improving speed, memory usage or compile times

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants