Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
46 changes: 46 additions & 0 deletions crates/bevy_ecs/src/schedule/ambiguity.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
use core::{
fmt::Write as _,
ops::{Deref, DerefMut},
};

use alloc::string::{String, ToString};
use bevy_platform::collections::HashSet;

use crate::{
component::{ComponentId, Components},
resource::Resource,
};

/// List of [`ComponentId`]s to ignore when reporting system order ambiguity conflicts.
#[derive(Resource, Default)]
pub struct IgnoredAmbiguities(pub HashSet<ComponentId>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Each schedule is going to produce it's own ambiguities. Shouldn't this be like HashMap<ScheduleLabel, IndexSet<ComponentId>>? or maybe a component might be better. Since you can store schedules outside of the Schedules resource and this would keep them from overwriting if they share a schedule label.

  • also change it to an IndexSet because we want a stable iteration order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each schedule is going to produce it's own ambiguities. Shouldn't this be like HashMap<ScheduleLabel, IndexSet<ComponentId>>? or maybe a component might be better. Since you can store schedules outside of the Schedules resource and this would keep them from overwriting if they share a schedule label.

ScheduleGraph::ambiguous_with_all and ScheduleGraph::ambiguous_with are the per-Schedule ambiguity stores, this is the global one. I'm just moving the global one outside of the Schedules resource. I can add a flag to Schedule for whether it should use the global storage or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also do a two-tier system where each Schedule also has their own HashSet<ComponentId>, and have a flag to also fetch from the global set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, before and after this PR as-is, all Schedules have to adhere to the global ambiguity set. So adding an opt-out would be a new thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* also change it to an IndexSet because we want a stable iteration order.

The engine itself doesn't iterate the set, and I can't find any examples on GitHub (including bevy_mod_debugdump) of people iterating the set, so I don't think we need to make it an IndexSet.


impl IgnoredAmbiguities {
/// Returns a string listing all ignored ambiguity component names.
///
/// May panic or retrieve incorrect names if [`Components`] is not from the
/// same world.
pub fn to_string(&self, components: &Components) -> String {
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();
}
message
}
}

impl Deref for IgnoredAmbiguities {
type Target = HashSet<ComponentId>;

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl DerefMut for IgnoredAmbiguities {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
16 changes: 6 additions & 10 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Contains APIs for ordering systems and executing them on a [`World`](crate::world::World)

mod ambiguity;
mod auto_insert_apply_deferred;
mod condition;
mod config;
Expand All @@ -12,7 +13,9 @@ mod set;
mod stepping;

pub use self::graph::GraphInfo;
pub use self::{condition::*, config::*, error::*, executor::*, node::*, schedule::*, set::*};
pub use self::{
ambiguity::*, condition::*, config::*, error::*, executor::*, node::*, schedule::*, set::*,
};
pub use pass::ScheduleBuildPass;

/// An implementation of a graph data structure.
Expand Down Expand Up @@ -801,9 +804,6 @@ mod tests {
}

mod system_ambiguity {
#[cfg(feature = "trace")]
use alloc::collections::BTreeSet;

use super::*;
use crate::prelude::*;

Expand Down Expand Up @@ -1145,9 +1145,7 @@ mod tests {
));

schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(&mut world, &BTreeSet::new());
let _ = schedule.graph_mut().build_schedule(&mut world);

let ambiguities: Vec<_> = schedule
.graph()
Expand Down Expand Up @@ -1204,9 +1202,7 @@ mod tests {

let mut world = World::new();
schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(&mut world, &BTreeSet::new());
let _ = schedule.graph_mut().build_schedule(&mut world);

let ambiguities: Vec<_> = schedule
.graph()
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/schedule/node.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use alloc::{boxed::Box, collections::BTreeSet, string::String, vec::Vec};
use alloc::{boxed::Box, string::String, vec::Vec};
use core::{
any::TypeId,
fmt::{self, Debug},
Expand Down Expand Up @@ -609,7 +609,7 @@ impl Systems {
flat_dependency_analysis: &DagAnalysis<SystemKey>,
flat_ambiguous_with: &UnGraph<SystemKey>,
ambiguous_with_all: &HashSet<NodeId>,
ignored_ambiguities: &BTreeSet<ComponentId>,
ignored_ambiguities: &HashSet<ComponentId>,
) -> ConflictingSystems {
let mut conflicting_systems: Vec<(_, _, Box<[_]>)> = Vec::new();
for &(a, b) in flat_dependency_analysis.disconnected() {
Expand Down
75 changes: 23 additions & 52 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
)]
use alloc::{
boxed::Box,
collections::{BTreeMap, BTreeSet},
collections::BTreeMap,
format,
string::{String, ToString},
vec,
Expand All @@ -14,24 +14,17 @@ use bevy_platform::collections::{HashMap, HashSet};
use bevy_utils::{default, TypeIdMap};
use core::{
any::{Any, TypeId},
fmt::{Debug, Write},
fmt::Debug,
};
use fixedbitset::FixedBitSet;
use log::{info, warn};
use log::warn;
use pass::ScheduleBuildPassObj;
use thiserror::Error;
#[cfg(feature = "trace")]
use tracing::info_span;

use crate::{change_detection::CheckChangeTicks, system::System};
use crate::{
component::{ComponentId, Components},
prelude::Component,
resource::Resource,
schedule::*,
system::ScheduleSystem,
world::World,
};
use crate::{change_detection::CheckChangeTicks, component::Component, system::System};
use crate::{resource::Resource, schedule::*, system::ScheduleSystem, world::World};

pub use stepping::Stepping;
use Direction::{Incoming, Outgoing};
Expand All @@ -40,8 +33,6 @@ use Direction::{Incoming, Outgoing};
#[derive(Default, Resource)]
pub struct Schedules {
inner: HashMap<InternedScheduleLabel, Schedule>,
/// List of [`ComponentId`]s to ignore when reporting system order ambiguity conflicts
pub ignored_scheduling_ambiguities: BTreeSet<ComponentId>,
}

impl Schedules {
Expand Down Expand Up @@ -136,35 +127,21 @@ impl Schedules {
}

/// Ignore system order ambiguities caused by conflicts on [`Component`]s of type `T`.
#[deprecated(
since = "0.18.0",
note = "Use `World::allow_ambiguous_component` instead"
)]
pub fn allow_ambiguous_component<T: Component>(&mut self, world: &mut World) {
self.ignored_scheduling_ambiguities
.insert(world.register_component::<T>());
world.allow_ambiguous_component::<T>();
}

/// Ignore system order ambiguities caused by conflicts on [`Resource`]s of type `T`.
#[deprecated(
since = "0.18.0",
note = "Use `World::allow_ambiguous_resource` instead"
)]
pub fn allow_ambiguous_resource<T: Resource>(&mut self, world: &mut World) {
self.ignored_scheduling_ambiguities
.insert(world.components_registrator().register_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}");
world.allow_ambiguous_resource::<T>();
}

/// Adds one or more systems to the [`Schedule`] matching the provided [`ScheduleLabel`].
Expand Down Expand Up @@ -562,16 +539,9 @@ impl Schedule {
pub fn initialize(&mut self, world: &mut World) -> Result<(), ScheduleBuildError> {
if self.graph.changed {
self.graph.initialize(world);
let ignored_ambiguities = world
.get_resource_or_init::<Schedules>()
.ignored_scheduling_ambiguities
.clone();
self.warnings = self.graph.update_schedule(
world,
&mut self.executable,
&ignored_ambiguities,
self.label,
)?;
self.warnings = self
.graph
.update_schedule(world, &mut self.executable, self.label)?;
self.graph.changed = false;
self.executor_initialized = false;
}
Expand Down Expand Up @@ -1105,7 +1075,6 @@ impl ScheduleGraph {
pub fn build_schedule(
&mut self,
world: &mut World,
ignored_ambiguities: &BTreeSet<ComponentId>,
) -> Result<(SystemSchedule, Vec<ScheduleBuildWarning>), ScheduleBuildError> {
let mut warnings = Vec::new();

Expand Down Expand Up @@ -1189,7 +1158,10 @@ impl ScheduleGraph {
&flat_dependency_analysis,
&flat_ambiguous_with,
&self.ambiguous_with_all,
ignored_ambiguities,
world
.get_resource::<IgnoredAmbiguities>()
.map(|ia| &ia.0)
.unwrap_or(&HashSet::new()),
);
// If there are any ambiguities, log warnings or return errors as configured.
if self.settings.ambiguity_detection != LogLevel::Ignore
Expand Down Expand Up @@ -1309,7 +1281,6 @@ impl ScheduleGraph {
&mut self,
world: &mut World,
schedule: &mut SystemSchedule,
ignored_ambiguities: &BTreeSet<ComponentId>,
schedule_label: InternedScheduleLabel,
) -> Result<Vec<ScheduleBuildWarning>, ScheduleBuildError> {
if !self.systems.is_initialized() || !self.system_sets.is_initialized() {
Expand Down Expand Up @@ -1342,7 +1313,7 @@ impl ScheduleGraph {
}
}

let (new_schedule, warnings) = self.build_schedule(world, ignored_ambiguities)?;
let (new_schedule, warnings) = self.build_schedule(world)?;
*schedule = new_schedule;

for warning in &warnings {
Expand Down
12 changes: 5 additions & 7 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ use crate::{
query::{DebugCheckedUnwrap, QueryData, QueryFilter, QueryState},
relationship::RelationshipHookMode,
resource::Resource,
schedule::{Schedule, ScheduleLabel, Schedules},
schedule::{IgnoredAmbiguities, Schedule, ScheduleLabel, Schedules},
storage::{ResourceData, Storages},
system::Commands,
world::{
Expand Down Expand Up @@ -3675,16 +3675,14 @@ impl World {

/// Ignore system order ambiguities caused by conflicts on [`Component`]s of type `T`.
pub fn allow_ambiguous_component<T: Component>(&mut self) {
let mut schedules = self.remove_resource::<Schedules>().unwrap_or_default();
schedules.allow_ambiguous_component::<T>(self);
self.insert_resource(schedules);
let id = self.register_component::<T>();
self.get_resource_or_init::<IgnoredAmbiguities>().insert(id);
}

/// Ignore system order ambiguities caused by conflicts on [`Resource`]s of type `T`.
pub fn allow_ambiguous_resource<T: Resource>(&mut self) {
let mut schedules = self.remove_resource::<Schedules>().unwrap_or_default();
schedules.allow_ambiguous_resource::<T>(self);
self.insert_resource(schedules);
let id = self.components_registrator().register_resource::<T>();
self.get_resource_or_init::<IgnoredAmbiguities>().insert(id);
}
}

Expand Down
5 changes: 5 additions & 0 deletions release-content/migration-guides/schedule_cleanup.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,8 @@ pull_requests: [21608, 21817]
- `ScheduleBuildPass::collapse_set` now takes `&HashSet<SystemKey>` instead of a slice, to reduce redundant allocations.
- `simple_cycles_in_component` has been changed from a free function into a method on `DiGraph`.
- `DiGraph::try_into`/`UnGraph::try_into` was renamed to `DiGraph::try_convert`/`UnGraph::try_convert` to prevent overlap with the `TryInto` trait, and now makes use of `TryInto` instead of `TryFrom` for conversions.
- Removed `Schedules::ignored_scheduling_ambiguities`: access the same info via the new `IgnoredAmbiguities` resource instead.
- Deprecated `Schedules::allow_ambiguous_component` and `Schedules::allow_ambiguous_resource`: use the equivalent functions on `World` instead.
- Removed `Schedules::iter_ignored_ambiguities`: use `IgnoredAmbiguities::iter` via Deref instead.
- Removed `Schedules::print_ignored_ambiguities`: print the string returned by `IgnoredAmbiguities::to_string` yourself.
- Removed the `ignored_ambiguities` parameter from `Schedule::build_schedule`, it is now fetched internally from the new `IgnoredAmbiguities` resource.