Skip to content

Commit

Permalink
Ignore ambiguous components or resources (#9895)
Browse files Browse the repository at this point in the history
# Objective

- Fixes #9884
- Add API for ignoring ambiguities on certain resource or components.

## Solution

- Add a `IgnoreSchedulingAmbiguitiy` resource to the world which holds
the `ComponentIds` to be ignored
- Filter out ambiguities with those component id's.

## Changelog

- add `allow_ambiguous_component` and `allow_ambiguous_resource` apis
for ignoring ambiguities

---------

Co-authored-by: Ryan Johnson <[email protected]>
  • Loading branch information
hymm and rj00a authored Oct 4, 2023
1 parent 375af64 commit 7c5b324
Show file tree
Hide file tree
Showing 5 changed files with 219 additions and 25 deletions.
77 changes: 77 additions & 0 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,83 @@ impl App {
.configure_schedules(schedule_build_settings);
self
}

/// When doing [ambiguity checking](bevy_ecs::schedule::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::schedule::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 R
/// 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
28 changes: 27 additions & 1 deletion crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ pub trait AssetApp {
/// * Registering the [`Asset`] in the [`AssetServer`]
/// * Initializing the [`AssetEvent`] resource for the [`Asset`]
/// * Adding other relevant systems and resources for the [`Asset`]
/// * Ignoring schedule ambiguities in [`Assets`] resource. Any time a system takes
/// mutable access to this resource this causes a conflict, but they rarely actually
/// modify the same underlying asset.
fn init_asset<A: Asset>(&mut self) -> &mut Self;
/// Registers the asset type `T` using `[App::register]`,
/// and adds [`ReflectAsset`] type data to `T` and [`ReflectHandle`] type data to [`Handle<T>`] in the type registry.
Expand Down Expand Up @@ -303,6 +306,7 @@ impl AssetApp for App {
));
}
self.insert_resource(assets)
.allow_ambiguous_resource::<Assets<A>>()
.add_event::<AssetEvent<A>>()
.register_type::<Handle<A>>()
.register_type::<AssetId<A>>()
Expand Down Expand Up @@ -429,8 +433,11 @@ mod tests {
};
use bevy_app::{App, Update};
use bevy_core::TaskPoolPlugin;
use bevy_ecs::event::ManualEventReader;
use bevy_ecs::prelude::*;
use bevy_ecs::{
event::ManualEventReader,
schedule::{LogLevel, ScheduleBuildSettings},
};
use bevy_log::LogPlugin;
use bevy_reflect::TypePath;
use bevy_utils::BoxedFuture;
Expand Down Expand Up @@ -1168,4 +1175,23 @@ mod tests {
None
});
}

#[test]
fn ignore_system_ambiguities_on_assets() {
let mut app = App::new();
app.add_plugins(AssetPlugin::default())
.init_asset::<CoolText>();

fn uses_assets(_asset: ResMut<Assets<CoolText>>) {}
app.add_systems(Update, (uses_assets, uses_assets));
app.edit_schedule(Update, |s| {
s.set_build_settings(ScheduleBuildSettings {
ambiguity_detection: LogLevel::Error,
..Default::default()
});
});

// running schedule does not error on ambiguity between the 2 uses_assets systems
app.world.run_schedule(Update);
}
}
51 changes: 35 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,9 @@ mod tests {
}

mod system_ambiguity {
use std::collections::BTreeSet;

use super::*;
// Required to make the derive macro behave
use crate as bevy_ecs;
use crate::event::Events;
Expand Down Expand Up @@ -981,14 +984,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 +1009,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(),
&BTreeSet::new(),
);

let ambiguities: Vec<_> = schedule
.graph()
Expand Down Expand Up @@ -1050,19 +1053,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(),
&BTreeSet::new(),
);

let ambiguities: Vec<_> = schedule
.graph()
Expand All @@ -1078,5 +1078,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());
}
}
}
74 changes: 66 additions & 8 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use std::{
collections::BTreeSet,
fmt::{Debug, Write},
result::Result,
};

use bevy_utils::default;
#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;
use bevy_utils::{default, tracing::info};
use bevy_utils::{
petgraph::{algo::TarjanScc, prelude::*},
thiserror::Error,
Expand All @@ -18,6 +19,7 @@ use fixedbitset::FixedBitSet;
use crate::{
self as bevy_ecs,
component::{ComponentId, Components, Tick},
prelude::Component,
schedule::*,
system::{BoxedSystem, Resource, System},
world::World,
Expand All @@ -27,13 +29,16 @@ use crate::{
#[derive(Default, Resource)]
pub struct Schedules {
inner: HashMap<BoxedScheduleLabel, Schedule>,
/// List of [`ComponentId`]s to ignore when reporting system order ambiguity conflicts
pub ignored_scheduling_ambiguities: BTreeSet<ComponentId>,
}

impl Schedules {
/// Constructs an empty `Schedules` with zero initial capacity.
pub fn new() -> Self {
Self {
inner: HashMap::new(),
ignored_scheduling_ambiguities: BTreeSet::new(),
}
}

Expand Down Expand Up @@ -110,6 +115,38 @@ impl Schedules {
schedule.set_build_settings(schedule_build_settings.clone());
}
}

/// Ignore system order ambiguities caused by conflicts on [`Component`]s of type `T`.
pub fn allow_ambiguous_component<T: Component>(&mut self, world: &mut World) {
self.ignored_scheduling_ambiguities
.insert(world.init_component::<T>());
}

/// Ignore system order ambiguities caused by conflicts on [`Resource`]s of type `T`.
pub fn allow_ambiguous_resource<T: Resource>(&mut self, world: &mut World) {
self.ignored_scheduling_ambiguities
.insert(world.components.init_resource::<T>());
}

/// Iterate through the [`ComponentId`]'s that will be ignored.
pub fn iter_ignored_ambiguities(&self) -> impl Iterator<Item = &ComponentId> + '_ {
self.ignored_scheduling_ambiguities.iter()
}

/// Prints the names of the components and resources with [`info`]
///
/// May panic or retrieve incorrect names if [`Components`] is not from the same
/// world
pub fn print_ignored_ambiguities(&self, components: &Components) {
let mut message =
"System order ambiguities caused by conflicts on the following types are ignored:\n"
.to_string();
for id in self.iter_ignored_ambiguities() {
writeln!(message, "{}", components.get_name(*id).unwrap()).unwrap();
}

info!("{}", message);
}
}

fn make_executor(kind: ExecutorKind) -> Box<dyn SystemExecutor> {
Expand Down Expand Up @@ -262,8 +299,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 ignored_ambiguities = world
.get_resource_or_insert_with::<Schedules>(Schedules::default)
.ignored_scheduling_ambiguities
.clone();
self.graph.update_schedule(
&mut self.executable,
world.components(),
&ignored_ambiguities,
&self.name,
)?;
self.graph.changed = false;
self.executor_initialized = false;
}
Expand Down Expand Up @@ -871,6 +916,7 @@ impl ScheduleGraph {
&mut self,
components: &Components,
schedule_label: &BoxedScheduleLabel,
ignored_ambiguities: &BTreeSet<ComponentId>,
) -> Result<SystemSchedule, ScheduleBuildError> {
// check hierarchy for cycles
self.hierarchy.topsort =
Expand Down Expand Up @@ -919,8 +965,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 +1089,7 @@ impl ScheduleGraph {
&self,
flat_results_disconnected: &Vec<(NodeId, NodeId)>,
ambiguous_with_flattened: &GraphMap<NodeId, (), Undirected>,
ignored_ambiguities: &BTreeSet<ComponentId>,
) -> Vec<(NodeId, NodeId, Vec<ComponentId>)> {
let mut conflicting_systems = Vec::new();
for &(a, b) in flat_results_disconnected {
Expand All @@ -1058,8 +1108,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 +1228,7 @@ impl ScheduleGraph {
&mut self,
schedule: &mut SystemSchedule,
components: &Components,
ignored_ambiguities: &BTreeSet<ComponentId>,
schedule_label: &BoxedScheduleLabel,
) -> Result<(), ScheduleBuildError> {
if !self.uninit.is_empty() {
Expand All @@ -1196,7 +1254,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
Loading

0 comments on commit 7c5b324

Please sign in to comment.