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] - Add upstream bevy_ecs and prepare for custom-shaders merge #2815

Closed
wants to merge 7 commits into from

Conversation

cart
Copy link
Member

@cart cart commented Sep 12, 2021

This updates the pipelined-rendering branch to use the latest bevy_ecs from main. This accomplishes a couple of goals:

  1. prepares for upcoming custom-shaders branch changes, which were what drove many of the recent bevy_ecs changes on main
  2. prepares for the soon-to-happen merge of pipelined-rendering into main. By including bevy_ecs changes now, we make that merge simpler / easier to review.

I split this up into 3 commits:

  1. add upstream bevy_ecs: please don't bother reviewing this content. it has already received thorough review on main and is a literal copy/paste of the relevant folders (the old folders were deleted so the directories are literally exactly the same as main).
  2. support manual buffer application in stages: this is used to enable the Extract step. we've already reviewed this once on the pipelined-rendering branch, but its worth looking at one more time in the new context of (1).
  3. support manual archetype updates in QueryState: same situation as (2).

@bjorn3
Copy link
Contributor

bjorn3 commented Sep 12, 2021

Is there any reason you didn't merge the main branch into pipelined-rendering instead of this new commit with the main source copied?

@cart
Copy link
Member Author

cart commented Sep 12, 2021

Largely because I'm worried about accidentally including deleted / garbage content from the pipelined-rendering bevy_ecs variant. A full delete / copy ensures an exact version of the upstream folder, whereas git merges often try to be fancy.

@cart
Copy link
Member Author

cart commented Sep 12, 2021

A careful review could insulate us from that, but this approach is impossible to mess up.

@cart
Copy link
Member Author

cart commented Sep 12, 2021

I'm going through the merge process now just to see what that looks like / how safe it feels.

@cart
Copy link
Member Author

cart commented Sep 12, 2021

Found some weirdness with the "merge" approach:
image

Note that the fetch_next_aliased_unchecked<'a> lifetime (which is on main and is an important part of the safety of the impl) isn't a part of the merge diff at all. Fortunately selecting the "upstream/main" line in the merge would fail to compile, which would then force me to go back, look at the code, and realize that we need to add <'a> back to the function.

But an important part of that decision making process is "hidden" from view and I would only make the right decision there because I happened to remember the safety concerns of this line. Thats "scary" to me / makes me worry that I missed something.

This weirdness happens because the pipelined-rendering changes happen "after" the main changes in the timeline. From git's perspective, we already reviewed the pipelined-rendering changes relative to the other branch and "chose" to make the change, so why show it in the diff?

Therefore I think its better to "fix" the timeline by replacing all of the "wrong" pipelined-rendering ecs changes with the "upstream" changes. Then when we merge into main, we can trust the diffs again.

@alice-i-cecile alice-i-cecile added the A-ECS Entities, components, systems, and events label Sep 13, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Sep 13, 2021
@cart
Copy link
Member Author

cart commented Sep 14, 2021

bors r+

@cart
Copy link
Member Author

cart commented Sep 14, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 14, 2021
This updates the `pipelined-rendering` branch to use the latest `bevy_ecs` from `main`. This accomplishes a couple of goals:

1. prepares for upcoming `custom-shaders` branch changes, which were what drove many of the recent bevy_ecs changes on `main`
2. prepares for the soon-to-happen merge of `pipelined-rendering` into `main`. By including bevy_ecs changes now, we make that merge simpler / easier to review. 

I split this up into 3 commits:

1. **add upstream bevy_ecs**: please don't bother reviewing this content. it has already received thorough review on `main` and is a literal copy/paste of the relevant folders (the old folders were deleted so the directories are literally exactly the same as `main`).
2. **support manual buffer application in stages**: this is used to enable the Extract step. we've already reviewed this once on the `pipelined-rendering` branch, but its worth looking at one more time in the new context of (1).
3. **support manual archetype updates in QueryState**: same situation as (2).
@bors
Copy link
Contributor

bors bot commented Sep 14, 2021

@bors bors bot changed the title Add upstream bevy_ecs and prepare for custom-shaders merge [Merged by Bors] - Add upstream bevy_ecs and prepare for custom-shaders merge Sep 14, 2021
@bors bors bot closed this Sep 14, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants