Skip to content
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

EntityRef/Mut get_components #13375

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
59 changes: 56 additions & 3 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
change_detection::MutUntyped,
component::{Component, ComponentId, ComponentTicks, Components, StorageType},
entity::{Entities, Entity, EntityLocation},
query::{Access, DebugCheckedUnwrap},
query::{Access, DebugCheckedUnwrap, QueryData, ReadOnlyQueryData},
removal_detection::RemovedComponentEvents,
storage::Storages,
world::{Mut, World},
Expand Down Expand Up @@ -153,6 +153,17 @@ impl<'w> EntityRef<'w> {
// SAFETY: We have read-only access to all components of this entity.
unsafe { self.0.get_by_id(component_id) }
}

/// Returns read-only components for the current entity that match the query `Q`.
pub fn components<Q: ReadOnlyQueryData>(&self) -> Q::Item<'_> {
self.get_components::<Q>().unwrap()
}

/// Returns read-only components for the current entity that match the query `Q`.
pub fn get_components<Q: ReadOnlyQueryData>(&self) -> Option<Q::Item<'_>> {
// SAFETY: &mut self implies exclusive access for duration of returned value
cart marked this conversation as resolved.
Show resolved Hide resolved
unsafe { self.0.get_components::<Q>() }
}
}

impl<'w> From<EntityWorldMut<'w>> for EntityRef<'w> {
Expand Down Expand Up @@ -348,6 +359,28 @@ impl<'w> EntityMut<'w> {
self.as_readonly().get()
}

/// Returns read-only components for the current entity that match the query `Q`.
pub fn components<Q: ReadOnlyQueryData>(&self) -> Q::Item<'_> {
self.get_components::<Q>().unwrap()
}

/// Returns components for the current entity that match the query `Q`.
pub fn components_mut<Q: QueryData>(&mut self) -> Q::Item<'_> {
self.get_components_mut::<Q>().unwrap()
}

/// Returns read-only components for the current entity that match the query `Q`.
pub fn get_components<Q: ReadOnlyQueryData>(&self) -> Option<Q::Item<'_>> {
// SAFETY: &mut self implies exclusive access for duration of returned value
cart marked this conversation as resolved.
Show resolved Hide resolved
unsafe { self.0.get_components::<Q>() }
}

/// Returns components for the current entity that match the query `Q`.
pub fn get_components_mut<Q: QueryData>(&mut self) -> Option<Q::Item<'_>> {
// SAFETY: &mut self implies exclusive access for duration of returned value
unsafe { self.0.get_components::<Q>() }
}

/// Consumes `self` and gets access to the component of type `T` with the
/// world `'w` lifetime for the current entity.
///
Expand Down Expand Up @@ -1770,7 +1803,7 @@ impl<'w> FilteredEntityRef<'w> {

/// Returns an iterator over the component ids that are accessed by self.
#[inline]
pub fn components(&self) -> impl Iterator<Item = ComponentId> + '_ {
pub fn accessed_components(&self) -> impl Iterator<Item = ComponentId> + '_ {
self.access.reads_and_writes()
}

Expand Down Expand Up @@ -2024,7 +2057,7 @@ impl<'w> FilteredEntityMut<'w> {

/// Returns an iterator over the component ids that are accessed by self.
#[inline]
pub fn components(&self) -> impl Iterator<Item = ComponentId> + '_ {
pub fn accessed_components(&self) -> impl Iterator<Item = ComponentId> + '_ {
self.access.reads_and_writes()
}

Expand Down Expand Up @@ -2918,4 +2951,24 @@ mod tests {

assert_is_system(incompatible_system);
}

#[test]
fn get_components() {
#[derive(Component, PartialEq, Eq, Debug)]
struct X(usize);

#[derive(Component, PartialEq, Eq, Debug)]
struct Y(usize);
let mut world = World::default();
let e1 = world.spawn((X(7), Y(10))).id();
let e2 = world.spawn(X(8)).id();
let e3 = world.spawn_empty().id();

assert_eq!(
Some((&X(7), &Y(10))),
world.entity(e1).get_components::<(&X, &Y)>()
);
assert_eq!(None, world.entity(e2).get_components::<(&X, &Y)>());
assert_eq!(None, world.entity(e3).get_components::<(&X, &Y)>());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test to make sure it returns none or panics if you do world.entity_mut(e).get_components_mut::<&mut X, &mut X>()?

Copy link
Contributor

Choose a reason for hiding this comment

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

also should have one for (&mut X, &X)

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha I'm so glad you asked this question. The Access part of the Query infrastructure was what enforced this elsewhere, so this was allowed. We can't merge this as-is, and fixing it will almost certainly incur a significant perf cost.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't creating an empty access and calling update_component_access(state, &mut access) on it work?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would. It would just be dissatisfying from a perf perspective because Access allocates.

Copy link
Contributor

@cBournhonesque cBournhonesque Jun 26, 2024

Choose a reason for hiding this comment

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

FYI I have a PR that removes the allocation from an empty Access: https://github.com/bevyengine/bevy/pull/14026/files which might unblock this?

Or maybe not.. calling update_component_access will itself allocate by updating the bitsets

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the allocation not just be removed in the meantime by using a smallvec that has 8 items or something?

}
47 changes: 47 additions & 0 deletions crates/bevy_ecs/src/world/unsafe_world_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::{
component::{ComponentId, ComponentTicks, Components, StorageType, Tick, TickCells},
entity::{Entities, Entity, EntityLocation},
prelude::Component,
query::{DebugCheckedUnwrap, QueryData},
removal_detection::RemovedComponentEvents,
storage::{Column, ComponentSparseSet, Storages},
system::{Res, Resource},
Expand Down Expand Up @@ -818,6 +819,52 @@ impl<'w> UnsafeEntityCell<'w> {
})
}
}

/// # Safety
/// It is the callers responsibility to ensure that
/// - the [`UnsafeEntityCell`] has permission to access the queried data mutably
/// - no other references to the queried data exist at the same time
pub(crate) unsafe fn get_components<Q: QueryData>(&self) -> Option<Q::Item<'_>> {
// SAFETY: World is only used to access query data and initialize query state
let state = unsafe {
let world = self.world().world();
Q::get_state(world)?
};
let location = self.location();
// SAFETY: Location is guaranteed to exist
let archetype = unsafe {
self.world
.archetypes()
.get(location.archetype_id)
.debug_checked_unwrap()
};
if Q::matches_component_set(&state, &|id| archetype.contains(id)) {
Copy link
Member

Choose a reason for hiding this comment

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

Could use bool::then instead of the if/else here.

// SAFETY: state was initialized above using the world passed into this function
let mut fetch = unsafe {
Q::init_fetch(
self.world,
&state,
self.world.last_change_tick(),
self.world.change_tick(),
)
};
// SAFETY: Table is guaranteed to exist
let table = unsafe {
self.world
.storages()
.tables
.get(location.table_id)
.debug_checked_unwrap()
};
// SAFETY: Archetype and table are from the same world used to initialize state and fetch.
// Table corresponds to archetype. State is the same state used to init fetch above.
unsafe { Q::set_archetype(&mut fetch, &state, archetype, table) }
// SAFETY: Called after set_archetype above. Entity and location are guaranteed to exist.
unsafe { Some(Q::fetch(&mut fetch, self.id(), location.table_row)) }
} else {
None
}
}
}

impl<'w> UnsafeEntityCell<'w> {
Expand Down
2 changes: 1 addition & 1 deletion examples/ecs/dynamic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ fn main() {

query.iter_mut(&mut world).for_each(|filtered_entity| {
let terms = filtered_entity
.components()
.accessed_components()
.map(|id| {
let ptr = filtered_entity.get_by_id(id).unwrap();
let info = component_info.get(&id).unwrap();
Expand Down
Loading