Skip to content

Commit

Permalink
Separate component and resource access (#14561)
Browse files Browse the repository at this point in the history
# Objective

- Fixes #13139
- Fixes #7255
- Separates component from resource access so that we can correctly
handles edge cases like the issue above
- Inspired from #14472

## Solution

- Update access to have `component` fields and `resource` fields

## Testing

- Added some unit tests
  • Loading branch information
cBournhonesque authored Aug 6, 2024
1 parent 0d0f77a commit 3a664b0
Show file tree
Hide file tree
Showing 11 changed files with 580 additions and 232 deletions.
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1420,8 +1420,8 @@ mod tests {
let mut expected = FilteredAccess::<ComponentId>::default();
let a_id = world.components.get_id(TypeId::of::<A>()).unwrap();
let b_id = world.components.get_id(TypeId::of::<B>()).unwrap();
expected.add_write(a_id);
expected.add_read(b_id);
expected.add_component_write(a_id);
expected.add_component_read(b_id);
assert!(
query.component_access.eq(&expected),
"ComponentId access from query fetch and query filter should be combined"
Expand Down
580 changes: 424 additions & 156 deletions crates/bevy_ecs/src/query/access.rs

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/query/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,14 @@ impl<'w, D: QueryData, F: QueryFilter> QueryBuilder<'w, D, F> {
/// Adds `&T` to the [`FilteredAccess`] of self.
pub fn ref_id(&mut self, id: ComponentId) -> &mut Self {
self.with_id(id);
self.access.add_read(id);
self.access.add_component_read(id);
self
}

/// Adds `&mut T` to the [`FilteredAccess`] of self.
pub fn mut_id(&mut self, id: ComponentId) -> &mut Self {
self.with_id(id);
self.access.add_write(id);
self.access.add_component_write(id);
self
}

Expand Down
60 changes: 30 additions & 30 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,10 @@ unsafe impl<'a> WorldQuery for EntityRef<'a> {

fn update_component_access(_state: &Self::State, access: &mut FilteredAccess<ComponentId>) {
assert!(
!access.access().has_any_write(),
!access.access().has_any_component_write(),
"EntityRef conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
);
access.read_all();
access.read_all_components();
}

fn init_state(_world: &mut World) {}
Expand Down Expand Up @@ -556,10 +556,10 @@ unsafe impl<'a> WorldQuery for EntityMut<'a> {

fn update_component_access(_state: &Self::State, access: &mut FilteredAccess<ComponentId>) {
assert!(
!access.access().has_any_read(),
!access.access().has_any_component_read(),
"EntityMut conflicts with a previous access in this query. Exclusive access cannot coincide with any other accesses.",
);
access.write_all();
access.write_all_components();
}

fn init_state(_world: &mut World) {}
Expand Down Expand Up @@ -600,7 +600,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> {
_this_run: Tick,
) -> Self::Fetch<'w> {
let mut access = Access::default();
access.read_all();
access.read_all_components();
(world, access)
}

Expand All @@ -612,9 +612,9 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> {
_table: &Table,
) {
let mut access = Access::default();
state.access.reads().for_each(|id| {
state.access.component_reads().for_each(|id| {
if archetype.contains(id) {
access.add_read(id);
access.add_component_read(id);
}
});
fetch.1 = access;
Expand All @@ -623,9 +623,9 @@ unsafe impl<'a> WorldQuery for FilteredEntityRef<'a> {
#[inline]
unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) {
let mut access = Access::default();
state.access.reads().for_each(|id| {
state.access.component_reads().for_each(|id| {
if table.has_column(id) {
access.add_read(id);
access.add_component_read(id);
}
});
fetch.1 = access;
Expand Down Expand Up @@ -703,7 +703,7 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> {
_this_run: Tick,
) -> Self::Fetch<'w> {
let mut access = Access::default();
access.write_all();
access.write_all_components();
(world, access)
}

Expand All @@ -715,14 +715,14 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> {
_table: &Table,
) {
let mut access = Access::default();
state.access.reads().for_each(|id| {
state.access.component_reads().for_each(|id| {
if archetype.contains(id) {
access.add_read(id);
access.add_component_read(id);
}
});
state.access.writes().for_each(|id| {
state.access.component_writes().for_each(|id| {
if archetype.contains(id) {
access.add_write(id);
access.add_component_write(id);
}
});
fetch.1 = access;
Expand All @@ -731,14 +731,14 @@ unsafe impl<'a> WorldQuery for FilteredEntityMut<'a> {
#[inline]
unsafe fn set_table<'w>(fetch: &mut Self::Fetch<'w>, state: &Self::State, table: &'w Table) {
let mut access = Access::default();
state.access.reads().for_each(|id| {
state.access.component_reads().for_each(|id| {
if table.has_column(id) {
access.add_read(id);
access.add_component_read(id);
}
});
state.access.writes().for_each(|id| {
state.access.component_writes().for_each(|id| {
if table.has_column(id) {
access.add_write(id);
access.add_component_write(id);
}
});
fetch.1 = access;
Expand Down Expand Up @@ -988,11 +988,11 @@ unsafe impl<T: Component> WorldQuery for &T {
access: &mut FilteredAccess<ComponentId>,
) {
assert!(
!access.access().has_write(component_id),
!access.access().has_component_write(component_id),
"&{} conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
std::any::type_name::<T>(),
std::any::type_name::<T>(),
);
access.add_read(component_id);
access.add_component_read(component_id);
}

fn init_state(world: &mut World) -> ComponentId {
Expand Down Expand Up @@ -1183,11 +1183,11 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {
access: &mut FilteredAccess<ComponentId>,
) {
assert!(
!access.access().has_write(component_id),
!access.access().has_component_write(component_id),
"&{} conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",
std::any::type_name::<T>(),
std::any::type_name::<T>(),
);
access.add_read(component_id);
access.add_component_read(component_id);
}

fn init_state(world: &mut World) -> ComponentId {
Expand Down Expand Up @@ -1378,11 +1378,11 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
access: &mut FilteredAccess<ComponentId>,
) {
assert!(
!access.access().has_read(component_id),
!access.access().has_component_read(component_id),
"&mut {} conflicts with a previous access in this query. Mutable component access must be unique.",
std::any::type_name::<T>(),
std::any::type_name::<T>(),
);
access.add_write(component_id);
access.add_component_write(component_id);
}

fn init_state(world: &mut World) -> ComponentId {
Expand Down Expand Up @@ -1476,11 +1476,11 @@ unsafe impl<'__w, T: Component> WorldQuery for Mut<'__w, T> {
// Update component access here instead of in `<&mut T as WorldQuery>` to avoid erroneously referencing
// `&mut T` in error message.
assert!(
!access.access().has_read(component_id),
!access.access().has_component_read(component_id),
"Mut<{}> conflicts with a previous access in this query. Mutable component access mut be unique.",
std::any::type_name::<T>(),
std::any::type_name::<T>(),
);
access.add_write(component_id);
access.add_component_write(component_id);
}

// Forwarded to `&mut T`
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/query/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -688,10 +688,10 @@ unsafe impl<T: Component> WorldQuery for Added<T> {

#[inline]
fn update_component_access(&id: &ComponentId, access: &mut FilteredAccess<ComponentId>) {
if access.access().has_write(id) {
if access.access().has_component_write(id) {
panic!("$state_name<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",std::any::type_name::<T>());
}
access.add_read(id);
access.add_component_read(id);
}

fn init_state(world: &mut World) -> ComponentId {
Expand Down Expand Up @@ -899,10 +899,10 @@ unsafe impl<T: Component> WorldQuery for Changed<T> {

#[inline]
fn update_component_access(&id: &ComponentId, access: &mut FilteredAccess<ComponentId>) {
if access.access().has_write(id) {
if access.access().has_component_write(id) {
panic!("$state_name<{}> conflicts with a previous access in this query. Shared access cannot coincide with exclusive access.",std::any::type_name::<T>());
}
access.add_read(id);
access.add_component_read(id);
}

fn init_state(world: &mut World) -> ComponentId {
Expand Down
26 changes: 16 additions & 10 deletions crates/bevy_ecs/src/query/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,16 +495,22 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
archetype: &Archetype,
access: &mut Access<ArchetypeComponentId>,
) {
self.component_access.access.reads().for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_read(id);
}
});
self.component_access.access.writes().for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_write(id);
}
});
self.component_access
.access
.component_reads()
.for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_component_read(id);
}
});
self.component_access
.access
.component_writes()
.for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_component_write(id);
}
});
}

/// Use this to transform a [`QueryState`] into a more generic [`QueryState`].
Expand Down
74 changes: 74 additions & 0 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,9 @@ mod tests {
#[derive(Event)]
struct E;

#[derive(Resource, Component)]
struct RC;

fn empty_system() {}
fn res_system(_res: Res<R>) {}
fn resmut_system(_res: ResMut<R>) {}
Expand All @@ -746,6 +749,8 @@ mod tests {
fn write_component_system(_query: Query<&mut A>) {}
fn with_filtered_component_system(_query: Query<&mut A, With<B>>) {}
fn without_filtered_component_system(_query: Query<&mut A, Without<B>>) {}
fn entity_ref_system(_query: Query<EntityRef>) {}
fn entity_mut_system(_query: Query<EntityMut>) {}
fn event_reader_system(_reader: EventReader<E>) {}
fn event_writer_system(_writer: EventWriter<E>) {}
fn event_resource_system(_events: ResMut<Events<E>>) {}
Expand Down Expand Up @@ -788,6 +793,8 @@ mod tests {
nonsend_system,
read_component_system,
read_component_system,
entity_ref_system,
entity_ref_system,
event_reader_system,
event_reader_system,
read_world_system,
Expand Down Expand Up @@ -893,6 +900,73 @@ mod tests {
assert_eq!(schedule.graph().conflicting_systems().len(), 3);
}

/// Test that when a struct is both a Resource and a Component, they do not
/// conflict with each other.
#[test]
fn shared_resource_mut_component() {
let mut world = World::new();
world.insert_resource(RC);

let mut schedule = Schedule::default();
schedule.add_systems((|_: ResMut<RC>| {}, |_: Query<&mut RC>| {}));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn resource_mut_and_entity_ref() {
let mut world = World::new();
world.insert_resource(R);

let mut schedule = Schedule::default();
schedule.add_systems((resmut_system, entity_ref_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn resource_and_entity_mut() {
let mut world = World::new();
world.insert_resource(R);

let mut schedule = Schedule::default();
schedule.add_systems((res_system, nonsend_system, entity_mut_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn write_component_and_entity_ref() {
let mut world = World::new();
world.insert_resource(R);

let mut schedule = Schedule::default();
schedule.add_systems((write_component_system, entity_ref_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 1);
}

#[test]
fn read_component_and_entity_mut() {
let mut world = World::new();
world.insert_resource(R);

let mut schedule = Schedule::default();
schedule.add_systems((read_component_system, entity_mut_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 1);
}

#[test]
fn exclusive() {
let mut world = World::new();
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,7 @@ mod tests {
assert_eq!(
system
.archetype_component_access()
.reads()
.component_reads()
.collect::<HashSet<_>>(),
expected_ids
);
Expand Down Expand Up @@ -1525,7 +1525,7 @@ mod tests {
assert_eq!(
system
.archetype_component_access()
.reads()
.component_reads()
.collect::<HashSet<_>>(),
expected_ids
);
Expand All @@ -1543,7 +1543,7 @@ mod tests {
assert_eq!(
system
.archetype_component_access()
.reads()
.component_reads()
.collect::<HashSet<_>>(),
expected_ids
);
Expand Down
Loading

0 comments on commit 3a664b0

Please sign in to comment.