bevy_render: Reserve only entities from the last frame

[zyllian]

Objective

• Fix High entity counts cause decreased rendering performance even when rendering nothing. #3953

Solution

• Reserve only the number of entities used during the previous frame in the render world

[alice-i-cecile]

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

[zyllian]

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.

[alice-i-cecile]

This strategy makes sense to me: it's certainly better than the existing solution. I think we can actually just compute the exact number of entities to reserve quickly and correctly, which should be preferred (see #3953), but this PR has my approval if that doesn't pan out.

[zyllian]

Sounds good to me.

[nicopap]

Yeah, it should be possible to use #4244 in addition with a Query<(), RenderCriteriaFilter> (where RenderCriteriaFilter is a "good enough" filter that selects only renderable entities) to solve this without having an additional explicitly declared cache. Note that due to the nature of Visibility.is_visible, the cached value approach is always going to be more accurate (and therefore efficient) than relying on the archetype array size. So I would vouch for the explicit cached value.

Note the same problem has been encountered and solved similarly in the extract_mesh system in bevy_pbr, here they store in a Local the size of the array last frame and re-use it.

[nicopap]

Suggested change

	render_app
	.world
	.resource_mut::<NumberOfRenderingEntitiesToReserve>()
	.0 = render_app.world.entities().meta.len() as u32;
	let mut entities_to_reserve = render_app
	.world
	.resource_mut::<NumberOfRenderingEntitiesToReserve>();
	entities_to_reserve.0 = render_app.world.entities().meta.len() as u32;

The PR looks good. But I find assignments at the end of field access chains a bit difficult to read. It could be improved this way.

[alice-i-cecile]

#4244 is merged now; can you update this PR to use it?

[nicopap]

@alice-i-cecile As I mentioned in my earlier reply. Size hint is a double-edged sword. As soon as there is filtering based on non-archetype criteria, it goes woop.

I did an analysis of the impact of size hints on another PR: #4240

To make sure the benefits are there, I suggest benchmarking.

[alice-i-cecile]

Ah right, good choice.

In that case, I'm in favor of merging this as is and creating an issue to investigate this further. The existing strategy is clearly poor.

I'll defer to @superdump on the final call though. @zyllian can you rebase this?

[superdump]

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.

[cart]

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.

[zyllian]

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.