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::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 @@ -261,6 +261,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
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a unit test to validate that these docs stay correct would be nice.

/// 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 @@ -301,6 +304,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 @@ -427,8 +431,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 @@ -1166,4 +1173,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());
}
}
}
75 changes: 67 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 Down Expand Up @@ -262,8 +264,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::<IgnoredSchedulingAmbiguities>()
.cloned()
.unwrap_or_default();
self.graph.update_schedule(
&mut self.executable,
world.components(),
&ignored_ambiguities.0,
&self.name,
)?;
self.graph.changed = false;
self.executor_initialized = false;
}
Expand Down Expand Up @@ -871,6 +881,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 +930,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 +1054,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 +1073,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 +1193,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 +1219,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 +1733,42 @@ impl ScheduleBuildSettings {
}
}

/// List of [`ComponentId`]s to ignore when reporting system order ambiguity conflicts
#[derive(Resource, Default, Clone, Debug)]
pub struct IgnoredSchedulingAmbiguities(BTreeSet<ComponentId>);

impl IgnoredSchedulingAmbiguities {
/// 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.0.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.0.insert(world.components.init_resource::<T>());
}

/// Iterate through the [`ComponentId`]'s that will be ignored.
pub fn iter(&self) -> impl Iterator<Item = &ComponentId> + '_ {
self.0.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_names(&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() {
writeln!(message, "{}", components.get_name(*id).unwrap()).unwrap();
}

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

#[cfg(test)]
mod tests {
use crate::{
Expand Down
Loading