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] - Make RenderStage::Extract run on the render world #4402

Closed
wants to merge 18 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Apr 3, 2022

Objective

  • Currently, the Extract RenderStage is executed on the main world, with the render world available as a resource.
  • However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole RenderWorld resource.
  • This meant that effectively only one extract which wrote to resources could run at a time.
  • We didn't previously make Extracting writing to the world a non-happy path, even though we want to discourage that.

Solution

  • Move the extract stage to run on the render world.
  • Add the main world as a MainWorld resource.
  • Add an Extract SystemParam as a convenience to access a (read only) SystemParam in the main world during Extract.

Future work

It should be possible to avoid needing to use get_or_spawn for the render commands, since now the Commands' Entities matches up with the world being executed on.
We need to determine how this interacts with #3519
It's theoretically possible to remove the need for the value method on Extract. However, that requires slightly changing the SystemParam interface, which would make it more complicated. That would probably mess up the SystemState api too.

Todo

I still need to add doc comments to Extract.


Changelog

Changed

  • The Extract RenderStage now runs on the render world (instead of the main world as before).
    You must use the Extract SystemParam to access the main world during the extract phase.
    Resources on the render world can now be accessed using ResMut during extract.

Removed

  • Commands::spawn_and_forget. Use Commands::get_or_spawn(e).insert_bundle(bundle) instead

Migration Guide

The Extract RenderStage now runs on the render world (instead of the main world as before).
You must use the Extract SystemParam to access the main world during the extract phase. Extract takes a single type parameter, which is any system parameter (such as Res, Query etc.). It will extract this from the main world, and returns the result of this extraction when value is called on it.

For example, if previously your extract system looked like:

fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
    for cloud in clouds.iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}

the new version would be:

fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
    for cloud in clouds.value().iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}

The diff is:

--- a/src/clouds.rs
+++ b/src/clouds.rs
@@ -1,5 +1,5 @@
-fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
-    for cloud in clouds.iter() {
+fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
+    for cloud in clouds.value().iter() {
         commands.get_or_spawn(cloud).insert(Cloud);
     }
 }

You can now also access resources from the render world using the normal system parameters during Extract:

fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) {
     *render_assets = source_assets.clone();
}

Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 3, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Apr 3, 2022
TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 5, 2022
Setup infrastructure for render app owns extract

Commit 'progress'

Finish API

Cleverly avoid needing the param to be 'static

This same trick can't work for `StaticSystemParam` fwiw
This is because `StaticSystemParam`'s item needs to exactly
reference the original param, whereas this just stores it

Migrate to the new pattern

Remove `spawn_and_forget`

It's unused, and was only ever needed for the weird extract logic

Fix clippy errors

Remove the trick because it's actually stupid

Add some docs

Fixup docs
TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 5, 2022
Setup infrastructure for render app owns extract

Commit 'progress'

Finish API

Cleverly avoid needing the param to be 'static

This same trick can't work for `StaticSystemParam` fwiw
This is because `StaticSystemParam`'s item needs to exactly
reference the original param, whereas this just stores it

Migrate to the new pattern

Remove `spawn_and_forget`

It's unused, and was only ever needed for the weird extract logic

Fix clippy errors

Remove the trick because it's actually stupid

Add some docs

Fixup docs
TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 19, 2022
Setup infrastructure for render app owns extract

Commit 'progress'

Finish API

Cleverly avoid needing the param to be 'static

This same trick can't work for `StaticSystemParam` fwiw
This is because `StaticSystemParam`'s item needs to exactly
reference the original param, whereas this just stores it

Migrate to the new pattern

Remove `spawn_and_forget`

It's unused, and was only ever needed for the weird extract logic

Fix clippy errors

Remove the trick because it's actually stupid

Add some docs

Fixup docs
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 22, 2022
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Generally looks good, just one thing regarding ergonomics.

crates/bevy_render/src/extract_param.rs Show resolved Hide resolved
#[derive(Default)]
pub struct RenderWorld(World);
pub struct MainWorld(World);
Copy link
Member

Choose a reason for hiding this comment

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

Probably could use the Deref derive macro we have.

@superdump
Copy link
Contributor

Did you do any benchmarks to compare?

@DJMcNab
Copy link
Member Author

DJMcNab commented May 6, 2022

As far as I remember, I was told to do this in discord, based on a visualisation? someone had created so I did it.

I haven't benchmarked this.

bors bot pushed a commit that referenced this pull request May 9, 2022
Required for #4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
bors bot pushed a commit that referenced this pull request May 9, 2022
Required for #4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
bors bot pushed a commit that referenced this pull request May 9, 2022
Required for #4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
bors bot pushed a commit that referenced this pull request May 9, 2022
Required for #4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
Required for bevyengine#4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
@alice-i-cecile alice-i-cecile added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label May 18, 2022
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
Required for bevyengine#4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
@james7132
Copy link
Member

@DJMcNab can you rebase this PR? I'd like to test the perf of it, but the merge conflicts are non-trivial.

@DJMcNab
Copy link
Member Author

DJMcNab commented Jun 1, 2022

I'm not very keen to - I don't know that it's likely to land too, and rebasing is annoying because there's no easy way to find all of the extract systems.

At least for the latter, we can warn if an extract system doesn't read MainWorld, which should make it significantly easier.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jun 1, 2022
Cleverly avoid needing the param to be 'static

This same trick can't work for `StaticSystemParam` fwiw
This is because `StaticSystemParam`'s item needs to exactly
reference the original param, whereas this just stores it
It's unused, and was only ever needed for the weird extract logic

Remove the trick because it's actually stupid
@cart
Copy link
Member

cart commented Jul 8, 2022

bors r+

bors bot pushed a commit that referenced this pull request Jul 8, 2022
# Objective

- Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource.
- However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource.
- This meant that effectively only one extract which wrote to resources could run at a time.
- We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that.

## Solution

- Move the extract stage to run on the render world.
- Add the main world as a `MainWorld` resource.
- Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`.

## Future work

It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on.
We need to determine how this interacts with #3519
It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too.

## Todo
I still need to add doc comments to `Extract`.

---

## Changelog

### Changed
- The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
   You must use the `Extract` `SystemParam` to access the main world during the extract phase.
   Resources on the render world can now be accessed using `ResMut` during extract.

### Removed
- `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead

## Migration Guide

The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it.

For example, if previously your extract system looked like:
```rust
fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
    for cloud in clouds.iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
the new version would be:
```rust
fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
    for cloud in clouds.value().iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
The diff is:
```diff
--- a/src/clouds.rs
+++ b/src/clouds.rs
@@ -1,5 +1,5 @@
-fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
-    for cloud in clouds.iter() {
+fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
+    for cloud in clouds.value().iter() {
         commands.get_or_spawn(cloud).insert(Cloud);
     }
 }
```
You can now also access resources from the render world using the normal system parameters during `Extract`:
```rust
fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) {
     *render_assets = source_assets.clone();
}
```
Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Make RenderStage::Extract run on the render world [Merged by Bors] - Make RenderStage::Extract run on the render world Jul 9, 2022
@bors bors bot closed this Jul 9, 2022
@IceSentry IceSentry added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 9, 2022
@DJMcNab DJMcNab deleted the render_owns_extract branch July 9, 2022 06:58
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 9, 2022
Following bevyengine#4402, it is now impossible to mutate the main world from the
render world, so we split the update to the projection into a different
system ran in the main world.
bors bot pushed a commit that referenced this pull request Jul 10, 2022
# Objective

- Extracting resources currently always uses commands, which requires *at least* one additional move of the extracted value, as well as dynamic dispatch.
- Addresses #4402 (comment)

## Solution

- Write the resource into a `ResMut<R>` directly.
- Fall-back to commands if the resource hasn't been added yet.
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 19, 2022
Following bevyengine#4402, it is now impossible to mutate the main world from the
render world, so we split the update to the projection into a different
system ran in the main world.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource.
- However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource.
- This meant that effectively only one extract which wrote to resources could run at a time.
- We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that.

## Solution

- Move the extract stage to run on the render world.
- Add the main world as a `MainWorld` resource.
- Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`.

## Future work

It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on.
We need to determine how this interacts with bevyengine#3519
It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too.

## Todo
I still need to add doc comments to `Extract`.

---

## Changelog

### Changed
- The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
   You must use the `Extract` `SystemParam` to access the main world during the extract phase.
   Resources on the render world can now be accessed using `ResMut` during extract.

### Removed
- `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead

## Migration Guide

The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it.

For example, if previously your extract system looked like:
```rust
fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
    for cloud in clouds.iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
the new version would be:
```rust
fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
    for cloud in clouds.value().iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
The diff is:
```diff
--- a/src/clouds.rs
+++ b/src/clouds.rs
@@ -1,5 +1,5 @@
-fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
-    for cloud in clouds.iter() {
+fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
+    for cloud in clouds.value().iter() {
         commands.get_or_spawn(cloud).insert(Cloud);
     }
 }
```
You can now also access resources from the render world using the normal system parameters during `Extract`:
```rust
fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) {
     *render_assets = source_assets.clone();
}
```
Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- Extracting resources currently always uses commands, which requires *at least* one additional move of the extracted value, as well as dynamic dispatch.
- Addresses bevyengine#4402 (comment)

## Solution

- Write the resource into a `ResMut<R>` directly.
- Fall-back to commands if the resource hasn't been added yet.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource.
- However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource.
- This meant that effectively only one extract which wrote to resources could run at a time.
- We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that.

## Solution

- Move the extract stage to run on the render world.
- Add the main world as a `MainWorld` resource.
- Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`.

## Future work

It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on.
We need to determine how this interacts with bevyengine#3519
It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too.

## Todo
I still need to add doc comments to `Extract`.

---

## Changelog

### Changed
- The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
   You must use the `Extract` `SystemParam` to access the main world during the extract phase.
   Resources on the render world can now be accessed using `ResMut` during extract.

### Removed
- `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead

## Migration Guide

The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it.

For example, if previously your extract system looked like:
```rust
fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
    for cloud in clouds.iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
the new version would be:
```rust
fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
    for cloud in clouds.value().iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
The diff is:
```diff
--- a/src/clouds.rs
+++ b/src/clouds.rs
@@ -1,5 +1,5 @@
-fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
-    for cloud in clouds.iter() {
+fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
+    for cloud in clouds.value().iter() {
         commands.get_or_spawn(cloud).insert(Cloud);
     }
 }
```
You can now also access resources from the render world using the normal system parameters during `Extract`:
```rust
fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) {
     *render_assets = source_assets.clone();
}
```
Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Extracting resources currently always uses commands, which requires *at least* one additional move of the extracted value, as well as dynamic dispatch.
- Addresses bevyengine#4402 (comment)

## Solution

- Write the resource into a `ResMut<R>` directly.
- Fall-back to commands if the resource hasn't been added yet.
bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective
Following #4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes #4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective
Following #4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes #4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective
Following #4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes #4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective
Following #4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes #4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective
Following #4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes #4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective
Following bevyengine#4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes bevyengine#4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Currently, the `Extract` `RenderStage` is executed on the main world, with the render world available as a resource.
- However, when needing access to resources in the render world (e.g. to mutate them), the only way to do so was to get exclusive access to the whole `RenderWorld` resource.
- This meant that effectively only one extract which wrote to resources could run at a time.
- We didn't previously make `Extract`ing writing to the world a non-happy path, even though we want to discourage that.

## Solution

- Move the extract stage to run on the render world.
- Add the main world as a `MainWorld` resource.
- Add an `Extract` `SystemParam` as a convenience to access a (read only) `SystemParam` in the main world during `Extract`.

## Future work

It should be possible to avoid needing to use `get_or_spawn` for the render commands, since now the `Commands`' `Entities` matches up with the world being executed on.
We need to determine how this interacts with bevyengine#3519
It's theoretically possible to remove the need for the `value` method on `Extract`. However, that requires slightly changing the `SystemParam` interface, which would make it more complicated. That would probably mess up the `SystemState` api too.

## Todo
I still need to add doc comments to `Extract`.

---

## Changelog

### Changed
- The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
   You must use the `Extract` `SystemParam` to access the main world during the extract phase.
   Resources on the render world can now be accessed using `ResMut` during extract.

### Removed
- `Commands::spawn_and_forget`. Use `Commands::get_or_spawn(e).insert_bundle(bundle)` instead

## Migration Guide

The `Extract` `RenderStage` now runs on the render world (instead of the main world as before).
You must use the `Extract` `SystemParam` to access the main world during the extract phase. `Extract` takes a single type parameter, which is any system parameter (such as `Res`, `Query` etc.). It will extract this from the main world, and returns the result of this extraction when `value` is called on it.

For example, if previously your extract system looked like:
```rust
fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
    for cloud in clouds.iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
the new version would be:
```rust
fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
    for cloud in clouds.value().iter() {
        commands.get_or_spawn(cloud).insert(Cloud);
    }
}
```
The diff is:
```diff
--- a/src/clouds.rs
+++ b/src/clouds.rs
@@ -1,5 +1,5 @@
-fn extract_clouds(mut commands: Commands, clouds: Query<Entity, With<Cloud>>) {
-    for cloud in clouds.iter() {
+fn extract_clouds(mut commands: Commands, mut clouds: Extract<Query<Entity, With<Cloud>>>) {
+    for cloud in clouds.value().iter() {
         commands.get_or_spawn(cloud).insert(Cloud);
     }
 }
```
You can now also access resources from the render world using the normal system parameters during `Extract`:
```rust
fn extract_assets(mut render_assets: ResMut<MyAssets>, source_assets: Extract<Res<MyAssets>>) {
     *render_assets = source_assets.clone();
}
```
Please note that all existing extract systems need to be updated to match this new style; even if they currently compile they will not run as expected. A warning will be emitted on a best-effort basis if this is not met.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Extracting resources currently always uses commands, which requires *at least* one additional move of the extracted value, as well as dynamic dispatch.
- Addresses bevyengine#4402 (comment)

## Solution

- Write the resource into a `ResMut<R>` directly.
- Fall-back to commands if the resource hasn't been added yet.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Following bevyengine#4402, extract systems run on the render world instead of the main world, and allow retained state operations on it's resources. We're currently extracting to `ExtractedJoints` and then copying it twice during Prepare. Once into `SkinnedMeshJoints` and again into the actual GPU buffer.

This makes bevyengine#4902 obsolete.

## Solution
Cut out the middle copy and directly extract joints into `SkinnedMeshJoints` and remove `ExtractedJoints` entirely.

This also removes the per-frame allocation that is being made to send `ExtractedJoints` into the render world.

## Performance
On my local machine, this halves the time for `prepare_skinned _meshes` on `many_foxes` (195.75us -> 93.93us on average).

![image](https://user-images.githubusercontent.com/3137680/205427455-ab91a8a3-a6b0-4f0a-bd48-e54482c563b2.png)

---

## Changelog
Added: `BufferVec::truncate`
Added: `BufferVec::extend`
Changed: `SkinnedMeshJoints::build` now takes a `&mut BufferVec` instead of a `&mut Vec` as a parameter.
Removed: `ExtractedJoints`.

## Migration Guide
`ExtractedJoints` has been removed. Read the bound bones from `SkinnedMeshJoints` instead.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Required for bevyengine#4402.

# Objective

- derived `SystemParam` implementations were never `ReadOnlySystemParamFetch`
- We want them to be, e.g. for `EventReader`

## Solution

- If possible, 'forward' the impl of `ReadOnlySystemParamFetch`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants