-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Stop extracting mesh entities to the render world. #11803
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
FIXME
inextract_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 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
Entity
IDs anymore. As a result, theentity
parameter toRenderCommand::render()
is now wrapped in anOption
. Custom rendering code may need to be updated to handle the case in which noEntity
exists for an object that is to be rendered.