Insert ResolvedScenes as dynamic bundles#23808
Insert ResolvedScenes as dynamic bundles#23808alice-i-cecile merged 11 commits intobevyengine:mainfrom
Conversation
urben1680
left a comment
There was a problem hiding this comment.
One thing, did not review the other things because they are a bit of deep water for me yet.
alice-i-cecile
left a comment
There was a problem hiding this comment.
I've left a number of comments on the details here, and I think there are a number of modest improvements we could make.
The main questions center around overall architecture though: this is quite a bit of tricky, very niche unsafe code.
Can we copy from the entity cloning machinery (which you called out when talking to @Shatur about related work in BEI)?
The core problem feels very similar to that, and I think that with some changes to how we manage ownership of scratch data we should be able to get something that compiles without the need for this level of sketchiness.
|
If you have safety concerns, maybe just move it to But the API seems reasonable to me. |
alice-i-cecile
left a comment
There was a problem hiding this comment.
Good: the by_id change was my primary concern, and the changes to merge OwnedBundleScratch into BundleWriter make this a bit more robust.
The drop thing would be nice to fix, but eh, probably not worth the overhead. I'm content with this.
| /// written as a dynamic bundle. The contents are cleared after each write and the allocated scratch | ||
| /// space is reused across writes. | ||
| #[derive(Default)] | ||
| pub struct BundleWriter { |
There was a problem hiding this comment.
If I'm reading this correctly, the only difference between BundleWriter and BundleScratch is that BundleScratch stores the Bump externally so it can rely on lifetimes a bit more for safety. Is that right? Will we want to do follow-up work to replace the uses of BundleScratch in the cloning code with BundleWriter?
There was a problem hiding this comment.
Doing so would give some perf wins, which in the context of cloning makes sense. So my vote is yes.
| } | ||
|
|
||
| /// A filter to skip the template for a given `TypeId` | ||
| trait SkipTemplate { |
There was a problem hiding this comment.
Why is this a trait, as opposed to leaving the methods non-generic and just passing an empty HashSet or using Option<HashSet>?
There was a problem hiding this comment.
I was in "skip all unnecessary work" mode. This saves us a branch on each entity spawn (if false {} is trivially optimized out). Given that there are only two versions, I don't think its a steep price to pay.
| /// # Safety | ||
| /// `components` must be from the same world as the components that were pushed to this writer. |
There was a problem hiding this comment.
This should mention to call this method at most once.
There was a problem hiding this comment.
There is no harm in calling this multiple times.
Objective
Currently BSN inserts each component one-by-one. This is incredibly expensive, as it forces an archetype move after every insert.
Solution
Scene templates now write their outputs to reusable bundle scratch space (which uses a bump allocator). The final bundle is written after all components (including inherited components) are written to the bundle.
BundleWriter, which enables defining a dynamic bundle using scratch space in a bump allocator. Currently this only supports writing individual components to theBundleWriter, because supporting arbitrary bundles is much harder (ex: dynamic bundle effects). This sadly means that we are temporarily constraining bothbsn! { Node }andbsn! { @SomeTemplate }"template patches" to require a component output. Custom scene impls can still push arbitrary bundles, which are inserted before the final dynamic bundle write. In practice I believe this will cover the relevant use cases in the short term.ErasedTemplatehas been moved tobevy_scene, as it is now "opinionated", more specific tobevy_scene, and less safe to use in a general contextThese are benchmarks that produce the same UI scene hierarchy through different means (the benchmarks have been updated to have a few "matrix wrapper" components to show the cost of archetype moves):
immediate_function_scene: a test of going through the whole "scene building" process, then spawning. this is the cost of ad-hoc scenes that don't reuse work from inherited scenesimmediate_loaded_scene: a test where we instantiate a bunch of inherited scene instances, where the inherited scene has already been computed / cached.raw_bundle_no_scene: just spawning the raw bundle directly