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

Ignore ambiguous components or resources #9895

Merged
merged 14 commits into from
Oct 4, 2023
77 changes: 77 additions & 0 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,83 @@ impl App {
.configure_schedules(schedule_build_settings);
self
}

/// When doing [ambiguity checking](bevy_ecs::scheduling::ScheduleBuildSettings) this
/// ignores systems that are ambiguious on [`Component`] T.
///
/// This settings only applies to the main world. To apply this to other worlds call the
/// [corresponding method](World::allow_ambiguous_component) on World
///
/// ## Example
///
/// ```rust
/// # use bevy_app::prelude::*;
/// # use bevy_ecs::prelude::*;
/// # use bevy_ecs::schedule::{LogLevel, ScheduleBuildSettings};
/// # use bevy_utils::default;
///
/// #[derive(Component)]
/// struct A;
///
/// // these systems are ambiguous on A
/// fn system_1(_: Query<&mut A>) {}
/// fn system_2(_: Query<&A>) {}
///
/// let mut app = App::new();
/// app.configure_schedules(ScheduleBuildSettings {
/// ambiguity_detection: LogLevel::Error,
/// ..default()
/// });
///
/// app.add_systems(Update, ( system_1, system_2 ));
/// app.allow_ambiguous_component::<A>();
///
/// // running the app does not error.
/// app.update();
/// ```
pub fn allow_ambiguous_component<T: Component>(&mut self) -> &mut Self {
self.world.allow_ambiguous_component::<T>();
self
}

/// When doing [ambiguity checking](bevy_ecs::scheduling::ScheduleBuildSettings) this
/// ignores systems that are ambiguious on [`Resource`] T.
///
/// This settings only applies to the main world. To apply this to other worlds call the
/// [corresponding method](World::allow_ambiguous_resource) on World
///
/// ## Example
///
/// ```rust
/// # use bevy_app::prelude::*;
/// # use bevy_ecs::prelude::*;
/// # use bevy_ecs::schedule::{LogLevel, ScheduleBuildSettings};
/// # use bevy_utils::default;
///
/// #[derive(Resource)]
/// struct R;
///
/// // these systems are ambiguous on A
hymm marked this conversation as resolved.
Show resolved Hide resolved
/// fn system_1(_: ResMut<R>) {}
/// fn system_2(_: Res<R>) {}
///
/// let mut app = App::new();
/// app.configure_schedules(ScheduleBuildSettings {
/// ambiguity_detection: LogLevel::Error,
/// ..default()
/// });
/// app.insert_resource(R);
///
/// app.add_systems(Update, ( system_1, system_2 ));
/// app.allow_ambiguous_resource::<R>();
///
/// // running the app does not error.
/// app.update();
/// ```
pub fn allow_ambiguous_resource<T: Resource>(&mut self) -> &mut Self {
self.world.allow_ambiguous_resource::<T>();
self
}
}

fn run_once(mut app: App) {
Expand Down
49 changes: 33 additions & 16 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,7 @@ mod tests {
}

mod system_ambiguity {
use super::*;
// Required to make the derive macro behave
use crate as bevy_ecs;
use crate::event::Events;
Expand Down Expand Up @@ -981,14 +982,12 @@ mod tests {
assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
struct TestSchedule;

// Tests that the correct ambiguities were reported in the correct order.
#[test]
fn correct_ambiguities() {
use super::*;

#[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
struct TestSchedule;

fn system_a(_res: ResMut<R>) {}
fn system_b(_res: ResMut<R>) {}
fn system_c(_res: ResMut<R>) {}
Expand All @@ -1008,9 +1007,11 @@ mod tests {
));

schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(world.components(), &TestSchedule.dyn_clone());
let _ = schedule.graph_mut().build_schedule(
world.components(),
&TestSchedule.dyn_clone(),
&[],
);

let ambiguities: Vec<_> = schedule
.graph()
Expand Down Expand Up @@ -1050,19 +1051,16 @@ mod tests {
// Related issue https://github.com/bevyengine/bevy/issues/9641
#[test]
fn anonymous_set_name() {
use super::*;

#[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
struct TestSchedule;

let mut schedule = Schedule::new(TestSchedule);
schedule.add_systems((resmut_system, resmut_system).run_if(|| true));

let mut world = World::new();
schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(world.components(), &TestSchedule.dyn_clone());
let _ = schedule.graph_mut().build_schedule(
world.components(),
&TestSchedule.dyn_clone(),
&[],
);

let ambiguities: Vec<_> = schedule
.graph()
Expand All @@ -1078,5 +1076,24 @@ mod tests {
)
);
}

#[test]
fn ignore_component_resource_ambiguities() {
let mut world = World::new();
world.insert_resource(R);
world.allow_ambiguous_resource::<R>();
let mut schedule = Schedule::new(TestSchedule);

//check resource
schedule.add_systems((resmut_system, res_system));
schedule.initialize(&mut world).unwrap();
assert!(schedule.graph().conflicting_systems().is_empty());

// check components
world.allow_ambiguous_component::<A>();
schedule.add_systems((write_component_system, read_component_system));
schedule.initialize(&mut world).unwrap();
assert!(schedule.graph().conflicting_systems().is_empty());
}
}
}
52 changes: 45 additions & 7 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use fixedbitset::FixedBitSet;
use crate::{
self as bevy_ecs,
component::{ComponentId, Components, Tick},
prelude::Component,
schedule::*,
system::{BoxedSystem, Resource, System},
world::World,
Expand Down Expand Up @@ -262,8 +263,16 @@ impl Schedule {
pub fn initialize(&mut self, world: &mut World) -> Result<(), ScheduleBuildError> {
if self.graph.changed {
self.graph.initialize(world);
self.graph
.update_schedule(&mut self.executable, world.components(), &self.name)?;
let ignore_ambiguities = world
hymm marked this conversation as resolved.
Show resolved Hide resolved
.get_resource::<IgnoreSchedulingAmbiguities>()
.cloned()
.unwrap_or_default();
self.graph.update_schedule(
&mut self.executable,
world.components(),
&ignore_ambiguities.0,
&self.name,
)?;
self.graph.changed = false;
self.executor_initialized = false;
}
Expand Down Expand Up @@ -871,6 +880,7 @@ impl ScheduleGraph {
&mut self,
components: &Components,
schedule_label: &BoxedScheduleLabel,
ignored_ambiguities: &[ComponentId],
) -> Result<SystemSchedule, ScheduleBuildError> {
// check hierarchy for cycles
self.hierarchy.topsort =
Expand Down Expand Up @@ -919,8 +929,11 @@ impl ScheduleGraph {
let ambiguous_with_flattened = self.get_ambiguous_with_flattened(&set_systems);

// check for conflicts
let conflicting_systems =
self.get_conflicting_systems(&flat_results.disconnected, &ambiguous_with_flattened);
let conflicting_systems = self.get_conflicting_systems(
&flat_results.disconnected,
&ambiguous_with_flattened,
ignored_ambiguities,
);
self.optionally_check_conflicts(&conflicting_systems, components, schedule_label)?;
self.conflicting_systems = conflicting_systems;

Expand Down Expand Up @@ -1040,6 +1053,7 @@ impl ScheduleGraph {
&self,
flat_results_disconnected: &Vec<(NodeId, NodeId)>,
ambiguous_with_flattened: &GraphMap<NodeId, (), Undirected>,
ignored_ambiguities: &[ComponentId],
) -> Vec<(NodeId, NodeId, Vec<ComponentId>)> {
let mut conflicting_systems = Vec::new();
for &(a, b) in flat_results_disconnected {
Expand All @@ -1058,8 +1072,15 @@ impl ScheduleGraph {
let access_a = system_a.component_access();
let access_b = system_b.component_access();
if !access_a.is_compatible(access_b) {
let conflicts = access_a.get_conflicts(access_b);
conflicting_systems.push((a, b, conflicts));
let conflicts: Vec<_> = access_a
.get_conflicts(access_b)
.into_iter()
.filter(|id| !ignored_ambiguities.contains(id))
.collect();

if !conflicts.is_empty() {
conflicting_systems.push((a, b, conflicts));
}
}
}
}
Expand Down Expand Up @@ -1171,6 +1192,7 @@ impl ScheduleGraph {
&mut self,
schedule: &mut SystemSchedule,
components: &Components,
ignored_ambiguities: &[ComponentId],
schedule_label: &BoxedScheduleLabel,
) -> Result<(), ScheduleBuildError> {
if !self.uninit.is_empty() {
Expand All @@ -1196,7 +1218,7 @@ impl ScheduleGraph {
self.system_set_conditions[id.index()] = conditions;
}

*schedule = self.build_schedule(components, schedule_label)?;
*schedule = self.build_schedule(components, schedule_label, ignored_ambiguities)?;

// move systems into new schedule
for &id in &schedule.system_ids {
Expand Down Expand Up @@ -1710,6 +1732,22 @@ impl ScheduleBuildSettings {
}
}

/// list of [`ComponentId`]'s to ignore when reporting ambiguity conflicts between systems
#[derive(Resource, Default, Clone)]
hymm marked this conversation as resolved.
Show resolved Hide resolved
pub struct IgnoreSchedulingAmbiguities(Vec<ComponentId>);
hymm marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This should have two distinct fields: one for resources, and one for components. Otherwise the API below doesn't make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between one or two fields can't be observed in the API, so this seems alright to me. Only reason I could think to split it would be for a nicer Debug impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of splitting it up, makes the logic for filtering more complicated.

hymm marked this conversation as resolved.
Show resolved Hide resolved

impl IgnoreSchedulingAmbiguities {
/// Ignore ambiguities between systems in [`Component`] T.
hymm marked this conversation as resolved.
Show resolved Hide resolved
pub fn allow_ambiguous_component<T: Component>(&mut self, world: &mut World) {
self.0.push(world.init_component::<T>());
}

/// Ignore ambiguities between systems in [`Resource`] T.
hymm marked this conversation as resolved.
Show resolved Hide resolved
pub fn allow_ambiguous_resource<T: Resource>(&mut self, world: &mut World) {
self.0.push(world.components.init_resource::<T>());
}
}

#[cfg(test)]
mod tests {
use crate::{
Expand Down
20 changes: 19 additions & 1 deletion crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
event::{Event, Events},
query::{DebugCheckedUnwrap, QueryEntityError, QueryState, ReadOnlyWorldQuery, WorldQuery},
removal_detection::RemovedComponentEvents,
schedule::{Schedule, ScheduleLabel, Schedules},
schedule::{IgnoreSchedulingAmbiguities, Schedule, ScheduleLabel, Schedules},
storage::{ResourceData, Storages},
system::Resource,
world::error::TryRunScheduleError,
Expand Down Expand Up @@ -2087,6 +2087,24 @@ impl World {
pub fn run_schedule(&mut self, label: impl AsRef<dyn ScheduleLabel>) {
self.schedule_scope(label, |world, sched| sched.run(world));
}

/// Ignore ambiguities between systems in [`Component`] T.
pub fn allow_ambiguous_component<T: Component>(&mut self) {
let mut ignore_ambiguities = self
.remove_resource::<IgnoreSchedulingAmbiguities>()
.unwrap_or_default();
ignore_ambiguities.allow_ambiguous_component::<T>(self);
self.insert_resource(ignore_ambiguities);
}

/// Ignore ambiguities between systems in [`Resource`] T.
pub fn allow_ambiguous_resource<T: Resource>(&mut self) {
let mut ignore_ambiguities = self
.remove_resource::<IgnoreSchedulingAmbiguities>()
.unwrap_or_default();
ignore_ambiguities.allow_ambiguous_resource::<T>(self);
self.insert_resource(ignore_ambiguities);
}
}

impl fmt::Debug for World {
Expand Down
Loading