Entity ranges#24102
Conversation
alice-i-cecile
added
D-Complex
ElliottjPierce
left a comment
ElliottjPierce
left a comment
There was a problem hiding this comment.
I like the direction of this. I've got a few suggestions to improve performance, and there's somewhat of a problem in the new FreshEntityAllocator::alloc (see my comment there), but nothing too major. Looking forward to components as entities!
| let new = match start_new | ||
| .checked_add(count) | ||
| .filter(|new| *new < Self::MAX_ENTITIES) | ||
| .filter(|new| *new < self.max_index) |
There was a problem hiding this comment.
It would be nice if we could instead say, "Here's the id range you could allocate," so it never panics; it just sometimes doesn't contain all count entities. But that's definitely not for this PR. I think this is the correct implementation for now.
# Objective - Provide a higher quality texture compressor - Automatically generate mipmaps Closes bevyengine#14671. ## Solution - Use the ctt crate ## Testing - New compressed_image_saver example (for now I have merged the textures into this branch, but before merging we should place them in the bevy asset repo) --- ## Showcase <img width="3206" height="1875" alt="image" src="https://github.com/user-attachments/assets/4b236f00-3f5d-4618-a53a-efcc74e9d32b" />
…yengine#24065) # Objective Alternative to bevyengine#24004. bevyengine#23288 adds ltc luts for rect light support which implicitly requires `bevy_image/ktx2` and `bevy_image/zstd` otherwise loading ltc luts will panic. We either accept to always enable area light supoort (bevyengine#24004), or add a feature to opt out it (this PR). ## Solution Gate ltc luts behind a feature and merge them to a texture array. ## Testing `rect_light` example works. --------- Co-authored-by: Kevin Chen <chen.kevin.f@gmail.com>
…f `T` type (bevyengine#24048) # Objective Fixes issue where handles generate no problem regardless of T type via `from_reflect` due to strong handle's being fully opaque and simply cloned. ## Solution Adds a small type id check to the `FromReflect` implemenation which fails conversion if the type id is not what we expect: Reference automatically derived implementation: ```rust impl<A: Asset> bevy_reflect::FromReflect for Handle<A> where Handle<A>: ::core::any::Any + ::core::marker::Send + ::core::marker::Sync, A: bevy_reflect::TypePath, { fn from_reflect(__param0: &dyn bevy_reflect::PartialReflect) -> ::core::option::Option<Self> { if let bevy_reflect::ReflectRef::Enum(__param0) = bevy_reflect::PartialReflect::reflect_ref(__param0) { match bevy_reflect::enums::Enum::variant_name(__param0) { "Strong" => ::core::option::Option::Some(Handle::Strong { 0: { let __0 = __param0.field_at(0usize); let __0 = __0?; <Arc<StrongHandle> as bevy_reflect::FromReflect>::from_reflect(__0)? }, }), "Uuid" => ::core::option::Option::Some(Handle::Uuid { 0: { let __0 = __param0.field_at(0usize); let __0 = __0?; <Uuid as bevy_reflect::FromReflect>::from_reflect(__0)? }, 1: ::core::default::Default::default(), }), name => panic!( "variant with name `{}` does not exist on enum `{}`", name, <Self as bevy_reflect::TypePath>::type_path() ), } } else { ::core::option::Option::None } } } ``` ## Testing added basic tests --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Greeble <166992735+greeble-dev@users.noreply.github.com>
…e#24089) # Objective - Alongside bevyengine#24086, helps with bevyengine#24084, although I think we should double check any other added conditionals for bind group entries to make sure they are accurate. ## Solution So, originally the `SCREEN_SPACE_TRANSMISSION` was enabled with `key.intersects(MeshPipelineKey::SCREEN_SPACE_SPECULAR_TRANSMISSION_RESERVED_BITS)`. However, a low quality transmission would make this false, since low’s MeshPipelineKey is configured like this: `const SCREEN_SPACE_SPECULAR_TRANSMISSION_LOW = 0 << Self::SCREEN_SPACE_SPECULAR_TRANSMISSION_SHIFT_BITS;`. So, a ScreenSpaceTransmission with Low Quality would break rendering (since another if-block merely checks that the `ScreenSpaceTransmission` component exists) Making it so that the low transmission truly does *not* include the view_transmission_textures makes the transmission render not properly - the spheres disappear! So, I think the proper fix here is to remove the gating around transmission textures. Edit: Another potential fix is to change the condition of the `intersects` but I’m not sure how to encode what we want unless we want to add another bit for `ScreenSpaceTransmission` component exists essentially? Happy to close this PR if that is an acceptable direction. ## Testing `cargo run --example transmission` works for all quality levels.
…evyengine#24046) I was initially using `MessageReader<WindowResized>` in my system for my app but once my system grew to use more and more window events, I refactored to using `MessageReader<WindowEvent>` and matching on its variants. This is where I ran into the issue of the `WindowEvent::WindowResized` case never matching. When making this PR, I noticed `WindowEvent::WindowBackendScaleFactorChanged` and `WindowEvent::WindowScaleFactorChanged` had the same issue. # Objective Fixes bevyengine#15268 ## Solution Instead of writing into `MessageWriter<WindowResized>`, `MessageWriter<WindowBackendScaleFactorChanged>` and `MessageWriter<WindowScaleFactorChanged>`, push into `bevy_window_events` where it gets sent to the `forward_bevy_events` function for syncing the messages. ## Testing I made local modifications to the `window_resizing` example to use a `MessageReader<WindowEvent>` instead of `MessageReader<WindowResized>` like so: ```rs fn on_resize_system( mut text: Single<&mut Text, With<ResolutionText>>, mut resize_reader: MessageReader<WindowEvent>, ) { for e in resize_reader.read() { if let WindowEvent::WindowResized(e) = e { // When resolution is being changed text.0 = format!("{:.1} x {:.1}", e.width, e.height); } } } ``` I ran this example on linux wayland.
# Objective Make the possibility of 1-to-1 `Relationship` clearer in the docs, so that people know it's an option. (There's already a passing mention of it at the top, but that doesn't show that it's supported in code.) ## Solution Added an example to the doc comment to show how it's done, and explained what happens if you try to add another entity anyway. ## Testing Ran `cargo doc` and looked at the new docs to see if they're ok.
chescock
left a comment
chescock
left a comment
There was a problem hiding this comment.
This looks good, but I agree with @ElliottjPierce that we need to handle the case where fetch_add overflows, so I'll hold off on clicking Approve until that's resolved.
I've ran some micro benchmarks and I'll be honest: They do not look good.
I'm surprised that this would cause a big difference! It might be worth checking the assembly for calls to alloc. The only difference I would expect is the overflow check changing from comparing to a constant to comparing to a variable. Maybe that's already enough in code this hot? Or maybe something is failing to inline where it should.
|
New Perf numbers! I've also tried adding |
|
Broken by the force push noted in #24130; you'll need to clean up the Git history per @cart's message. |
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
|
I reran the numbers. I think it's good enough now. Not perfect, but good enough to proceed. |
ElliottjPierce
left a comment
ElliottjPierce
left a comment
There was a problem hiding this comment.
I'm satisfied with the implementation at this point, just a few nitpicks and docs stuff for the api, and I think this is ready.
Also, heads up that this brought back in the large assets, so there's probably some git cleanup to do, but I haven't been following that, so IDK.
mockersf
left a comment
mockersf
left a comment
There was a problem hiding this comment.
PR contains the reverted compressed_image_saver changes, it shouldn't
| /// | ||
| /// Conceptually, this is a collection of [`Entity`] ids who's [`EntityIndex`] is despawned and who's [`EntityGeneration`] is the most recent. | ||
| /// See the module docs for how these ids and this allocator participate in the life cycle of an entity. | ||
| #[derive(Default, Debug)] |
There was a problem hiding this comment.
Removing Default here is technically a breaking change, right? And it's no longer possible to create these at all? You might need a migration guide.
There was a problem hiding this comment.
Shoot, you're right. Maybe it should stay Default after all. But definitely the world and stuff should make it with the range instead of the default IMO, but that change could be saved for later too, so up to you.
ElliottjPierce
left a comment
ElliottjPierce
left a comment
There was a problem hiding this comment.
I'm happy with this! Components as entities here we come!
|
I'll remove the Default later, no breaking changes in this PR. |
Trashtalk217
added
S-Ready-For-Final-Review
|
Two things: Second, if they need only be contiguous, why do we need to be able to give exact ranges to the allocator? A more focused change would be to say "Give me N contiguous entities", instead of "Give me entities ranging from X to Y". |
|
To your first point, I'd say an index op is much faster than a hashmap lookup, so I think the perf diff would be pretty big. Even if not, it still makes sense for an ecs to pursue the maximum performance wherever possible. (But I agree that it would be nice to see the benchmarks out of curiosity.) To your second point, we need the range of entities to start at 0 ideally for faster indexing, so starting at 0 and with a length of Weighing in on the entity allocator, specifying ranges like this is extremely useful for lots of other stuff. Ex: syncing an entity id space between two worlds is really useful for multiplayer, and this makes it easy. This isn't worth making public though until we have entity paging to make large ranges performant. You're right though that this could be done in a more focused way. Just allocate |
While we do aim for great performance, it does get more complicated once performance infrastructure changes behavior.
The thing is, I'm not sure whether fragmentation is relevant for the use case of this PR. Once we spill over the first range, the rest will be fragmented. When you say it would be hard to depend on, what are you referring to?
I agree that there is a lot of potential use that this could see, but does that discussion belong paired to the motivation of this PR?
I think there is still more inbetween space here. If we allow Allocators to be divided into sizes of I'm not necessarily against the more complex interface, but I think it is worthy of more detailed discussion of its own. ECS SME capacity is already sparsely allocated, so punting on these questions and/or giving them proper space may make it easier for them and other contributors to weigh in. |
|
With regards to performance. I opened a branch where the entity domain isn't split and we use normal hashmaps and hashsets: https://github.com/Trashtalk217/bevy/tree/alternate-comps-as-ents I got the following result: I've also ran There was no material decrease in frame time. I've also ran And had a minor increase in fps (~708 fps -> ~730fps). This was the motivation behind entity ranges. Given that flecs also uses this, I feel confident that this will help. To roughly respond to (some of) your comments:
Sure, if I were able to break off an
Components should get the 0 index, because they are components. They are easily the most important entities in the ECS. |
I just mean that I'd be wary of putting code in the allocator that assumes there is no fragmentation. It could easily be misused.
You mean like a private
This is true, but I think components are the only thing that would benefit from having a dense id like this. Systems, assets, etc could have too many entities to reliably fit in a pre-allocated range anyway. Plus, most of their data will be stored as components on their entities. Components are the one and only exception to this. That is, "components as entities" is the only case where the actual data can't be stored only as components because that would be circular. If we let components as entities start at 0, literately ever other X-as-entities becomes faster. As it stands, the only thing I would flag with this approach as being wastefully over complicated is that the components entity id allocator doesn't need to have I'm open to other designs, but unless I'm misunderstanding (which is entirely possible), I don't think there's an objectively simpler approach here, and the reusability with entity pages (maybe) is certainly a cherry on top. |
14 participants
Objective
For 'components as entities' (#23988), we move away from a seperate
ComponentIdallocator to theEntityAllocator. This has some advantages, but one major downside.ComponentIds are no longer 'dense'. Take the following code:Assuming that we've never initialized
ComponentAandComponentBbefore, this code does four things:ComponentAwith theWorld, this results inComponentId(0)ComponentA, entity indices 1 through 100 are allocated here.ComponentBwith theWorld, this results inComponentId(101)ComponentB.If you now have a
SparseArrayorFixedIndexSetthat relies on the indices ofComponentIds to stay relatively small, this becomes a problem, because 99% of the space is wasted on entities that are neverComponentIds.Solution
We introduce ranges to
EntityAllocator, such that you can use different allocators for different purposes. Saycompont_id_allocator = EntityAllocator::new(0..1024), whileentity_allocator = EntityAllocator::new(1024..), then the first 1024 components are allocated sequentially, after that, you can fallback on the entity allocator.This does require the use of hybrid data structures, that use an array for the first
nelements and a hashmap for when the index becomes larger thann.Prior Art
Flecs uses entity ranges, reserving the low entity indexes for components. Read more about 'Id Partitioning'.
Benchmarking
I've ran some micro benchmarks and I'll be honest: They do not look good.
Details