-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Renderer Optimization Round 1 #958
Conversation
Looks like the |
Actually that seems unrelated. I'm also hitting the gltf issue on the master branch. |
I'm seeing a problem using this branch. I see a panic on this unwrap https://github.com/cart/bevy/blob/render-opt-round1/crates/bevy_wgpu/src/renderer/wgpu_render_resource_context.rs#L489 The issue seems to be that one part of the infrastructure has a BufferId that has been invalidated because the buffer it references has been resized here: https://github.com/cart/bevy/blob/render-opt-round1/crates/bevy_render/src/render_graph/nodes/render_resources_node.rs#L454 I think this call to write_uniform_buffers should be fixing the BufferId handle that's been removed but it's now behind a Changed filter that didn't used to be there and that is preventing it from getting called: https://github.com/cart/bevy/blob/render-opt-round1/crates/bevy_render/src/render_graph/nodes/render_resources_node.rs#L468 I'm not sure what the relationship is between the RenderResources component that controls the Changed filter and the arrays that are getting resized. But I assume that whatever change it is that causes the array to resize isn't getting caught by the change tracker. If I remove the Changed filter from the query then the panic goes away for me. |
I'm also seeing this panic, which I think is related to this branch as I've never noticed it before but I'm not sure what exactly I'm doing to trigger it. It appears intermittently.
|
I'm also seeing the same panic as above randomly
most of the time with Also, I'm having a missing sprite sometimes but always the same, will try to reduce to something minimal |
this will not display the sprite, even though it's logged: use bevy::prelude::*;
fn main() {
App::build()
.add_plugins(DefaultPlugins)
.add_startup_system(setup)
.add_system(display)
.run();
}
fn setup(commands: &mut Commands) {
commands.spawn(Camera2dBundle::default());
}
fn display(
commands: &mut Commands,
mut first: Local<bool>,
asset_server: Res<AssetServer>,
mut materials: ResMut<Assets<ColorMaterial>>,
) {
if !*first {
let texture_handle = asset_server.load("branding/icon.png");
info!(
"displaying sprite: {:?}",
commands
.spawn(SpriteBundle {
material: materials.add(texture_handle.into()),
..Default::default()
})
.current_entity()
);
*first = true;
}
} If I put |
this will crash with use bevy::prelude::*;
fn main() {
App::build()
.add_plugins(DefaultPlugins)
.add_startup_system(setup)
.add_system(display)
.run();
}
fn setup(commands: &mut Commands) {
commands
.spawn(UiCameraBundle::default())
.with(Timer::from_seconds(1.0, true));
}
fn display(
commands: &mut Commands,
mut state: Local<bool>,
time: Res<Time>,
mut timers: Query<&mut Timer>,
texts: Query<Entity, With<Text>>,
asset_server: Res<AssetServer>,
) {
for mut timer in timers.iter_mut() {
timer.tick(time.delta_seconds());
if timer.just_finished() {
if *state {
info!("removing all texts");
for entity in texts.iter() {
commands.despawn_recursive(entity);
}
} else {
info!("spawning texts");
commands.spawn(TextBundle {
style: Style {
align_self: AlignSelf::FlexEnd,
..Default::default()
},
text: Text {
value: "text".to_string(),
font: asset_server.load("fonts/FiraSans-Bold.ttf"),
style: TextStyle {
font_size: 60.0,
color: Color::WHITE,
..Default::default()
},
},
..Default::default()
});
}
*state = !*state;
}
}
} |
@mockersf That example makes sense based on when I'm seeing the panic. I think in my app it only happens when I switch between a state with UI components and one without or vice versa. But it doesn't happen every time the state changes, maybe 50% of the time which makes me think it might be a timing issue. |
A big thanks to everyone for the testing / investigation. I'm diving in now. |
@mockersf this was a result of us now caching NoMatch bind group lookup results (which prevented future changes to RenderResourceBindings from producing a match). I fixed this in the latest commit. Inserting new bindings now invalidates non-matching results. |
@alec-deason I can trigger it consistently in the |
@alec-deason dropping this commit resolves this issue, so its definitely scoped to that change set. Lol past @cart was right about it needing a quality check. @mockersf your text add/remove example is not fixed by reverting that commit, so it comes from somewhere else |
@mockersf the add/remove example panic comes from the "remove stale bind groups" commit. interesting |
@mockersf The add/remove example was broken because RenderResourceBindings isn't made aware when RenderResourceContext removes a "stale" bind group. Therefore even if the BindGroupStatus is "unchanged", we still need to try creating the bind group in case it doesn't exist. Performance-wise these are our options:
We're leaving some performance on the table with the "fix", but its still an improvement over the previous behavior. |
Alrighty I think I've fixed all of the problems! Let me know if everything works on your end |
e194eaa
to
ea5f41f
Compare
Also I just noticed that the button example is using large amounts of CPU across all my cores, even though it's not doing anything or even visible on the screen right now. |
Never mind. Both problems happen on the current master so they aren't part of this branch. |
Hmm the windowing issue is probably related to #947 |
And im planning on optimizing text rendering next, so hopefully that will resolve the button cpu usage issue |
Ok, having discovered that #967 is an upstream issue I'm back to looking at this. All the issues I was having seem to be resolved. And everything is so much faster! |
no more issues for me too 🎉 |
Score! Lets merge this. |
Its time to start making the renderer fast! This first round is just picking a lot of low hanging fruit.
I ran my tests using the Bevymark example with 10,000 entities.
Starting Performance
This PR
Summary of changes
(applied in order, so each item's numbers includes the previous item's changes)