Skip to content
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions benches/benches/bevy_ecs/world/world_get.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pub fn world_query_get(criterion: &mut Criterion) {

for entity_count in RANGE.map(|i| i * 10_000) {
group.bench_function(format!("{entity_count}_entities_table"), |bencher| {
let (mut world, entities) = setup::<Table>(entity_count);
let (world, entities) = setup::<Table>(entity_count);
let mut query = world.query::<&Table>();

bencher.iter(|| {
Expand All @@ -116,7 +116,7 @@ pub fn world_query_get(criterion: &mut Criterion) {
});
});
group.bench_function(format!("{entity_count}_entities_table_wide"), |bencher| {
let (mut world, entities) = setup_wide::<(
let (world, entities) = setup_wide::<(
WideTable<0>,
WideTable<1>,
WideTable<2>,
Expand All @@ -140,7 +140,7 @@ pub fn world_query_get(criterion: &mut Criterion) {
});
});
group.bench_function(format!("{entity_count}_entities_sparse"), |bencher| {
let (mut world, entities) = setup::<Sparse>(entity_count);
let (world, entities) = setup::<Sparse>(entity_count);
let mut query = world.query::<&Sparse>();

bencher.iter(|| {
Expand All @@ -150,7 +150,7 @@ pub fn world_query_get(criterion: &mut Criterion) {
});
});
group.bench_function(format!("{entity_count}_entities_sparse_wide"), |bencher| {
let (mut world, entities) = setup_wide::<(
let (world, entities) = setup_wide::<(
WideSparse<0>,
WideSparse<1>,
WideSparse<2>,
Expand Down Expand Up @@ -185,7 +185,7 @@ pub fn world_query_iter(criterion: &mut Criterion) {

for entity_count in RANGE.map(|i| i * 10_000) {
group.bench_function(format!("{entity_count}_entities_table"), |bencher| {
let (mut world, _) = setup::<Table>(entity_count);
let (world, _) = setup::<Table>(entity_count);
let mut query = world.query::<&Table>();

bencher.iter(|| {
Expand All @@ -199,7 +199,7 @@ pub fn world_query_iter(criterion: &mut Criterion) {
});
});
group.bench_function(format!("{entity_count}_entities_sparse"), |bencher| {
let (mut world, _) = setup::<Sparse>(entity_count);
let (world, _) = setup::<Sparse>(entity_count);
let mut query = world.query::<&Sparse>();

bencher.iter(|| {
Expand All @@ -224,7 +224,7 @@ pub fn world_query_for_each(criterion: &mut Criterion) {

for entity_count in RANGE.map(|i| i * 10_000) {
group.bench_function(format!("{entity_count}_entities_table"), |bencher| {
let (mut world, _) = setup::<Table>(entity_count);
let (world, _) = setup::<Table>(entity_count);
let mut query = world.query::<&Table>();

bencher.iter(|| {
Expand All @@ -238,7 +238,7 @@ pub fn world_query_for_each(criterion: &mut Criterion) {
});
});
group.bench_function(format!("{entity_count}_entities_sparse"), |bencher| {
let (mut world, _) = setup::<Sparse>(entity_count);
let (world, _) = setup::<Sparse>(entity_count);
let mut query = world.query::<&Sparse>();

bencher.iter(|| {
Expand Down
8 changes: 5 additions & 3 deletions crates/bevy_asset/src/asset_changed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ impl<'w, A: AsAssetId> AssetChangeCheck<'w, A> {
///
/// - Asset changes are registered in the [`AssetEventSystems`] system set.
/// - Removed assets are not detected.
/// - The asset must be initialized ([`App::init_asset`](crate::AssetApp::init_asset)).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a totally fine limitation: this filter is useless without initialized assets.

///
/// The list of changed assets only gets updated in the [`AssetEventSystems`] system set,
/// which runs in `PostUpdate`. Therefore, `AssetChanged` will only pick up asset changes in schedules
Expand Down Expand Up @@ -235,9 +236,10 @@ unsafe impl<A: AsAssetId> WorldQuery for AssetChanged<A> {
access.add_resource_read(state.resource_id);
}

fn init_state(world: &mut World) -> AssetChangedState<A> {
let resource_id = world.init_resource::<AssetChanges<A::Asset>>();
let asset_id = world.register_component::<A>();
fn init_state(world: &World) -> AssetChangedState<A> {
let components = world.components_queue();
let resource_id = components.queue_register_resource::<AssetChanges<A::Asset>>();
let asset_id = components.queue_register_component::<A>();
AssetChangedState {
asset_id,
resource_id,
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ pub use server::*;
pub use uuid;

use crate::{
asset_changed::AssetChanges,
io::{embedded::EmbeddedAssetRegistry, AssetSourceBuilder, AssetSourceBuilders, AssetSourceId},
processor::{AssetProcessor, Process},
};
Expand Down Expand Up @@ -650,6 +651,7 @@ impl AssetApp for App {
));
}
self.insert_resource(assets)
.init_resource::<AssetChanges<A>>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason this used to be initialized only in the AssetChanged filter is to avoid the overhead of updating the resource if nothing was using it. There's an asset_events system that takes Option<ResMut<AssetChanges<A>>> and only updates it if the resource exists:

asset_changes: Option<ResMut<AssetChanges<A>>>,

I don't know enough about assets to evaluate how important that is, though.


If we do need to preserve that behavior, I think there are still ways to do it with only &World. Maybe a resource with a ConcurrentQueue<fn(&mut World)>? AssetChanged::init_state would push |world| world.init_resource::<AssetChanges<A>>() and an exclusive system that runs before(AssetEventSystems) would drain the queue and run the function pointers? AssetChanged filters are rare enough that the extra atomics during initialization shouldn't be too expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not ideal. I'd like more information about the perf implications here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion here. It's quite unfortunate to have this running for every asset type though - some asset types may mutate frequently (e.g., Materials) but those asset types are also most likely to actually take advantage of this feature.

I wouldn't block on preserving this unless folks complain (which seems unlikely). With assets-as-entities unblocked, we may also throw out this type entirely, so putting the work in to fix it seems a little early.

.allow_ambiguous_resource::<Assets<A>>()
.add_message::<AssetEvent<A>>()
.add_message::<AssetLoadFailedEvent<A>>()
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/fullscreen_material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ impl<T: FullscreenMaterial> Plugin for FullscreenMaterialPlugin<T> {
}

fn extract_on_add<T: FullscreenMaterial>(world: &mut World) {
world.resource_scope::<MainWorld, ()>(|world, mut main_world| {
world.resource_scope::<MainWorld, ()>(|world, main_world| {
// Extract the material from the main world
let mut query =
main_world.query_filtered::<(Entity, Has<Camera3d>, Has<Camera2d>), Added<T>>();
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
// - `Bundle::get_components` is exactly once for each member. Rely's on the Component -> Bundle implementation to properly pass
// the correct `StorageType` into the callback.
unsafe impl #impl_generics #ecs_path::bundle::Bundle for #struct_name #ty_generics #where_clause {
fn component_ids(
components: &mut #ecs_path::component::ComponentsRegistrator,
) -> impl Iterator<Item = #ecs_path::component::ComponentId> + use<#(#generics_ty_list,)*> {
fn component_ids<Components: #ecs_path::component::ComponentIdDictator>(
components: &mut Components,
) -> impl Iterator<Item = #ecs_path::component::ComponentId> + use<#(#generics_ty_list,)* Components> {
core::iter::empty()#(.chain(<#active_field_types as #ecs_path::bundle::Bundle>::component_ids(components)))*
}

Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/world_query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ pub(crate) fn world_query_impl(
#( <#field_types>::update_component_access(&state.#field_aliases, _access); )*
}

fn init_state(world: &mut #path::world::World) -> #state_struct_name #user_ty_generics {
fn init_state(world: &#path::world::World) -> #state_struct_name #user_ty_generics {
#state_struct_name {
#(#field_aliases: <#field_types>::init_state(world),)*
}
Expand Down
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/bundle/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ use variadics_please::all_tuples_enumerated;

use crate::{
bundle::{Bundle, BundleFromComponents, DynamicBundle, NoBundleEffect},
component::{Component, ComponentId, Components, ComponentsRegistrator, StorageType},
component::{Component, ComponentId, ComponentIdDictator, Components, StorageType},
world::EntityWorldMut,
};

// SAFETY:
// - `Bundle::component_ids` calls `ids` for C's component id (and nothing else)
// - `Bundle::get_components` is called exactly once for C and passes the component's storage type based on its associated constant.
unsafe impl<C: Component> Bundle for C {
fn component_ids(
components: &mut ComponentsRegistrator,
) -> impl Iterator<Item = ComponentId> + use<C> {
iter::once(components.register_component::<C>())
fn component_ids<Components: ComponentIdDictator>(
components: &mut Components,
) -> impl Iterator<Item = ComponentId> + use<C, Components> {
iter::once(components.determine_component_id_of::<C>())
}

fn get_component_ids(components: &Components) -> impl Iterator<Item = Option<ComponentId>> {
Expand Down Expand Up @@ -73,7 +73,7 @@ macro_rules! tuple_impl {
// - `Bundle::get_components` is called exactly once for each member. Relies on the above implementation to pass the correct
// `StorageType` into the callback.
unsafe impl<$($name: Bundle),*> Bundle for ($($name,)*) {
fn component_ids<'a>(components: &'a mut ComponentsRegistrator) -> impl Iterator<Item = ComponentId> + use<$($name,)*> {
fn component_ids<'a, Components: ComponentIdDictator>(components: &'a mut Components) -> impl Iterator<Item = ComponentId> + use<$($name,)* Components> {
iter::empty()$(.chain(<$name as Bundle>::component_ids(components)))*
}

Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_ecs/src/bundle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ pub use info::*;
pub use bevy_ecs_macros::Bundle;

use crate::{
component::{ComponentId, Components, ComponentsRegistrator, StorageType},
component::{ComponentId, ComponentIdDictator, Components, StorageType},
world::EntityWorldMut,
};
use bevy_ptr::OwningPtr;
Expand Down Expand Up @@ -204,11 +204,11 @@ use bevy_ptr::OwningPtr;
)]
pub unsafe trait Bundle: DynamicBundle + Send + Sync + 'static {
/// Gets this [`Bundle`]'s component ids, in the order of this bundle's [`Component`]s
/// This will register the component if it doesn't exist.
/// This may register the component if it doesn't exist, depending on which [`ComponentIdDictator`] is used.
#[doc(hidden)]
fn component_ids(
components: &mut ComponentsRegistrator,
) -> impl Iterator<Item = ComponentId> + use<Self>;
fn component_ids<Components: ComponentIdDictator>(
components: &mut Components,
) -> impl Iterator<Item = ComponentId> + use<Self, Components>;

/// Return a iterator over this [`Bundle`]'s component ids. This will be [`None`] if the component has not been registered.
fn get_component_ids(components: &Components) -> impl Iterator<Item = Option<ComponentId>>;
Expand Down
24 changes: 24 additions & 0 deletions crates/bevy_ecs/src/component/register.rs
Original file line number Diff line number Diff line change
Expand Up @@ -698,3 +698,27 @@ impl<'w> ComponentsQueuedRegistrator<'w> {
})
}
}

/// Represents a way to get the id of component types.
///
/// This exists to encapsulate shared behavior of [`ComponentsRegistrator`] and [`ComponentsQueuedRegistrator`].
/// If you know which one you are working with, prefer their direct methods.
pub trait ComponentIdDictator {
/// Determines the [`ComponentId`] of `T`.
/// This makes no promises of whether or not `T` will be full registered; it just gets its id.
fn determine_component_id_of<T: Component>(&mut self) -> ComponentId;
}

impl ComponentIdDictator for ComponentsRegistrator<'_> {
#[inline]
fn determine_component_id_of<T: Component>(&mut self) -> ComponentId {
self.register_component::<T>()
}
}

impl ComponentIdDictator for ComponentsQueuedRegistrator<'_> {
#[inline]
fn determine_component_id_of<T: Component>(&mut self) -> ComponentId {
self.queue_register_component::<T>()
}
}
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/entity/entity_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ mod tests {
fn preserving_uniqueness() {
let mut world = World::new();

let mut query = QueryState::<&mut Thing>::new(&mut world);
let mut query = QueryState::<&mut Thing>::new(&world);

let spawn_batch: Vec<Entity> = world.spawn_batch(vec![Thing; 1000]).collect();

Expand Down
24 changes: 12 additions & 12 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1466,62 +1466,62 @@ mod tests {
#[test]
#[should_panic]
fn ref_and_mut_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(&A, &mut A)>();
}

#[test]
#[should_panic]
fn entity_ref_and_mut_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(EntityRef, &mut A)>();
}

#[test]
#[should_panic]
fn mut_and_ref_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(&mut A, &A)>();
}

#[test]
#[should_panic]
fn mut_and_entity_ref_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(&mut A, EntityRef)>();
}

#[test]
#[should_panic]
fn entity_ref_and_entity_mut_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(EntityRef, EntityMut)>();
}

#[test]
#[should_panic]
fn entity_mut_and_entity_mut_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(EntityMut, EntityMut)>();
}

#[test]
fn entity_ref_and_entity_ref_query_no_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(EntityRef, EntityRef)>();
}

#[test]
#[should_panic]
fn mut_and_mut_query_panic() {
let mut world = World::new();
let world = World::new();
world.query::<(&mut A, &mut A)>();
}

#[test]
#[should_panic]
fn multiple_worlds_same_query_iter() {
let mut world_a = World::new();
let world_a = World::new();
let world_b = World::new();
let mut query = world_a.query::<&A>();
query.iter(&world_a);
Expand All @@ -1530,7 +1530,7 @@ mod tests {

#[test]
fn query_filters_dont_collide_with_fetches() {
let mut world = World::new();
let world = World::new();
world.query_filtered::<&mut A, Changed<A>>();
}

Expand All @@ -1555,7 +1555,7 @@ mod tests {
#[test]
#[should_panic]
fn multiple_worlds_same_query_get() {
let mut world_a = World::new();
let world_a = World::new();
let world_b = World::new();
let mut query = world_a.query::<&A>();
let _ = query.get(&world_a, Entity::from_raw_u32(0).unwrap());
Expand All @@ -1565,7 +1565,7 @@ mod tests {
#[test]
#[should_panic]
fn multiple_worlds_same_query_for_each() {
let mut world_a = World::new();
let world_a = World::new();
let world_b = World::new();
let mut query = world_a.query::<&A>();
query.iter(&world_a).for_each(|_| {});
Expand Down
8 changes: 6 additions & 2 deletions crates/bevy_ecs/src/observer/distributed_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,11 +433,15 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
) {
world.commands().queue(move |world: &mut World| {
let event_key = world.register_event_key::<E>();
let components = B::component_ids(&mut world.components_registrator());
let components = B::component_ids(&mut world.components_registrator())
.collect::<smallvec::SmallVec<[ComponentId; 16]>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this collect is because component_ids captures the lifetime of the ComponentsRegistrator type now that it's a generic parameter, which means this now conflicts with the world borrow in world.get_mut::<Observer>. And the compiler forces it to capture that lifetime, even though the return types are always actually 'static and the use<> syntax looks like it would support leaving some parameters out.

But this only actually allocates when creating an observer with more than 16 components, which should basically never happen and which is already allocating to box the system, so the cost is pretty low.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. We could monomorphize component_ids ourselves to fix this, but that didn't feel worth it. And this should be fixable once we can specify the use<> bounds. I also tried adding a 'static bound, but that didn't work either.


if let Some(mut observer) = world.get_mut::<Observer>(entity) {
observer.descriptor.event_keys.push(event_key);
observer.descriptor.components.extend(components);
observer
.descriptor
.components
.extend_from_slice(&components);

let system: &mut dyn Any = observer.system.as_mut();
let system: *mut dyn ObserverSystem<E, B> = system.downcast_mut::<S>().unwrap();
Expand Down
Loading
Loading