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

Fix infinite asset preparation due to undrained AssetEvent events #11383

Conversation

brianreavis
Copy link
Contributor

@brianreavis brianreavis commented Jan 17, 2024

Objective

After #10520, I was experiencing seriously degraded performance that ended up being due to never-drained AssetEvent events causing havoc inside extract_render_asset::<A>. The same events being read over and over again meant the same assets were being prepared every frame for eternity. For what it's worth, I was noticing this on a static scene about every 3rd or so time running my project.

Why these events aren't sometimes drained between frames is beyond me and perhaps worthy of another investigation, but the approach in this PR effectively restores the original cached EventReader behavior (which fixes it).

Solution

I followed the CachedSystemState example to make sure that the EventReader state is cached between frames like it used to be when it was an argument of extract_render_asset::<A>.

@mockersf
Copy link
Member

this PR fixes #11240

@matiqo15 matiqo15 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 17, 2024
Copy link
Contributor

@torsteingrindvik torsteingrindvik left a comment

Choose a reason for hiding this comment

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

I checked the render_to_texture example on this PR and it is indeed fixed.

Also pointed my Bevy app which is broken on main towards this PR and that is fixed too 👍

@JMS55 JMS55 added this to the 0.13 milestone Jan 21, 2024
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

note: easier to review with whitespace diff hidden

@rparrett rparrett added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 25, 2024

impl<A: RenderAsset> FromWorld for CachedExtractRenderAssetSystemState<A> {
fn from_world(world: &mut bevy_ecs::world::World) -> Self {
Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: import World directly instead of writing out the full path

@Elabajaba
Copy link
Contributor

I was seeing significant memory usage increases (VRAM and CPU), as well as significantly longer load times when testing loading large scenes on main, and this fixes them.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 27, 2024
@alice-i-cecile
Copy link
Member

Merging now to help y'all diagnose other perf issues more easily.

Merged via the queue into bevyengine:main with commit c227fc9 Jan 27, 2024
27 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
Maximetinu pushed a commit to Sophya/bevy that referenced this pull request Feb 12, 2024
…vyengine#11383)

# Objective

After bevyengine#10520, I was experiencing seriously degraded performance that
ended up being due to never-drained `AssetEvent` events causing havoc
inside `extract_render_asset::<A>`. The same events being read over and
over again meant the same assets were being prepared every frame for
eternity. For what it's worth, I was noticing this on a static scene
about every 3rd or so time running my project.

* References bevyengine#10520
* Fixes bevyengine#11240

Why these events aren't sometimes drained between frames is beyond me
and perhaps worthy of another investigation, but the approach in this PR
effectively restores the original cached `EventReader` behavior (which
fixes it).

## Solution

I followed the [`CachedSystemState`
example](https://github.com/bevyengine/bevy/blob/3a666cab23eab5e83552ae074f47d8663221311e/crates/bevy_ecs/src/system/function_system.rs#L155)
to make sure that the `EventReader` state is cached between frames like
it used to be when it was an argument of `extract_render_asset::<A>`.
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-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Texture in render_to_texture example doesn't update
9 participants