Stop extracting mesh entities to the render world. #11803
Conversation
This fixes a `FIXME` in `extract_meshes` and results in a performance improvement. As a result of this change, meshes in the render world might not be attached to entities anymore. Therefore, the `entity` parameter to `RenderCommand::render()` is now wrapped in an `Option`. Most applications that use the render app's ECS can simply unwrap the `Option`. Note that for now sprites, gizmos, and UI elements still use the render world as usual. Migration guide --------------- * For efficiency reasons, some meshes in the render world may not have corresponding `Entity` IDs anymore. As a result, the `entity` parameter to `RenderCommand::render()` is now wrapped in an `Option`. Custom rendering code may need to be updated to handle the case in which no `Entity` exists for an object that is to be rendered.
|
CI failed formatting. |
alice-i-cecile
added
A-Rendering
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
There was a problem hiding this comment.
Code looks good to me.
Profiling reveals a small but reliable improvement in extract_meshs, but I could not detect a change in the frame-rate or render schedule timings on the scenes I have available. Tested on many_cubes, many_foxes and several of the gltf test scenes using scene_viewer.
This is from many_cubes.
This PR paves the way for some other improvements that could take extract_meshes down into the nanoseconds most of the time.
The breaking change is pretty minor so I think we could still merge this for 0.13.
pcwalton
added
the
S-Ready-For-Final-Review
There was a problem hiding this comment.
The render command encoding seems to be ~4% slower on average (tested on many_foxes):
This is probably a result of the panicking added into RenderCommand implementations, and this is likely still preferable than spending the time in the extract schedule as it blocks both the render and main schedules.
I can confirm similar performance improvements to what @NthTensor is noting with everything else though.
|
The migration guide here is a bit mysterious. Rust's suggestions for fixing up the function params seem good enough, but maybe it could be amended to specifically mention returning |
This fixes a
FIXMEinextract_meshesand results in a performance improvement.As a result of this change, meshes in the render world might not be attached to entities anymore. Therefore, the
entityparameter toRenderCommand::render()is now wrapped in anOption. Most applications that use the render app's ECS can simply unwrap theOption.Note that for now sprites, gizmos, and UI elements still use the render world as usual.
Migration guide
EntityIDs anymore. As a result, theentityparameter toRenderCommand::render()is now wrapped in anOption. Custom rendering code may need to be updated to handle the case in which noEntityexists for an object that is to be rendered.