search

bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.39k stars 3.59k forks source link

Use default storage for `TemporaryRenderEntity` #16462

Closed rparrett closed 2 days ago

rparrett commented 3 days ago

Objective

TemporaryRenderEntity currently uses SparseSet storage, but doesn't seem to fit the criteria for a component that would benefit from this.

Typical usage of TemporaryRenderEntity (and all current usages of it in engine as far as I can tell) would be to spawn an entity with it once and then iterate over it once to despawn that entity.

SparseSet is said to be useful for insert/removal perf at the cost of iteration perf.

Solution

Use the default table storage

Testing

Possibly this could show up in stress tests like many_buttons. I didn't do any benchmarking.

BenjaminBrienen commented 2 days ago

Is there a justification from the original author of this line? I don't have the tools to see which PR added it at this moment.

rparrett commented 2 days ago

I couldn't find any discussion about it in either the original or adopted PR that introduced the code.

https://github.com/bevyengine/bevy/pull/14449 https://github.com/bevyengine/bevy/pull/15320

Digging a bit further, it was added in this particular commit https://github.com/bevyengine/bevy/pull/15320/commits/8d2c81a1a887f6786fec944f7b9099e2259351fa but I still don't see any relevant review or conversation.