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.

Sign up for GitHub

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

bevy_render: Reserve only entities from the last frame #4322

Closed
wants to merge 4 commits into from
Closed

bevy_render: Reserve only entities from the last frame #4322

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69bc1b2
bevy_render: Reserve only entities from the last frame
Mar 24, 2022
32d7833
Rename `ReserveCount` to be more descriptive
Mar 24, 2022
c48db70
Fix missing renamed `NumberOfRenderingEntitiesToReserve`
Mar 24, 2022
07fceec
Fix access error
May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions crates/bevy_render/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ pub struct RenderApp;
#[derive(Default)]
struct ScratchRenderWorld(World);

/// The number of entities used in the render world in the previous frame.
#[derive(Default)]
Copy link
Member

Choose a reason for hiding this comment

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

I think it may make sense to initialize this at something low but not tiny, like perhaps 100?

Copy link
Author

@zyllian zyllian Mar 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Initializing would only work if rendering entities are added in the first frame. I think a minimum or offset applied when reserving would probably be better.

struct NumberOfRenderingEntitiesToReserve(u32);

impl Plugin for RenderPlugin {
/// Initializes the renderer, sets up the [`RenderStage`](RenderStage) and creates the rendering sub-app.
fn build(&self, app: &mut App) {
Expand Down Expand Up @@ -176,7 +180,8 @@ impl Plugin for RenderPlugin {
.insert_resource(adapter_info)
.insert_resource(pipeline_cache)
.insert_resource(asset_server)
.init_resource::<RenderGraph>();
.init_resource::<RenderGraph>()
.init_resource::<NumberOfRenderingEntitiesToReserve>();

app.add_sub_app(RenderApp, render_app, move |app_world, render_app| {
#[cfg(feature = "trace")]
Expand All @@ -187,13 +192,13 @@ impl Plugin for RenderPlugin {
bevy_utils::tracing::info_span!("stage", name = "reserve_and_flush")
.entered();

// reserve all existing app entities for use in render_app
// reserve the number of entities used in the previous frame for use in render_app
Copy link
Contributor

@superdump superdump May 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The point of reserving the entities is so that the same entity from the main world can be used to refer to an entity in the render world. If we don’t reserve the entire active main world entity address space, something could be spawned after what is reserved and then something else is supposed to refer to that. I’m not convinced this change is safe to do. I’ll need to think about it some more. Maybe @cart can beat me to it.

Copy link
Member

Choose a reason for hiding this comment

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

Yup the approach in this PR breaks the intended behavior. This line isn't about reserving space for rendering entities. Its about ensuring that rendering entities don't get spawned in the "current app id space".

We should not merge this PR.

Copy link
Author

Choose a reason for hiding this comment

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

When I made the change I was expecting it to not work at all so I could learn more about Bevy's internals. I'm curious why it doesn't obviously break things, though.

// they can only be spawned using `get_or_spawn()`
let meta_len = app_world.entities().meta_len();
render_app
let count = render_app
.world
.entities()
.reserve_entities(meta_len as u32);
.resource::<NumberOfRenderingEntitiesToReserve>()
.0;
render_app.world.entities().reserve_entities(count);

// flushing as "invalid" ensures that app world entities aren't added as "empty archetype" entities by default
// these entities cannot be accessed without spawning directly onto them
Expand Down Expand Up @@ -274,6 +279,11 @@ impl Plugin for RenderPlugin {
.unwrap();
cleanup.run(&mut render_app.world);

render_app
.world
.resource_mut::<NumberOfRenderingEntitiesToReserve>()
.0 = render_app.world.entities().meta_len() as u32;

render_app.world.clear_entities();
}
});
Expand Down