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

[Merged by Bors] - Resolve (most) internal system ambiguities #1606

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
30 changes: 14 additions & 16 deletions crates/bevy_app/src/app_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,29 +211,27 @@ impl AppBuilder {
}

pub fn add_default_stages(&mut self) -> &mut Self {
self.add_stage(
CoreStage::Startup,
Schedule::default()
.with_run_criteria(RunOnce::default())
.with_stage(StartupStage::PreStartup, SystemStage::parallel())
.with_stage(StartupStage::Startup, SystemStage::parallel())
.with_stage(StartupStage::PostStartup, SystemStage::parallel()),
)
.add_stage(CoreStage::First, SystemStage::parallel())
.add_stage(CoreStage::PreEvent, SystemStage::parallel())
.add_stage(CoreStage::Event, SystemStage::parallel())
.add_stage(CoreStage::PreUpdate, SystemStage::parallel())
.add_stage(CoreStage::Update, SystemStage::parallel())
.add_stage(CoreStage::PostUpdate, SystemStage::parallel())
.add_stage(CoreStage::Last, SystemStage::parallel())
self.add_stage(CoreStage::First, SystemStage::parallel())
.add_stage(
CoreStage::Startup,
Schedule::default()
.with_run_criteria(RunOnce::default())
.with_stage(StartupStage::PreStartup, SystemStage::parallel())
.with_stage(StartupStage::Startup, SystemStage::parallel())
.with_stage(StartupStage::PostStartup, SystemStage::parallel()),
)
.add_stage(CoreStage::PreUpdate, SystemStage::parallel())
.add_stage(CoreStage::Update, SystemStage::parallel())
.add_stage(CoreStage::PostUpdate, SystemStage::parallel())
.add_stage(CoreStage::Last, SystemStage::parallel())
}

pub fn add_event<T>(&mut self) -> &mut Self
where
T: Component,
{
self.insert_resource(Events::<T>::default())
.add_system_to_stage(CoreStage::Event, Events::<T>::update_system.system())
.add_system_to_stage(CoreStage::First, Events::<T>::update_system.system())
}

/// Inserts a resource to the current [App] and overwrites any resource previously added of the same type.
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_app/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ use bevy_ecs::{
system::{Local, Res, ResMut, SystemParam},
};
use bevy_utils::tracing::trace;
use std::{fmt, marker::PhantomData};
use std::{
fmt::{self},
hash::Hash,
marker::PhantomData,
};

/// An `EventId` uniquely identifies an event.
///
Expand Down
4 changes: 0 additions & 4 deletions crates/bevy_app/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ pub enum CoreStage {
Startup,
/// Name of app stage that runs before all other app stages
First,
/// Name of app stage that runs before EVENT
PreEvent,
/// Name of app stage that updates events. Runs before UPDATE
Event,
/// Name of app stage responsible for performing setup before an update. Runs before UPDATE.
PreUpdate,
/// Name of app stage responsible for doing most app logic. Systems should be registered here by default.
Expand Down
6 changes: 4 additions & 2 deletions crates/bevy_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pub mod prelude {
}

use bevy_app::prelude::*;
use bevy_ecs::{entity::Entity, system::IntoSystem};
use bevy_ecs::{entity::Entity, prelude::IntoExclusiveSystem, system::IntoSystem};
use bevy_utils::HashSet;
use std::ops::Range;

Expand All @@ -44,7 +44,9 @@ impl Plugin for CorePlugin {
.register_type::<Labels>()
.register_type::<Range<f32>>()
.register_type::<Timer>()
.add_system_to_stage(CoreStage::First, time_system.system())
// time system is added as an "exclusive system" to ensure it runs before other systems in CoreStage::First
// this also ensures that it runs before other exclusive systems added after CorePlugin
.add_system_to_stage(CoreStage::First, time_system.exclusive_system())
cart marked this conversation as resolved.
Show resolved Hide resolved
.add_startup_system_to_stage(StartupStage::PostStartup, entity_labels_system.system())
.add_system_to_stage(CoreStage::PostUpdate, entity_labels_system.system());

Expand Down
45 changes: 34 additions & 11 deletions crates/bevy_ecs/src/schedule/stage.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::{
component::ComponentId,
schedule::{
BoxedSystemLabel, ExclusiveSystemContainer, InsertionPoint, ParallelExecutor,
ParallelSystemContainer, ParallelSystemExecutor, RunCriteria, ShouldRun,
Expand Down Expand Up @@ -263,22 +264,30 @@ impl SystemStage {
}

/// Logs execution order ambiguities between systems. System orders must be fresh.
fn report_ambiguities(&self) {
fn report_ambiguities(&self, world: &World) {
debug_assert!(!self.systems_modified);
use std::fmt::Write;
fn write_display_names_of_pairs(
string: &mut String,
systems: &[impl SystemContainer],
mut ambiguities: Vec<(usize, usize)>,
mut ambiguities: Vec<(usize, usize, Vec<ComponentId>)>,
world: &World,
) {
for (index_a, index_b) in ambiguities.drain(..) {
for (index_a, index_b, conflicts) in ambiguities.drain(..) {
writeln!(
string,
" -- {:?} and {:?}",
systems[index_a].name(),
systems[index_b].name()
)
.unwrap();
if !conflicts.is_empty() {
let names = conflicts
.iter()
.map(|id| world.components().get_info(*id).unwrap().name())
.collect::<Vec<_>>();
writeln!(string, " conflicts: {:?}", names).unwrap();
}
}
}
let parallel = find_ambiguities(&self.parallel);
Expand All @@ -295,23 +304,29 @@ impl SystemStage {
.to_owned();
if !parallel.is_empty() {
writeln!(string, " * Parallel systems:").unwrap();
write_display_names_of_pairs(&mut string, &self.parallel, parallel);
write_display_names_of_pairs(&mut string, &self.parallel, parallel, world);
}
if !at_start.is_empty() {
writeln!(string, " * Exclusive systems at start of stage:").unwrap();
write_display_names_of_pairs(&mut string, &self.exclusive_at_start, at_start);
write_display_names_of_pairs(
&mut string,
&self.exclusive_at_start,
at_start,
world,
);
}
if !before_commands.is_empty() {
writeln!(string, " * Exclusive systems before commands of stage:").unwrap();
write_display_names_of_pairs(
&mut string,
&self.exclusive_before_commands,
before_commands,
world,
);
}
if !at_end.is_empty() {
writeln!(string, " * Exclusive systems at end of stage:").unwrap();
write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end);
write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world);
}
info!("{}", string);
}
Expand Down Expand Up @@ -456,7 +471,7 @@ fn topological_order(

/// Returns vector containing all pairs of indices of systems with ambiguous execution order.
/// Systems must be topologically sorted beforehand.
cart marked this conversation as resolved.
Show resolved Hide resolved
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize)> {
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
let mut ambiguity_set_labels = HashMap::default();
for set in systems.iter().flat_map(|c| c.ambiguity_sets()) {
let len = ambiguity_set_labels.len();
Expand Down Expand Up @@ -511,9 +526,17 @@ fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize)> {
{
if !processed.contains(index_b)
&& all_ambiguity_sets[index_a].is_disjoint(&all_ambiguity_sets[index_b])
&& !systems[index_a].is_compatible(&systems[index_b])
{
ambiguities.push((index_a, index_b));
let a_access = systems[index_a].component_access();
let b_access = systems[index_b].component_access();
if let (Some(a), Some(b)) = (a_access, b_access) {
let conflicts = a.get_conflicts(b);
if !conflicts.is_empty() {
ambiguities.push((index_a, index_b, conflicts))
}
} else {
ambiguities.push((index_a, index_b, Vec::new()));
}
}
}
processed.insert(index_a);
Expand Down Expand Up @@ -549,7 +572,7 @@ impl Stage for SystemStage {
self.executor.rebuild_cached_data(&self.parallel);
self.executor_modified = false;
if world.contains_resource::<ReportExecutionOrderAmbiguities>() {
self.report_ambiguities();
self.report_ambiguities(world);
}
} else if self.executor_modified {
self.executor.rebuild_cached_data(&self.parallel);
Expand Down Expand Up @@ -1184,7 +1207,7 @@ mod tests {
) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> {
find_ambiguities(systems)
.drain(..)
.map(|(index_a, index_b)| {
.map(|(index_a, index_b, _conflicts)| {
cart marked this conversation as resolved.
Show resolved Hide resolved
(
systems[index_a].labels()[0].clone(),
systems[index_b].labels()[0].clone(),
Expand Down
14 changes: 7 additions & 7 deletions crates/bevy_ecs/src/schedule/system_container.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::{
component::ComponentId,
query::Access,
schedule::{
BoxedAmbiguitySetLabel, BoxedSystemLabel, ExclusiveSystemDescriptor,
ParallelSystemDescriptor,
Expand All @@ -16,7 +18,7 @@ pub(super) trait SystemContainer {
fn before(&self) -> &[BoxedSystemLabel];
fn after(&self) -> &[BoxedSystemLabel];
fn ambiguity_sets(&self) -> &[BoxedAmbiguitySetLabel];
fn is_compatible(&self, other: &Self) -> bool;
fn component_access(&self) -> Option<&Access<ComponentId>>;
}

pub(super) struct ExclusiveSystemContainer {
Expand Down Expand Up @@ -81,8 +83,8 @@ impl SystemContainer for ExclusiveSystemContainer {
&self.ambiguity_sets
}

fn is_compatible(&self, _: &Self) -> bool {
false
fn component_access(&self) -> Option<&Access<ComponentId>> {
None
}
}

Expand Down Expand Up @@ -178,9 +180,7 @@ impl SystemContainer for ParallelSystemContainer {
&self.ambiguity_sets
}

fn is_compatible(&self, other: &Self) -> bool {
self.system()
.component_access()
.is_compatible(other.system().component_access())
fn component_access(&self) -> Option<&Access<ComponentId>> {
Some(self.system().component_access())
}
}
2 changes: 2 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ impl<'a, T: Component> SystemParam for Option<Res<'a, T>> {

unsafe impl<T: Component> SystemParamState for OptionResState<T> {
type Config = ();

fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self {
Self(ResState::init(world, system_state, ()))
}
Expand Down Expand Up @@ -383,6 +384,7 @@ impl<'a, T: Component> SystemParam for Option<ResMut<'a, T>> {

unsafe impl<T: Component> SystemParamState for OptionResMutState<T> {
type Config = ();

fn init(world: &mut World, system_state: &mut SystemState, _config: Self::Config) -> Self {
Self(ResMutState::init(world, system_state, ()))
}
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_gilrs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ impl Plugin for GilrsPlugin {
gilrs_event_startup_system.exclusive_system(),
)
.add_system_to_stage(
CoreStage::PreEvent,
CoreStage::PreUpdate,
gilrs_event_system.exclusive_system(),
);
}
Expand Down
38 changes: 30 additions & 8 deletions crates/bevy_input/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ pub mod system;
pub mod touch;

pub use axis::*;
use bevy_ecs::system::IntoSystem;
use bevy_ecs::{
schedule::{ParallelSystemDescriptorCoercion, SystemLabel},
system::IntoSystem,
};
pub use input::*;

pub mod prelude {
Expand Down Expand Up @@ -37,27 +40,46 @@ use gamepad::{
#[derive(Default)]
pub struct InputPlugin;

#[derive(Debug, PartialEq, Eq, Clone, Hash, SystemLabel)]
pub struct InputSystem;

impl Plugin for InputPlugin {
fn build(&self, app: &mut AppBuilder) {
app.add_event::<KeyboardInput>()
app
// keyboard
.add_event::<KeyboardInput>()
.init_resource::<Input<KeyCode>>()
.add_system_to_stage(
CoreStage::PreUpdate,
keyboard_input_system.system().label(InputSystem),
)
// mouse
.add_event::<MouseButtonInput>()
.add_event::<MouseMotion>()
.add_event::<MouseWheel>()
.init_resource::<Input<KeyCode>>()
.add_system_to_stage(CoreStage::Event, keyboard_input_system.system())
.init_resource::<Input<MouseButton>>()
.add_system_to_stage(CoreStage::Event, mouse_button_input_system.system())
.add_system_to_stage(
CoreStage::PreUpdate,
mouse_button_input_system.system().label(InputSystem),
)
// gamepad
.add_event::<GamepadEvent>()
.add_event::<GamepadEventRaw>()
.init_resource::<GamepadSettings>()
.init_resource::<Input<GamepadButton>>()
.init_resource::<Axis<GamepadAxis>>()
.init_resource::<Axis<GamepadButton>>()
.add_system_to_stage(CoreStage::Event, gamepad_event_system.system())
.add_startup_system_to_stage(StartupStage::Startup, gamepad_event_system.system())
.add_system_to_stage(
CoreStage::PreUpdate,
gamepad_event_system.system().label(InputSystem),
)
// touch
.add_event::<TouchInput>()
.init_resource::<Touches>()
.add_system_to_stage(CoreStage::Event, touch_screen_input_system.system());
.add_system_to_stage(
CoreStage::PreUpdate,
touch_screen_input_system.system().label(InputSystem),
);
}
}

Expand Down
24 changes: 18 additions & 6 deletions crates/bevy_render/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ pub mod texture;
pub mod wireframe;

use bevy_ecs::{
schedule::SystemStage,
schedule::{ParallelSystemDescriptorCoercion, SystemStage},
system::{IntoExclusiveSystem, IntoSystem},
};
use bevy_transform::TransformSystem;
use draw::Visible;
pub use once_cell;

Expand All @@ -37,7 +38,7 @@ use crate::prelude::*;
use base::Msaa;
use bevy_app::prelude::*;
use bevy_asset::{AddAsset, AssetStage};
use bevy_ecs::schedule::StageLabel;
use bevy_ecs::schedule::{StageLabel, SystemLabel};
use camera::{
ActiveCameras, Camera, DepthCalculation, OrthographicProjection, PerspectiveProjection,
RenderLayers, ScalingMode, VisibleEntities, WindowOrigin,
Expand All @@ -57,6 +58,11 @@ use texture::HdrTextureLoader;
#[cfg(feature = "png")]
use texture::ImageTextureLoader;

#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemLabel)]
pub enum RenderSystem {
VisibleEntities,
}

/// The names of "render" App stages
#[derive(Debug, Hash, PartialEq, Eq, Clone, StageLabel)]
pub enum RenderStage {
Expand Down Expand Up @@ -157,16 +163,22 @@ impl Plugin for RenderPlugin {
)
.add_system_to_stage(
CoreStage::PostUpdate,
camera::camera_system::<OrthographicProjection>.system(),
camera::camera_system::<OrthographicProjection>
.system()
.before(RenderSystem::VisibleEntities),
)
.add_system_to_stage(
CoreStage::PostUpdate,
camera::camera_system::<PerspectiveProjection>.system(),
camera::camera_system::<PerspectiveProjection>
.system()
.before(RenderSystem::VisibleEntities),
)
// registration order matters here. this must come after all camera_system::<T> systems
.add_system_to_stage(
CoreStage::PostUpdate,
camera::visible_entities_system.system(),
camera::visible_entities_system
.system()
.label(RenderSystem::VisibleEntities)
.after(TransformSystem::TransformPropagate),
)
.add_system_to_stage(
RenderStage::RenderResource,
Expand Down
Loading