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

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

Closed
wants to merge 8 commits into from

Conversation

cart
Copy link
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
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
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
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
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
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
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
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
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
Member Author

cart commented Aug 19, 2021

Alrighty I think this is good to go!

@cart
Copy link
Member Author

cart commented Aug 19, 2021

(after another round of reviews / final signoff)

@cart
Copy link
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
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