From 5fd628ebd32aea9a38882dfc38cc3160cf4c82f7 Mon Sep 17 00:00:00 2001 From: ira Date: Mon, 16 Jan 2023 20:20:37 +0000 Subject: [PATCH 01/24] Fix Alien Cake Addict example despawn warnings (#7236) # Problem The example's `teardown` system despawns all entities besides the camera using `despawn_recursive` causing it to despawn child entities multiple times which logs a warning. ![image](https://user-images.githubusercontent.com/29694403/212756554-06b3fa42-ddcb-4a05-b841-f587488a10fc.png) # Solution Use `despawn` instead. Co-authored-by: Devil Ira --- examples/games/alien_cake_addict.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/games/alien_cake_addict.rs b/examples/games/alien_cake_addict.rs index c0267af6077a4..bc4f04bd1ac6c 100644 --- a/examples/games/alien_cake_addict.rs +++ b/examples/games/alien_cake_addict.rs @@ -179,7 +179,7 @@ fn setup(mut commands: Commands, asset_server: Res, mut game: ResMu // remove all entities that are not a camera fn teardown(mut commands: Commands, entities: Query>) { for entity in &entities { - commands.entity(entity).despawn_recursive(); + commands.entity(entity).despawn(); } } From addc36fe297bce3325cf1fa110a67692ac09d232 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 16 Jan 2023 20:35:15 +0000 Subject: [PATCH 02/24] Add safety comments to usages of `byte_add` (`Ptr`, `PtrMut`, `OwningPtr`) (#7214) # Objective The usages of the unsafe function `byte_add` are not properly documented. Follow-up to #7151. ## Solution Add safety comments to each call-site. --- crates/bevy_ecs/src/storage/blob_vec.rs | 42 ++++++++++++++++++------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index aa7b3718aba08..49b370b7b8e3a 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -233,6 +233,8 @@ impl BlobVec { #[must_use = "The returned pointer should be used to dropped the removed element"] pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> OwningPtr<'_> { debug_assert!(index < self.len()); + // Since `index` must be strictly less than `self.len` and `index` is at least zero, + // `self.len` must be at least one. Thus, this cannot underflow. let new_len = self.len - 1; let size = self.item_layout.size(); if index != new_len { @@ -244,6 +246,10 @@ impl BlobVec { } self.len = new_len; // Cannot use get_unchecked here as this is technically out of bounds after changing len. + // SAFETY: + // - `new_len` is less than the old len, so it must fit in this vector's allocation. + // - `size` is a multiple of the erased type's alignment, + // so adding a multiple of `size` will preserve alignment. self.get_ptr_mut().byte_add(new_len * size).promote() } @@ -285,7 +291,13 @@ impl BlobVec { #[inline] pub unsafe fn get_unchecked(&self, index: usize) -> Ptr<'_> { debug_assert!(index < self.len()); - self.get_ptr().byte_add(index * self.item_layout.size()) + let size = self.item_layout.size(); + // SAFETY: + // - The caller ensures that `index` fits in this vector, + // so this operation will not overflow the original allocation. + // - `size` is a multiple of the erased type's alignment, + // so adding a multiple of `size` will preserve alignment. + self.get_ptr().byte_add(index * size) } /// # Safety @@ -293,8 +305,13 @@ impl BlobVec { #[inline] pub unsafe fn get_unchecked_mut(&mut self, index: usize) -> PtrMut<'_> { debug_assert!(index < self.len()); - let layout_size = self.item_layout.size(); - self.get_ptr_mut().byte_add(index * layout_size) + let size = self.item_layout.size(); + // SAFETY: + // - The caller ensures that `index` fits in this vector, + // so this operation will not overflow the original allocation. + // - `size` is a multiple of the erased type's alignment, + // so adding a multiple of `size` will preserve alignment. + self.get_ptr_mut().byte_add(index * size) } /// Gets a [`Ptr`] to the start of the vec @@ -326,15 +343,18 @@ impl BlobVec { // accidentally drop elements twice in the event of a drop impl panicking. self.len = 0; if let Some(drop) = self.drop { - let layout_size = self.item_layout.size(); + let size = self.item_layout.size(); for i in 0..len { - // SAFETY: `i * layout_size` is inbounds for the allocation, and the item is left unreachable so it can be safely promoted to an `OwningPtr` - unsafe { - // NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index - // will panic here due to self.len being set to 0 - let ptr = self.get_ptr_mut().byte_add(i * layout_size).promote(); - (drop)(ptr); - } + // SAFETY: + // * 0 <= `i` < `len`, so `i * size` must be in bounds for the allocation. + // * `size` is a multiple of the erased type's alignment, + // so adding a multiple of `size` will preserve alignment. + // * The item is left unreachable so it can be safely promoted to an `OwningPtr`. + // NOTE: `self.get_unchecked_mut(i)` cannot be used here, since the `debug_assert` + // would panic due to `self.len` being set to 0. + let item = unsafe { self.get_ptr_mut().byte_add(i * size).promote() }; + // SAFETY: `item` was obtained from this `BlobVec`, so its underlying type must match `drop`. + unsafe { drop(item) }; } } } From d4e3fcdfbf306c10cd28cb914e228cc0a24c2336 Mon Sep 17 00:00:00 2001 From: Elbert Ronnie Date: Mon, 16 Jan 2023 21:09:24 +0000 Subject: [PATCH 03/24] Fix incorrect behavior of `just_pressed` and `just_released` in `Input` (#7238) # Objective - Fixes a bug where `just_pressed` and `just_released` in `Input` might behave incorrectly due calling `clear` 3 times in a single frame through these three different systems: `gamepad_button_event_system`, `gamepad_axis_event_system` and `gamepad_connection_system` in any order ## Solution - Call `clear` only once and before all the above three systems, i.e. in `gamepad_event_system` ## Additional Info - Discussion in Discord: https://discord.com/channels/691052431525675048/768253008416342076/1064621963693273279 --- crates/bevy_input/src/gamepad.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/bevy_input/src/gamepad.rs b/crates/bevy_input/src/gamepad.rs index 03ac48a241129..cc8101d6810a6 100644 --- a/crates/bevy_input/src/gamepad.rs +++ b/crates/bevy_input/src/gamepad.rs @@ -992,7 +992,6 @@ pub fn gamepad_connection_system( mut button_axis: ResMut>, mut button_input: ResMut>, ) { - button_input.bypass_change_detection().clear(); for connection_event in connection_events.iter() { let gamepad = connection_event.gamepad; @@ -1117,27 +1116,24 @@ impl GamepadButtonChangedEvent { } } -/// Uses [`GamepadAxisChangedEvent`]s to update update the relevant `Input` and `Axis` values. +/// Uses [`GamepadAxisChangedEvent`]s to update the relevant `Input` and `Axis` values. pub fn gamepad_axis_event_system( - mut button_input: ResMut>, mut gamepad_axis: ResMut>, mut axis_events: EventReader, ) { - button_input.bypass_change_detection().clear(); for axis_event in axis_events.iter() { let axis = GamepadAxis::new(axis_event.gamepad, axis_event.axis_type); gamepad_axis.set(axis, axis_event.value); } } -/// Uses [`GamepadButtonChangedEvent`]s to update update the relevant `Input` and `Axis` values. +/// Uses [`GamepadButtonChangedEvent`]s to update the relevant `Input` and `Axis` values. pub fn gamepad_button_event_system( mut button_events: EventReader, mut button_input: ResMut>, mut button_axis: ResMut>, settings: Res, ) { - button_input.bypass_change_detection().clear(); for button_event in button_events.iter() { let button = GamepadButton::new(button_event.gamepad, button_event.button_type); let value = button_event.value; @@ -1197,7 +1193,9 @@ pub fn gamepad_event_system( mut connection_events: EventWriter, mut button_events: EventWriter, mut axis_events: EventWriter, + mut button_input: ResMut>, ) { + button_input.bypass_change_detection().clear(); for gamepad_event in gamepad_events.iter() { match gamepad_event { GamepadEvent::Connection(connection_event) => { From e44990a48d97fe73aef8f53d2016bd05b260e1ba Mon Sep 17 00:00:00 2001 From: ld000 Date: Mon, 16 Jan 2023 21:24:15 +0000 Subject: [PATCH 04/24] Add ReplaceChildren and ClearChildren EntityCommands (#6035) # Objective Fixes #5859 ## Solution - Add `ClearChildren` and `ReplaceChildren` commands in the `crates/bevy_hierarchy/src/child_builder.rs` --- ## Changelog - Added `ClearChildren` and `ReplaceChildren` struct - Added `clear_children(&mut self) -> &mut Self` and `replace_children(&mut self, children: &[Entity]) -> &mut Self` function in `BuildChildren` trait - Changed `PushChildren` `write` function body to a `push_children ` function to reused in `ReplaceChildren` - Added `clear_children` function - Added `push_and_replace_children_commands` and `push_and_clear_children_commands` test Co-authored-by: ld000 Co-authored-by: lidong63 --- crates/bevy_hierarchy/src/child_builder.rs | 133 +++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/crates/bevy_hierarchy/src/child_builder.rs b/crates/bevy_hierarchy/src/child_builder.rs index 31970ee4c0d5e..395667a6e78d1 100644 --- a/crates/bevy_hierarchy/src/child_builder.rs +++ b/crates/bevy_hierarchy/src/child_builder.rs @@ -140,6 +140,14 @@ fn remove_children(parent: Entity, children: &[Entity], world: &mut World) { } } +fn clear_children(parent: Entity, world: &mut World) { + if let Some(children) = world.entity_mut(parent).remove::() { + for &child in &children.0 { + world.entity_mut(child).remove::(); + } + } +} + /// Command that adds a child to an entity #[derive(Debug)] pub struct AddChild { @@ -196,6 +204,30 @@ impl Command for RemoveChildren { } } +/// Command that clear all children from an entity. +pub struct ClearChildren { + parent: Entity, +} + +impl Command for ClearChildren { + fn write(self, world: &mut World) { + clear_children(self.parent, world); + } +} + +/// Command that clear all children from an entity. And replace with the given children. +pub struct ReplaceChildren { + parent: Entity, + children: SmallVec<[Entity; 8]>, +} + +impl Command for ReplaceChildren { + fn write(self, world: &mut World) { + clear_children(self.parent, world); + world.entity_mut(self.parent).push_children(&self.children); + } +} + /// Command that removes the parent of an entity, and removes that entity from the parent's [`Children`]. pub struct RemoveParent { /// `Entity` whose parent must be removed. @@ -268,6 +300,10 @@ pub trait BuildChildren { /// will have those children removed from its list. Removing all children from a parent causes its /// [`Children`] component to be removed from the entity. fn add_child(&mut self, child: Entity) -> &mut Self; + /// Removes all children from this entity. The [`Children`] component will be removed if it exists, otherwise this does nothing. + fn clear_children(&mut self) -> &mut Self; + /// Removes all current children from this entity, replacing them with the specified list of entities. + fn replace_children(&mut self, children: &[Entity]) -> &mut Self; /// Sets the parent of this entity. fn set_parent(&mut self, parent: Entity) -> &mut Self; /// Removes the parent of this entity. @@ -325,6 +361,21 @@ impl<'w, 's, 'a> BuildChildren for EntityCommands<'w, 's, 'a> { self } + fn clear_children(&mut self) -> &mut Self { + let parent = self.id(); + self.commands().add(ClearChildren { parent }); + self + } + + fn replace_children(&mut self, children: &[Entity]) -> &mut Self { + let parent = self.id(); + self.commands().add(ReplaceChildren { + children: SmallVec::from(children), + parent, + }); + self + } + fn set_parent(&mut self, parent: Entity) -> &mut Self { let child = self.id(); self.commands().add(AddChild { child, parent }); @@ -728,6 +779,88 @@ mod tests { assert!(world.get::(child4).is_none()); } + #[test] + fn push_and_clear_children_commands() { + let mut world = World::default(); + let entities = world + .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) + .collect::>(); + + let mut queue = CommandQueue::default(); + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(entities[0]).push_children(&entities[1..3]); + } + queue.apply(&mut world); + + let parent = entities[0]; + let child1 = entities[1]; + let child2 = entities[2]; + + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child2]; + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent).clear_children(); + } + queue.apply(&mut world); + + assert!(world.get::(parent).is_none()); + + assert!(world.get::(child1).is_none()); + assert!(world.get::(child2).is_none()); + } + + #[test] + fn push_and_replace_children_commands() { + let mut world = World::default(); + let entities = world + .spawn_batch(vec![(C(1),), (C(2),), (C(3),), (C(4),), (C(5),)]) + .collect::>(); + + let mut queue = CommandQueue::default(); + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(entities[0]).push_children(&entities[1..3]); + } + queue.apply(&mut world); + + let parent = entities[0]; + let child1 = entities[1]; + let child2 = entities[2]; + let child4 = entities[4]; + + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child2]; + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child2).unwrap(), Parent(parent)); + + let replace_children = [child1, child4]; + { + let mut commands = Commands::new(&mut queue, &world); + commands.entity(parent).replace_children(&replace_children); + } + queue.apply(&mut world); + + let expected_children: SmallVec<[Entity; 8]> = smallvec![child1, child4]; + assert_eq!( + world.get::(parent).unwrap().0.clone(), + expected_children + ); + assert_eq!(*world.get::(child1).unwrap(), Parent(parent)); + assert_eq!(*world.get::(child4).unwrap(), Parent(parent)); + assert!(world.get::(child2).is_none()); + } + #[test] fn push_and_insert_and_remove_children_world() { let mut world = World::default(); From 39e14a4a40014fe5f14bef9cf39916ed3e799b2e Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Mon, 16 Jan 2023 22:10:51 +0000 Subject: [PATCH 05/24] Make `EntityRef::new` unsafe (#7222) # Objective - We rely on the construction of `EntityRef` to be valid elsewhere in unsafe code. This construction is not checked (for performance reasons), and thus this private method must be unsafe. - Fixes #7218. ## Solution - Make the method unsafe. - Add safety docs. - Improve safety docs slightly for the sibling `EntityMut::new`. - Add debug asserts to start to verify these assumptions in debug mode. ## Context for reviewers I attempted to verify the `EntityLocation` more thoroughly, but this turned out to be more work than expected. I've spun that off into #7221 as a result. --- crates/bevy_ecs/src/entity/mod.rs | 2 +- crates/bevy_ecs/src/world/entity_ref.rs | 24 +++++++++++++++++++++--- crates/bevy_ecs/src/world/mod.rs | 10 ++++++++-- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 5107923e4a22d..afcba27bb68c1 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -747,7 +747,7 @@ impl EntityMeta { // SAFETY: // This type must not contain any pointers at any level, and be safe to fully fill with u8::MAX. /// A location of an entity in an archetype. -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq)] #[repr(C)] pub struct EntityLocation { /// The ID of the [`Archetype`] the [`Entity`] belongs to. diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index f21675103fb34..a165678516987 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/crates/bevy_ecs/src/world/entity_ref.rs @@ -22,8 +22,17 @@ pub struct EntityRef<'w> { } impl<'w> EntityRef<'w> { + /// # Safety + /// + /// - `entity` must be valid for `world`: the generation should match that of the entity at the same index. + /// - `location` must be sourced from `world`'s `Entities` and must exactly match the location for `entity` + /// + /// The above is trivially satisfied if `location` was sourced from `world.entities().get(entity)`. #[inline] - pub(crate) fn new(world: &'w World, entity: Entity, location: EntityLocation) -> Self { + pub(crate) unsafe fn new(world: &'w World, entity: Entity, location: EntityLocation) -> Self { + debug_assert!(world.entities().contains(entity)); + debug_assert_eq!(world.entities().get(entity), Some(location)); + Self { world, entity, @@ -193,7 +202,9 @@ impl<'w> EntityRef<'w> { impl<'w> From> for EntityRef<'w> { fn from(entity_mut: EntityMut<'w>) -> EntityRef<'w> { - EntityRef::new(entity_mut.world, entity_mut.entity, entity_mut.location) + // SAFETY: the safety invariants on EntityMut and EntityRef are identical + // and EntityMut is promised to be valid by construction. + unsafe { EntityRef::new(entity_mut.world, entity_mut.entity, entity_mut.location) } } } @@ -206,13 +217,20 @@ pub struct EntityMut<'w> { impl<'w> EntityMut<'w> { /// # Safety - /// entity and location _must_ be valid + /// + /// - `entity` must be valid for `world`: the generation should match that of the entity at the same index. + /// - `location` must be sourced from `world`'s `Entities` and must exactly match the location for `entity` + /// + /// The above is trivially satisfied if `location` was sourced from `world.entities().get(entity)`. #[inline] pub(crate) unsafe fn new( world: &'w mut World, entity: Entity, location: EntityLocation, ) -> Self { + debug_assert!(world.entities().contains(entity)); + debug_assert_eq!(world.entities().get(entity), Some(location)); + EntityMut { world, entity, diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 2243af65deb6e..af4807344660c 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -317,7 +317,10 @@ impl World { #[inline] pub fn get_entity(&self, entity: Entity) -> Option { let location = self.entities.get(entity)?; - Some(EntityRef::new(self, entity, location)) + // SAFETY: if the Entity is invalid, the function returns early. + // Additionally, Entities::get(entity) returns the correct EntityLocation if the entity exists. + let entity_ref = unsafe { EntityRef::new(self, entity, location) }; + Some(entity_ref) } /// Returns an [`Entity`] iterator of current entities. @@ -331,13 +334,16 @@ impl World { .iter() .enumerate() .map(|(archetype_row, archetype_entity)| { + let entity = archetype_entity.entity(); let location = EntityLocation { archetype_id: archetype.id(), archetype_row: ArchetypeRow::new(archetype_row), table_id: archetype.table_id(), table_row: archetype_entity.table_row(), }; - EntityRef::new(self, archetype_entity.entity(), location) + + // SAFETY: entity exists and location accurately specifies the archetype where the entity is stored + unsafe { EntityRef::new(self, entity, location) } }) }) } From 6b4795c428f694a332f0a4710df1ba9b5499bc26 Mon Sep 17 00:00:00 2001 From: ira Date: Mon, 16 Jan 2023 23:13:11 +0000 Subject: [PATCH 06/24] Add `Camera::viewport_to_world_2d` (#6557) # Objective Add a simpler and less expensive 2D variant of `viewport_to_world`. Co-authored-by: devil-ira --- crates/bevy_render/src/camera/camera.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/bevy_render/src/camera/camera.rs b/crates/bevy_render/src/camera/camera.rs index 72bcfa3556df0..dc6901d8c2e3f 100644 --- a/crates/bevy_render/src/camera/camera.rs +++ b/crates/bevy_render/src/camera/camera.rs @@ -248,6 +248,25 @@ impl Camera { }) } + /// Returns a 2D world position computed from a position on this [`Camera`]'s viewport. + /// + /// Useful for 2D cameras and other cameras with an orthographic projection pointing along the Z axis. + /// + /// To get the world space coordinates with Normalized Device Coordinates, you should use + /// [`ndc_to_world`](Self::ndc_to_world). + pub fn viewport_to_world_2d( + &self, + camera_transform: &GlobalTransform, + viewport_position: Vec2, + ) -> Option { + let target_size = self.logical_viewport_size()?; + let ndc = viewport_position * 2. / target_size - Vec2::ONE; + + let world_near_plane = self.ndc_to_world(camera_transform, ndc.extend(1.))?; + + Some(world_near_plane.truncate()) + } + /// Given a position in world space, use the camera's viewport to compute the Normalized Device Coordinates. /// /// When the position is within the viewport the values returned will be between -1.0 and 1.0 on the X and Y axes, From 684f07595f2f440556cd26a3d8fb5af0d809f872 Mon Sep 17 00:00:00 2001 From: Cameron <51241057+maniwani@users.noreply.github.com> Date: Tue, 17 Jan 2023 01:39:17 +0000 Subject: [PATCH 07/24] Add `bevy_ecs::schedule_v3` module (#6587) # Objective Complete the first part of the migration detailed in bevyengine/rfcs#45. ## Solution Add all the new stuff. ### TODO - [x] Impl tuple methods. - [x] Impl chaining. - [x] Port ambiguity detection. - [x] Write docs. - [x] ~~Write more tests.~~(will do later) - [ ] Write changelog and examples here? - [x] ~~Replace `petgraph`.~~ (will do later) Co-authored-by: james7132 Co-authored-by: Michael Hsu Co-authored-by: Mike Hsu --- crates/bevy_ecs/macros/src/lib.rs | 30 +- crates/bevy_ecs/src/lib.rs | 1 + crates/bevy_ecs/src/schedule_v3/condition.rs | 97 ++ crates/bevy_ecs/src/schedule_v3/config.rs | 649 ++++++++++ .../bevy_ecs/src/schedule_v3/executor/mod.rs | 90 ++ .../schedule_v3/executor/multi_threaded.rs | 575 +++++++++ .../src/schedule_v3/executor/simple.rs | 111 ++ .../schedule_v3/executor/single_threaded.rs | 137 ++ .../bevy_ecs/src/schedule_v3/graph_utils.rs | 233 ++++ crates/bevy_ecs/src/schedule_v3/migration.rs | 38 + crates/bevy_ecs/src/schedule_v3/mod.rs | 471 +++++++ crates/bevy_ecs/src/schedule_v3/schedule.rs | 1099 +++++++++++++++++ crates/bevy_ecs/src/schedule_v3/set.rs | 149 +++ crates/bevy_ecs/src/schedule_v3/state.rs | 64 + .../src/system/exclusive_function_system.rs | 13 +- crates/bevy_ecs/src/system/function_system.rs | 13 +- crates/bevy_ecs/src/system/system.rs | 8 + crates/bevy_ecs/src/system/system_piping.rs | 18 +- crates/bevy_ecs/src/world/mod.rs | 25 +- crates/bevy_macro_utils/src/lib.rs | 64 + crates/bevy_time/src/fixed_timestep.rs | 4 + crates/bevy_utils/Cargo.toml | 2 + crates/bevy_utils/src/label.rs | 41 + crates/bevy_utils/src/lib.rs | 3 + crates/bevy_utils/src/syncunsafecell.rs | 122 ++ 25 files changed, 4050 insertions(+), 7 deletions(-) create mode 100644 crates/bevy_ecs/src/schedule_v3/condition.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/config.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/executor/mod.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/executor/multi_threaded.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/executor/simple.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/executor/single_threaded.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/graph_utils.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/migration.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/mod.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/schedule.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/set.rs create mode 100644 crates/bevy_ecs/src/schedule_v3/state.rs create mode 100644 crates/bevy_utils/src/syncunsafecell.rs diff --git a/crates/bevy_ecs/macros/src/lib.rs b/crates/bevy_ecs/macros/src/lib.rs index 3d8a10b3af68e..07d01a8c870ab 100644 --- a/crates/bevy_ecs/macros/src/lib.rs +++ b/crates/bevy_ecs/macros/src/lib.rs @@ -4,7 +4,9 @@ mod component; mod fetch; use crate::fetch::derive_world_query_impl; -use bevy_macro_utils::{derive_label, get_named_struct_fields, BevyManifest}; +use bevy_macro_utils::{ + derive_boxed_label, derive_label, derive_set, get_named_struct_fields, BevyManifest, +}; use proc_macro::TokenStream; use proc_macro2::Span; use quote::{format_ident, quote}; @@ -565,6 +567,32 @@ pub fn derive_run_criteria_label(input: TokenStream) -> TokenStream { derive_label(input, &trait_path, "run_criteria_label") } +/// Derive macro generating an impl of the trait `ScheduleLabel`. +#[proc_macro_derive(ScheduleLabel)] +pub fn derive_schedule_label(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + let mut trait_path = bevy_ecs_path(); + trait_path + .segments + .push(format_ident!("schedule_v3").into()); + trait_path + .segments + .push(format_ident!("ScheduleLabel").into()); + derive_boxed_label(input, &trait_path) +} + +/// Derive macro generating an impl of the trait `SystemSet`. +#[proc_macro_derive(SystemSet)] +pub fn derive_system_set(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + let mut trait_path = bevy_ecs_path(); + trait_path + .segments + .push(format_ident!("schedule_v3").into()); + trait_path.segments.push(format_ident!("SystemSet").into()); + derive_set(input, &trait_path) +} + pub(crate) fn bevy_ecs_path() -> syn::Path { BevyManifest::default().get_path("bevy_ecs") } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index 47d93beab837d..b0a9f9f155a06 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -14,6 +14,7 @@ pub mod query; #[cfg(feature = "bevy_reflect")] pub mod reflect; pub mod schedule; +pub mod schedule_v3; pub mod storage; pub mod system; pub mod world; diff --git a/crates/bevy_ecs/src/schedule_v3/condition.rs b/crates/bevy_ecs/src/schedule_v3/condition.rs new file mode 100644 index 0000000000000..e617375f763a0 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/condition.rs @@ -0,0 +1,97 @@ +pub use common_conditions::*; + +use crate::system::BoxedSystem; + +pub type BoxedCondition = BoxedSystem<(), bool>; + +/// A system that determines if one or more scheduled systems should run. +/// +/// Implemented for functions and closures that convert into [`System`](crate::system::System) +/// with [read-only](crate::system::ReadOnlySystemParam) parameters. +pub trait Condition: sealed::Condition {} + +impl Condition for F where F: sealed::Condition {} + +mod sealed { + use crate::system::{IntoSystem, IsFunctionSystem, ReadOnlySystemParam, SystemParamFunction}; + + pub trait Condition: IntoSystem<(), bool, Params> {} + + impl Condition<(IsFunctionSystem, Params, Marker)> for F + where + F: SystemParamFunction<(), bool, Params, Marker> + Send + Sync + 'static, + Params: ReadOnlySystemParam + 'static, + Marker: 'static, + { + } +} + +mod common_conditions { + use crate::schedule_v3::{State, States}; + use crate::system::{Res, Resource}; + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the resource exists. + pub fn resource_exists() -> impl FnMut(Option>) -> bool + where + T: Resource, + { + move |res: Option>| res.is_some() + } + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the resource is equal to `value`. + /// + /// # Panics + /// + /// The condition will panic if the resource does not exist. + pub fn resource_equals(value: T) -> impl FnMut(Res) -> bool + where + T: Resource + PartialEq, + { + move |res: Res| *res == value + } + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the resource exists and is equal to `value`. + /// + /// The condition will return `false` if the resource does not exist. + pub fn resource_exists_and_equals(value: T) -> impl FnMut(Option>) -> bool + where + T: Resource + PartialEq, + { + move |res: Option>| match res { + Some(res) => *res == value, + None => false, + } + } + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the state machine exists. + pub fn state_exists() -> impl FnMut(Option>>) -> bool { + move |current_state: Option>>| current_state.is_some() + } + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the state machine is currently in `state`. + /// + /// # Panics + /// + /// The condition will panic if the resource does not exist. + pub fn state_equals(state: S) -> impl FnMut(Res>) -> bool { + move |current_state: Res>| current_state.0 == state + } + + /// Generates a [`Condition`](super::Condition)-satisfying closure that returns `true` + /// if the state machine exists and is currently in `state`. + /// + /// The condition will return `false` if the state does not exist. + pub fn state_exists_and_equals( + state: S, + ) -> impl FnMut(Option>>) -> bool { + move |current_state: Option>>| match current_state { + Some(current_state) => current_state.0 == state, + None => false, + } + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/config.rs b/crates/bevy_ecs/src/schedule_v3/config.rs new file mode 100644 index 0000000000000..d7a87c374c047 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/config.rs @@ -0,0 +1,649 @@ +use bevy_ecs_macros::all_tuples; +use bevy_utils::default; + +use crate::{ + schedule_v3::{ + condition::{BoxedCondition, Condition}, + graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo}, + set::{BoxedSystemSet, IntoSystemSet, SystemSet}, + }, + system::{BoxedSystem, IntoSystem, System}, +}; + +/// A [`SystemSet`] with scheduling metadata. +pub struct SystemSetConfig { + pub(super) set: BoxedSystemSet, + pub(super) graph_info: GraphInfo, + pub(super) conditions: Vec, +} + +impl SystemSetConfig { + fn new(set: BoxedSystemSet) -> Self { + // system type sets are automatically populated + // to avoid unintentionally broad changes, they cannot be configured + assert!( + !set.is_system_type(), + "configuring system type sets is not allowed" + ); + + Self { + set, + graph_info: GraphInfo { + sets: Vec::new(), + dependencies: Vec::new(), + ambiguous_with: default(), + }, + conditions: Vec::new(), + } + } +} + +/// A [`System`] with scheduling metadata. +pub struct SystemConfig { + pub(super) system: BoxedSystem, + pub(super) graph_info: GraphInfo, + pub(super) conditions: Vec, +} + +impl SystemConfig { + fn new(system: BoxedSystem) -> Self { + // include system in its default sets + let sets = system.default_system_sets().into_iter().collect(); + Self { + system, + graph_info: GraphInfo { + sets, + dependencies: Vec::new(), + ambiguous_with: default(), + }, + conditions: Vec::new(), + } + } +} + +fn new_condition

(condition: impl Condition

) -> BoxedCondition { + let condition_system = IntoSystem::into_system(condition); + assert!( + condition_system.is_send(), + "Condition `{}` accesses thread-local resources. This is not currently supported.", + condition_system.name() + ); + + Box::new(condition_system) +} + +fn ambiguous_with(graph_info: &mut GraphInfo, set: BoxedSystemSet) { + match &mut graph_info.ambiguous_with { + detection @ Ambiguity::Check => { + *detection = Ambiguity::IgnoreWithSet(vec![set]); + } + Ambiguity::IgnoreWithSet(ambiguous_with) => { + ambiguous_with.push(set); + } + Ambiguity::IgnoreAll => (), + } +} + +/// Types that can be converted into a [`SystemSetConfig`]. +/// +/// This has been implemented for all types that implement [`SystemSet`] and boxed trait objects. +pub trait IntoSystemSetConfig: sealed::IntoSystemSetConfig { + /// Convert into a [`SystemSetConfig`]. + #[doc(hidden)] + fn into_config(self) -> SystemSetConfig; + /// Add to the provided `set`. + fn in_set(self, set: impl SystemSet) -> SystemSetConfig; + /// Run before all systems in `set`. + fn before(self, set: impl IntoSystemSet) -> SystemSetConfig; + /// Run after all systems in `set`. + fn after(self, set: impl IntoSystemSet) -> SystemSetConfig; + /// Run the systems in this set only if the [`Condition`] is `true`. + /// + /// The `Condition` will be evaluated at most once (per schedule run), + /// the first time a system in this set prepares to run. + fn run_if

(self, condition: impl Condition

) -> SystemSetConfig; + /// Suppress warnings and errors that would result from systems in this set having ambiguities + /// (conflicting access but indeterminate order) with systems in `set`. + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig; + /// Suppress warnings and errors that would result from systems in this set having ambiguities + /// (conflicting access but indeterminate order) with any other system. + fn ambiguous_with_all(self) -> SystemSetConfig; +} + +impl IntoSystemSetConfig for S +where + S: SystemSet + sealed::IntoSystemSetConfig, +{ + fn into_config(self) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)) + } + + fn in_set(self, set: impl SystemSet) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).in_set(set) + } + + fn before(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).before(set) + } + + fn after(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).after(set) + } + + fn run_if

(self, condition: impl Condition

) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).run_if(condition) + } + + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).ambiguous_with(set) + } + + fn ambiguous_with_all(self) -> SystemSetConfig { + SystemSetConfig::new(Box::new(self)).ambiguous_with_all() + } +} + +impl IntoSystemSetConfig for BoxedSystemSet { + fn into_config(self) -> SystemSetConfig { + SystemSetConfig::new(self) + } + + fn in_set(self, set: impl SystemSet) -> SystemSetConfig { + SystemSetConfig::new(self).in_set(set) + } + + fn before(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(self).before(set) + } + + fn after(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(self).after(set) + } + + fn run_if

(self, condition: impl Condition

) -> SystemSetConfig { + SystemSetConfig::new(self).run_if(condition) + } + + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { + SystemSetConfig::new(self).ambiguous_with(set) + } + + fn ambiguous_with_all(self) -> SystemSetConfig { + SystemSetConfig::new(self).ambiguous_with_all() + } +} + +impl IntoSystemSetConfig for SystemSetConfig { + fn into_config(self) -> Self { + self + } + + fn in_set(mut self, set: impl SystemSet) -> Self { + assert!( + !set.is_system_type(), + "adding arbitrary systems to a system type set is not allowed" + ); + self.graph_info.sets.push(Box::new(set)); + self + } + + fn before(mut self, set: impl IntoSystemSet) -> Self { + self.graph_info.dependencies.push(Dependency::new( + DependencyKind::Before, + Box::new(set.into_system_set()), + )); + self + } + + fn after(mut self, set: impl IntoSystemSet) -> Self { + self.graph_info.dependencies.push(Dependency::new( + DependencyKind::After, + Box::new(set.into_system_set()), + )); + self + } + + fn run_if

(mut self, condition: impl Condition

) -> Self { + self.conditions.push(new_condition(condition)); + self + } + + fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { + ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); + self + } + + fn ambiguous_with_all(mut self) -> Self { + self.graph_info.ambiguous_with = Ambiguity::IgnoreAll; + self + } +} + +/// Types that can be converted into a [`SystemConfig`]. +/// +/// This has been implemented for boxed [`System`](crate::system::System) +/// trait objects and all functions that turn into such. +pub trait IntoSystemConfig: sealed::IntoSystemConfig { + /// Convert into a [`SystemConfig`]. + #[doc(hidden)] + fn into_config(self) -> SystemConfig; + /// Add to `set` membership. + fn in_set(self, set: impl SystemSet) -> SystemConfig; + /// Run before all systems in `set`. + fn before(self, set: impl IntoSystemSet) -> SystemConfig; + /// Run after all systems in `set`. + fn after(self, set: impl IntoSystemSet) -> SystemConfig; + /// Run only if the [`Condition`] is `true`. + /// + /// The `Condition` will be evaluated at most once (per schedule run), + /// when the system prepares to run. + fn run_if

(self, condition: impl Condition

) -> SystemConfig; + /// Suppress warnings and errors that would result from this system having ambiguities + /// (conflicting access but indeterminate order) with systems in `set`. + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig; + /// Suppress warnings and errors that would result from this system having ambiguities + /// (conflicting access but indeterminate order) with any other system. + fn ambiguous_with_all(self) -> SystemConfig; +} + +impl IntoSystemConfig for F +where + F: IntoSystem<(), (), Params> + sealed::IntoSystemConfig, +{ + fn into_config(self) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))) + } + + fn in_set(self, set: impl SystemSet) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).in_set(set) + } + + fn before(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).before(set) + } + + fn after(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).after(set) + } + + fn run_if

(self, condition: impl Condition

) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).run_if(condition) + } + + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).ambiguous_with(set) + } + + fn ambiguous_with_all(self) -> SystemConfig { + SystemConfig::new(Box::new(IntoSystem::into_system(self))).ambiguous_with_all() + } +} + +impl IntoSystemConfig<()> for BoxedSystem<(), ()> { + fn into_config(self) -> SystemConfig { + SystemConfig::new(self) + } + + fn in_set(self, set: impl SystemSet) -> SystemConfig { + SystemConfig::new(self).in_set(set) + } + + fn before(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(self).before(set) + } + + fn after(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(self).after(set) + } + + fn run_if

(self, condition: impl Condition

) -> SystemConfig { + SystemConfig::new(self).run_if(condition) + } + + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { + SystemConfig::new(self).ambiguous_with(set) + } + + fn ambiguous_with_all(self) -> SystemConfig { + SystemConfig::new(self).ambiguous_with_all() + } +} + +impl IntoSystemConfig<()> for SystemConfig { + fn into_config(self) -> Self { + self + } + + fn in_set(mut self, set: impl SystemSet) -> Self { + assert!( + !set.is_system_type(), + "adding arbitrary systems to a system type set is not allowed" + ); + self.graph_info.sets.push(Box::new(set)); + self + } + + fn before(mut self, set: impl IntoSystemSet) -> Self { + self.graph_info.dependencies.push(Dependency::new( + DependencyKind::Before, + Box::new(set.into_system_set()), + )); + self + } + + fn after(mut self, set: impl IntoSystemSet) -> Self { + self.graph_info.dependencies.push(Dependency::new( + DependencyKind::After, + Box::new(set.into_system_set()), + )); + self + } + + fn run_if

(mut self, condition: impl Condition

) -> Self { + self.conditions.push(new_condition(condition)); + self + } + + fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { + ambiguous_with(&mut self.graph_info, Box::new(set.into_system_set())); + self + } + + fn ambiguous_with_all(mut self) -> Self { + self.graph_info.ambiguous_with = Ambiguity::IgnoreAll; + self + } +} + +// only `System` system objects can be scheduled +mod sealed { + use crate::{ + schedule_v3::{BoxedSystemSet, SystemSet}, + system::{BoxedSystem, IntoSystem}, + }; + + use super::{SystemConfig, SystemSetConfig}; + + pub trait IntoSystemConfig {} + + impl> IntoSystemConfig for F {} + + impl IntoSystemConfig<()> for BoxedSystem<(), ()> {} + + impl IntoSystemConfig<()> for SystemConfig {} + + pub trait IntoSystemSetConfig {} + + impl IntoSystemSetConfig for S {} + + impl IntoSystemSetConfig for BoxedSystemSet {} + + impl IntoSystemSetConfig for SystemSetConfig {} +} + +/// A collection of [`SystemConfig`]. +pub struct SystemConfigs { + pub(super) systems: Vec, + /// If `true`, adds `before -> after` ordering constraints between the successive elements. + pub(super) chained: bool, +} + +/// Types that can convert into a [`SystemConfigs`]. +pub trait IntoSystemConfigs +where + Self: Sized, +{ + /// Convert into a [`SystemConfigs`]. + #[doc(hidden)] + fn into_configs(self) -> SystemConfigs; + + /// Add these systems to the provided `set`. + fn in_set(self, set: impl SystemSet) -> SystemConfigs { + self.into_configs().in_set(set) + } + + /// Run before all systems in `set`. + fn before(self, set: impl IntoSystemSet) -> SystemConfigs { + self.into_configs().before(set) + } + + /// Run after all systems in `set`. + fn after(self, set: impl IntoSystemSet) -> SystemConfigs { + self.into_configs().after(set) + } + + /// Suppress warnings and errors that would result from these systems having ambiguities + /// (conflicting access but indeterminate order) with systems in `set`. + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfigs { + self.into_configs().ambiguous_with(set) + } + + /// Suppress warnings and errors that would result from these systems having ambiguities + /// (conflicting access but indeterminate order) with any other system. + fn ambiguous_with_all(self) -> SystemConfigs { + self.into_configs().ambiguous_with_all() + } + + /// Treat this collection as a sequence of systems. + /// + /// Ordering constraints will be applied between the successive elements. + fn chain(self) -> SystemConfigs { + self.into_configs().chain() + } +} + +impl IntoSystemConfigs<()> for SystemConfigs { + fn into_configs(self) -> Self { + self + } + + fn in_set(mut self, set: impl SystemSet) -> Self { + assert!( + !set.is_system_type(), + "adding arbitrary systems to a system type set is not allowed" + ); + for config in &mut self.systems { + config.graph_info.sets.push(set.dyn_clone()); + } + + self + } + + fn before(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.systems { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::Before, set.dyn_clone())); + } + + self + } + + fn after(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.systems { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::After, set.dyn_clone())); + } + + self + } + + fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.systems { + ambiguous_with(&mut config.graph_info, set.dyn_clone()); + } + + self + } + + fn ambiguous_with_all(mut self) -> Self { + for config in &mut self.systems { + config.graph_info.ambiguous_with = Ambiguity::IgnoreAll; + } + + self + } + + fn chain(mut self) -> Self { + self.chained = true; + self + } +} + +/// A collection of [`SystemSetConfig`]. +pub struct SystemSetConfigs { + pub(super) sets: Vec, + /// If `true`, adds `before -> after` ordering constraints between the successive elements. + pub(super) chained: bool, +} + +/// Types that can convert into a [`SystemSetConfigs`]. +pub trait IntoSystemSetConfigs +where + Self: Sized, +{ + /// Convert into a [`SystemSetConfigs`]. + #[doc(hidden)] + fn into_configs(self) -> SystemSetConfigs; + + /// Add these system sets to the provided `set`. + fn in_set(self, set: impl SystemSet) -> SystemSetConfigs { + self.into_configs().in_set(set) + } + + /// Run before all systems in `set`. + fn before(self, set: impl IntoSystemSet) -> SystemSetConfigs { + self.into_configs().before(set) + } + + /// Run after all systems in `set`. + fn after(self, set: impl IntoSystemSet) -> SystemSetConfigs { + self.into_configs().after(set) + } + + /// Suppress warnings and errors that would result from systems in these sets having ambiguities + /// (conflicting access but indeterminate order) with systems in `set`. + fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfigs { + self.into_configs().ambiguous_with(set) + } + + /// Suppress warnings and errors that would result from systems in these sets having ambiguities + /// (conflicting access but indeterminate order) with any other system. + fn ambiguous_with_all(self) -> SystemSetConfigs { + self.into_configs().ambiguous_with_all() + } + + /// Treat this collection as a sequence of system sets. + /// + /// Ordering constraints will be applied between the successive elements. + fn chain(self) -> SystemSetConfigs { + self.into_configs().chain() + } +} + +impl IntoSystemSetConfigs for SystemSetConfigs { + fn into_configs(self) -> Self { + self + } + + fn in_set(mut self, set: impl SystemSet) -> Self { + assert!( + !set.is_system_type(), + "adding arbitrary systems to a system type set is not allowed" + ); + for config in &mut self.sets { + config.graph_info.sets.push(set.dyn_clone()); + } + + self + } + + fn before(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.sets { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::Before, set.dyn_clone())); + } + + self + } + + fn after(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.sets { + config + .graph_info + .dependencies + .push(Dependency::new(DependencyKind::After, set.dyn_clone())); + } + + self + } + + fn ambiguous_with(mut self, set: impl IntoSystemSet) -> Self { + let set = set.into_system_set(); + for config in &mut self.sets { + ambiguous_with(&mut config.graph_info, set.dyn_clone()); + } + + self + } + + fn ambiguous_with_all(mut self) -> Self { + for config in &mut self.sets { + config.graph_info.ambiguous_with = Ambiguity::IgnoreAll; + } + + self + } + + fn chain(mut self) -> Self { + self.chained = true; + self + } +} + +macro_rules! impl_system_collection { + ($(($param: ident, $sys: ident)),*) => { + impl<$($param, $sys),*> IntoSystemConfigs<($($param,)*)> for ($($sys,)*) + where + $($sys: IntoSystemConfig<$param>),* + { + #[allow(non_snake_case)] + fn into_configs(self) -> SystemConfigs { + let ($($sys,)*) = self; + SystemConfigs { + systems: vec![$($sys.into_config(),)*], + chained: false, + } + } + } + } +} + +macro_rules! impl_system_set_collection { + ($($set: ident),*) => { + impl<$($set: IntoSystemSetConfig),*> IntoSystemSetConfigs for ($($set,)*) + { + #[allow(non_snake_case)] + fn into_configs(self) -> SystemSetConfigs { + let ($($set,)*) = self; + SystemSetConfigs { + sets: vec![$($set.into_config(),)*], + chained: false, + } + } + } + } +} + +all_tuples!(impl_system_collection, 0, 15, P, S); +all_tuples!(impl_system_set_collection, 0, 15, S); diff --git a/crates/bevy_ecs/src/schedule_v3/executor/mod.rs b/crates/bevy_ecs/src/schedule_v3/executor/mod.rs new file mode 100644 index 0000000000000..bfc1eef14d609 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/executor/mod.rs @@ -0,0 +1,90 @@ +mod multi_threaded; +mod simple; +mod single_threaded; + +pub use self::multi_threaded::MultiThreadedExecutor; +pub use self::simple::SimpleExecutor; +pub use self::single_threaded::SingleThreadedExecutor; + +use fixedbitset::FixedBitSet; + +use crate::{ + schedule_v3::{BoxedCondition, NodeId}, + system::BoxedSystem, + world::World, +}; + +/// Types that can run a [`SystemSchedule`] on a [`World`]. +pub(super) trait SystemExecutor: Send + Sync { + fn kind(&self) -> ExecutorKind; + fn init(&mut self, schedule: &SystemSchedule); + fn run(&mut self, schedule: &mut SystemSchedule, world: &mut World); +} + +/// Specifies how a [`Schedule`](super::Schedule) will be run. +/// +/// [`MultiThreaded`](ExecutorKind::MultiThreaded) is the default. +#[derive(PartialEq, Eq, Default)] +pub enum ExecutorKind { + /// Runs the schedule using a single thread. + /// + /// Useful if you're dealing with a single-threaded environment, saving your threads for + /// other things, or just trying minimize overhead. + SingleThreaded, + /// Like [`SingleThreaded`](ExecutorKind::SingleThreaded) but calls [`apply_buffers`](crate::system::System::apply_buffers) + /// immediately after running each system. + Simple, + /// Runs the schedule using a thread pool. Non-conflicting systems can run in parallel. + #[default] + MultiThreaded, +} + +/// Holds systems and conditions of a [`Schedule`](super::Schedule) sorted in topological order +/// (along with dependency information for multi-threaded execution). +/// +/// Since the arrays are sorted in the same order, elements are referenced by their index. +/// `FixedBitSet` is used as a smaller, more efficient substitute of `HashSet`. +#[derive(Default)] +pub(super) struct SystemSchedule { + pub(super) systems: Vec, + pub(super) system_conditions: Vec>, + pub(super) set_conditions: Vec>, + pub(super) system_ids: Vec, + pub(super) set_ids: Vec, + pub(super) system_dependencies: Vec, + pub(super) system_dependents: Vec>, + pub(super) sets_of_systems: Vec, + pub(super) systems_in_sets: Vec, +} + +impl SystemSchedule { + pub const fn new() -> Self { + Self { + systems: Vec::new(), + system_conditions: Vec::new(), + set_conditions: Vec::new(), + system_ids: Vec::new(), + set_ids: Vec::new(), + system_dependencies: Vec::new(), + system_dependents: Vec::new(), + sets_of_systems: Vec::new(), + systems_in_sets: Vec::new(), + } + } +} + +/// Instructs the executor to call [`apply_buffers`](crate::system::System::apply_buffers) +/// on the systems that have run but not applied their buffers. +/// +/// **Notes** +/// - This function (currently) does nothing if it's called manually or wrapped inside a [`PipeSystem`](crate::system::PipeSystem). +/// - Modifying a [`Schedule`](super::Schedule) may change the order buffers are applied. +#[allow(unused_variables)] +pub fn apply_system_buffers(world: &mut World) {} + +/// Returns `true` if the [`System`](crate::system::System) is an instance of [`apply_system_buffers`]. +pub(super) fn is_apply_system_buffers(system: &BoxedSystem) -> bool { + use std::any::Any; + // deref to use `System::type_id` instead of `Any::type_id` + system.as_ref().type_id() == apply_system_buffers.type_id() +} diff --git a/crates/bevy_ecs/src/schedule_v3/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule_v3/executor/multi_threaded.rs new file mode 100644 index 0000000000000..3debba3ac59b4 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/executor/multi_threaded.rs @@ -0,0 +1,575 @@ +use bevy_tasks::{ComputeTaskPool, Scope, TaskPool}; +use bevy_utils::default; +use bevy_utils::syncunsafecell::SyncUnsafeCell; +#[cfg(feature = "trace")] +use bevy_utils::tracing::{info_span, Instrument}; + +use async_channel::{Receiver, Sender}; +use fixedbitset::FixedBitSet; + +use crate::{ + archetype::ArchetypeComponentId, + query::Access, + schedule_v3::{ + is_apply_system_buffers, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, + }, + system::BoxedSystem, + world::World, +}; + +/// A funky borrow split of [`SystemSchedule`] required by the [`MultiThreadedExecutor`]. +struct SyncUnsafeSchedule<'a> { + systems: &'a [SyncUnsafeCell], + conditions: Conditions<'a>, +} + +struct Conditions<'a> { + system_conditions: &'a mut [Vec], + set_conditions: &'a mut [Vec], + sets_of_systems: &'a [FixedBitSet], + systems_in_sets: &'a [FixedBitSet], +} + +impl SyncUnsafeSchedule<'_> { + fn new(schedule: &mut SystemSchedule) -> SyncUnsafeSchedule<'_> { + SyncUnsafeSchedule { + systems: SyncUnsafeCell::from_mut(schedule.systems.as_mut_slice()).as_slice_of_cells(), + conditions: Conditions { + system_conditions: &mut schedule.system_conditions, + set_conditions: &mut schedule.set_conditions, + sets_of_systems: &schedule.sets_of_systems, + systems_in_sets: &schedule.systems_in_sets, + }, + } + } +} + +/// Per-system data used by the [`MultiThreadedExecutor`]. +// Copied here because it can't be read from the system when it's running. +struct SystemTaskMetadata { + /// The `ArchetypeComponentId` access of the system. + archetype_component_access: Access, + /// Indices of the systems that directly depend on the system. + dependents: Vec, + /// Is `true` if the system does not access `!Send` data. + is_send: bool, + /// Is `true` if the system is exclusive. + is_exclusive: bool, +} + +/// Runs the schedule using a thread pool. Non-conflicting systems can run in parallel. +pub struct MultiThreadedExecutor { + /// Sends system completion events. + sender: Sender, + /// Receives system completion events. + receiver: Receiver, + /// Metadata for scheduling and running system tasks. + system_task_metadata: Vec, + /// Union of the accesses of all currently running systems. + active_access: Access, + /// Returns `true` if a system with non-`Send` access is running. + local_thread_running: bool, + /// Returns `true` if an exclusive system is running. + exclusive_running: bool, + /// The number of systems that are running. + num_running_systems: usize, + /// The number of systems that have completed. + num_completed_systems: usize, + /// The number of dependencies each system has that have not completed. + num_dependencies_remaining: Vec, + /// System sets whose conditions have been evaluated. + evaluated_sets: FixedBitSet, + /// Systems that have no remaining dependencies and are waiting to run. + ready_systems: FixedBitSet, + /// copy of `ready_systems` + ready_systems_copy: FixedBitSet, + /// Systems that are running. + running_systems: FixedBitSet, + /// Systems that got skipped. + skipped_systems: FixedBitSet, + /// Systems whose conditions have been evaluated and were run or skipped. + completed_systems: FixedBitSet, + /// Systems that have run but have not had their buffers applied. + unapplied_systems: FixedBitSet, +} + +impl Default for MultiThreadedExecutor { + fn default() -> Self { + Self::new() + } +} + +impl SystemExecutor for MultiThreadedExecutor { + fn kind(&self) -> ExecutorKind { + ExecutorKind::MultiThreaded + } + + fn init(&mut self, schedule: &SystemSchedule) { + // pre-allocate space + let sys_count = schedule.system_ids.len(); + let set_count = schedule.set_ids.len(); + + self.evaluated_sets = FixedBitSet::with_capacity(set_count); + self.ready_systems = FixedBitSet::with_capacity(sys_count); + self.ready_systems_copy = FixedBitSet::with_capacity(sys_count); + self.running_systems = FixedBitSet::with_capacity(sys_count); + self.completed_systems = FixedBitSet::with_capacity(sys_count); + self.skipped_systems = FixedBitSet::with_capacity(sys_count); + self.unapplied_systems = FixedBitSet::with_capacity(sys_count); + + self.system_task_metadata = Vec::with_capacity(sys_count); + for index in 0..sys_count { + self.system_task_metadata.push(SystemTaskMetadata { + archetype_component_access: default(), + dependents: schedule.system_dependents[index].clone(), + is_send: schedule.systems[index].is_send(), + is_exclusive: schedule.systems[index].is_exclusive(), + }); + } + + self.num_dependencies_remaining = Vec::with_capacity(sys_count); + } + + fn run(&mut self, schedule: &mut SystemSchedule, world: &mut World) { + // reset counts + let num_systems = schedule.systems.len(); + self.num_running_systems = 0; + self.num_completed_systems = 0; + self.num_dependencies_remaining.clear(); + self.num_dependencies_remaining + .extend_from_slice(&schedule.system_dependencies); + + for (system_index, dependencies) in self.num_dependencies_remaining.iter_mut().enumerate() { + if *dependencies == 0 { + self.ready_systems.insert(system_index); + } + } + + let world = SyncUnsafeCell::from_mut(world); + let SyncUnsafeSchedule { + systems, + mut conditions, + } = SyncUnsafeSchedule::new(schedule); + + ComputeTaskPool::init(TaskPool::default).scope(|scope| { + // the executor itself is a `Send` future so that it can run + // alongside systems that claim the local thread + let executor = async { + while self.num_completed_systems < num_systems { + // SAFETY: self.ready_systems does not contain running systems + unsafe { + self.spawn_system_tasks(scope, systems, &mut conditions, world); + } + + if self.num_running_systems > 0 { + // wait for systems to complete + let index = self + .receiver + .recv() + .await + .unwrap_or_else(|error| unreachable!("{}", error)); + + self.finish_system_and_signal_dependents(index); + + while let Ok(index) = self.receiver.try_recv() { + self.finish_system_and_signal_dependents(index); + } + + self.rebuild_active_access(); + } + } + + // SAFETY: all systems have completed + let world = unsafe { &mut *world.get() }; + apply_system_buffers(&mut self.unapplied_systems, systems, world); + + debug_assert!(self.ready_systems.is_clear()); + debug_assert!(self.running_systems.is_clear()); + debug_assert!(self.unapplied_systems.is_clear()); + self.active_access.clear(); + self.evaluated_sets.clear(); + self.skipped_systems.clear(); + self.completed_systems.clear(); + }; + + #[cfg(feature = "trace")] + let executor_span = info_span!("schedule_task"); + #[cfg(feature = "trace")] + let executor = executor.instrument(executor_span); + scope.spawn(executor); + }); + } +} + +impl MultiThreadedExecutor { + pub fn new() -> Self { + let (sender, receiver) = async_channel::unbounded(); + Self { + sender, + receiver, + system_task_metadata: Vec::new(), + num_running_systems: 0, + num_completed_systems: 0, + num_dependencies_remaining: Vec::new(), + active_access: default(), + local_thread_running: false, + exclusive_running: false, + evaluated_sets: FixedBitSet::new(), + ready_systems: FixedBitSet::new(), + ready_systems_copy: FixedBitSet::new(), + running_systems: FixedBitSet::new(), + skipped_systems: FixedBitSet::new(), + completed_systems: FixedBitSet::new(), + unapplied_systems: FixedBitSet::new(), + } + } + + /// # Safety + /// Caller must ensure that `self.ready_systems` does not contain any systems that + /// have been mutably borrowed (such as the systems currently running). + unsafe fn spawn_system_tasks<'scope>( + &mut self, + scope: &Scope<'_, 'scope, ()>, + systems: &'scope [SyncUnsafeCell], + conditions: &mut Conditions, + cell: &'scope SyncUnsafeCell, + ) { + if self.exclusive_running { + return; + } + + // can't borrow since loop mutably borrows `self` + let mut ready_systems = std::mem::take(&mut self.ready_systems_copy); + ready_systems.clear(); + ready_systems.union_with(&self.ready_systems); + + for system_index in ready_systems.ones() { + assert!(!self.running_systems.contains(system_index)); + // SAFETY: Caller assured that these systems are not running. + // Therefore, no other reference to this system exists and there is no aliasing. + let system = unsafe { &mut *systems[system_index].get() }; + + // SAFETY: No exclusive system is running. + // Therefore, there is no existing mutable reference to the world. + let world = unsafe { &*cell.get() }; + if !self.can_run(system_index, system, conditions, world) { + // NOTE: exclusive systems with ambiguities are susceptible to + // being significantly displaced here (compared to single-threaded order) + // if systems after them in topological order can run + // if that becomes an issue, `break;` if exclusive system + continue; + } + + self.ready_systems.set(system_index, false); + + if !self.should_run(system_index, system, conditions, world) { + self.skip_system_and_signal_dependents(system_index); + continue; + } + + self.running_systems.insert(system_index); + self.num_running_systems += 1; + + if self.system_task_metadata[system_index].is_exclusive { + // SAFETY: `can_run` confirmed that no systems are running. + // Therefore, there is no existing reference to the world. + unsafe { + let world = &mut *cell.get(); + self.spawn_exclusive_system_task(scope, system_index, systems, world); + } + break; + } + + // SAFETY: No other reference to this system exists. + unsafe { + self.spawn_system_task(scope, system_index, systems, world); + } + } + + // give back + self.ready_systems_copy = ready_systems; + } + + fn can_run( + &mut self, + system_index: usize, + system: &mut BoxedSystem, + conditions: &mut Conditions, + world: &World, + ) -> bool { + #[cfg(feature = "trace")] + let _span = info_span!("check_access", name = &*system.name()).entered(); + + let system_meta = &self.system_task_metadata[system_index]; + if system_meta.is_exclusive && self.num_running_systems > 0 { + return false; + } + + if !system_meta.is_send && self.local_thread_running { + return false; + } + + // TODO: an earlier out if world's archetypes did not change + for set_idx in conditions.sets_of_systems[system_index].difference(&self.evaluated_sets) { + for condition in &mut conditions.set_conditions[set_idx] { + condition.update_archetype_component_access(world); + if !condition + .archetype_component_access() + .is_compatible(&self.active_access) + { + return false; + } + } + } + + for condition in &mut conditions.system_conditions[system_index] { + condition.update_archetype_component_access(world); + if !condition + .archetype_component_access() + .is_compatible(&self.active_access) + { + return false; + } + } + + if !self.skipped_systems.contains(system_index) { + system.update_archetype_component_access(world); + if !system + .archetype_component_access() + .is_compatible(&self.active_access) + { + return false; + } + + // PERF: use an optimized clear() + extend() operation + let meta_access = + &mut self.system_task_metadata[system_index].archetype_component_access; + meta_access.clear(); + meta_access.extend(system.archetype_component_access()); + } + + true + } + + fn should_run( + &mut self, + system_index: usize, + _system: &BoxedSystem, + conditions: &mut Conditions, + world: &World, + ) -> bool { + #[cfg(feature = "trace")] + let _span = info_span!("check_conditions", name = &*_system.name()).entered(); + + let mut should_run = !self.skipped_systems.contains(system_index); + for set_idx in conditions.sets_of_systems[system_index].ones() { + if self.evaluated_sets.contains(set_idx) { + continue; + } + + // evaluate system set's conditions + let set_conditions_met = + evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world); + + if !set_conditions_met { + self.skipped_systems + .union_with(&conditions.systems_in_sets[set_idx]); + } + + should_run &= set_conditions_met; + self.evaluated_sets.insert(set_idx); + } + + // evaluate system's conditions + let system_conditions_met = + evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world); + + if !system_conditions_met { + self.skipped_systems.insert(system_index); + } + + should_run &= system_conditions_met; + + should_run + } + + /// # Safety + /// Caller must not alias systems that are running. + unsafe fn spawn_system_task<'scope>( + &mut self, + scope: &Scope<'_, 'scope, ()>, + system_index: usize, + systems: &'scope [SyncUnsafeCell], + world: &'scope World, + ) { + // SAFETY: this system is not running, no other reference exists + let system = unsafe { &mut *systems[system_index].get() }; + + #[cfg(feature = "trace")] + let task_span = info_span!("system_task", name = &*system.name()); + #[cfg(feature = "trace")] + let system_span = info_span!("system", name = &*system.name()); + + let sender = self.sender.clone(); + let task = async move { + #[cfg(feature = "trace")] + let system_guard = system_span.enter(); + // SAFETY: access is compatible + unsafe { system.run_unsafe((), world) }; + #[cfg(feature = "trace")] + drop(system_guard); + sender + .send(system_index) + .await + .unwrap_or_else(|error| unreachable!("{}", error)); + }; + + #[cfg(feature = "trace")] + let task = task.instrument(task_span); + + let system_meta = &self.system_task_metadata[system_index]; + self.active_access + .extend(&system_meta.archetype_component_access); + + if system_meta.is_send { + scope.spawn(task); + } else { + self.local_thread_running = true; + scope.spawn_on_scope(task); + } + } + + /// # Safety + /// Caller must ensure no systems are currently borrowed. + unsafe fn spawn_exclusive_system_task<'scope>( + &mut self, + scope: &Scope<'_, 'scope, ()>, + system_index: usize, + systems: &'scope [SyncUnsafeCell], + world: &'scope mut World, + ) { + // SAFETY: this system is not running, no other reference exists + let system = unsafe { &mut *systems[system_index].get() }; + + #[cfg(feature = "trace")] + let task_span = info_span!("system_task", name = &*system.name()); + #[cfg(feature = "trace")] + let system_span = info_span!("system", name = &*system.name()); + + let sender = self.sender.clone(); + if is_apply_system_buffers(system) { + // TODO: avoid allocation + let mut unapplied_systems = self.unapplied_systems.clone(); + let task = async move { + #[cfg(feature = "trace")] + let system_guard = system_span.enter(); + apply_system_buffers(&mut unapplied_systems, systems, world); + #[cfg(feature = "trace")] + drop(system_guard); + sender + .send(system_index) + .await + .unwrap_or_else(|error| unreachable!("{}", error)); + }; + + #[cfg(feature = "trace")] + let task = task.instrument(task_span); + scope.spawn_on_scope(task); + } else { + let task = async move { + #[cfg(feature = "trace")] + let system_guard = system_span.enter(); + system.run((), world); + #[cfg(feature = "trace")] + drop(system_guard); + sender + .send(system_index) + .await + .unwrap_or_else(|error| unreachable!("{}", error)); + }; + + #[cfg(feature = "trace")] + let task = task.instrument(task_span); + scope.spawn_on_scope(task); + } + + self.exclusive_running = true; + self.local_thread_running = true; + } + + fn finish_system_and_signal_dependents(&mut self, system_index: usize) { + if self.system_task_metadata[system_index].is_exclusive { + self.exclusive_running = false; + } + + if !self.system_task_metadata[system_index].is_send { + self.local_thread_running = false; + } + + debug_assert!(self.num_running_systems >= 1); + self.num_running_systems -= 1; + self.num_completed_systems += 1; + self.running_systems.set(system_index, false); + self.completed_systems.insert(system_index); + self.unapplied_systems.insert(system_index); + self.signal_dependents(system_index); + } + + fn skip_system_and_signal_dependents(&mut self, system_index: usize) { + self.num_completed_systems += 1; + self.completed_systems.insert(system_index); + self.signal_dependents(system_index); + } + + fn signal_dependents(&mut self, system_index: usize) { + #[cfg(feature = "trace")] + let _span = info_span!("signal_dependents").entered(); + for &dep_idx in &self.system_task_metadata[system_index].dependents { + let remaining = &mut self.num_dependencies_remaining[dep_idx]; + debug_assert!(*remaining >= 1); + *remaining -= 1; + if *remaining == 0 && !self.completed_systems.contains(dep_idx) { + self.ready_systems.insert(dep_idx); + } + } + } + + fn rebuild_active_access(&mut self) { + self.active_access.clear(); + for index in self.running_systems.ones() { + let system_meta = &self.system_task_metadata[index]; + self.active_access + .extend(&system_meta.archetype_component_access); + } + } +} + +fn apply_system_buffers( + unapplied_systems: &mut FixedBitSet, + systems: &[SyncUnsafeCell], + world: &mut World, +) { + for system_index in unapplied_systems.ones() { + // SAFETY: none of these systems are running, no other references exist + let system = unsafe { &mut *systems[system_index].get() }; + #[cfg(feature = "trace")] + let _apply_buffers_span = info_span!("apply_buffers", name = &*system.name()).entered(); + system.apply_buffers(world); + } + + unapplied_systems.clear(); +} + +fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { + // not short-circuiting is intentional + #[allow(clippy::unnecessary_fold)] + conditions + .iter_mut() + .map(|condition| { + #[cfg(feature = "trace")] + let _condition_span = info_span!("condition", name = &*condition.name()).entered(); + // SAFETY: caller ensures system access is compatible + unsafe { condition.run_unsafe((), world) } + }) + .fold(true, |acc, res| acc && res) +} diff --git a/crates/bevy_ecs/src/schedule_v3/executor/simple.rs b/crates/bevy_ecs/src/schedule_v3/executor/simple.rs new file mode 100644 index 0000000000000..1d45aa29129b3 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/executor/simple.rs @@ -0,0 +1,111 @@ +#[cfg(feature = "trace")] +use bevy_utils::tracing::info_span; +use fixedbitset::FixedBitSet; + +use crate::{ + schedule_v3::{BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule}, + world::World, +}; + +/// A variant of [`SingleThreadedExecutor`](crate::schedule_v3::SingleThreadedExecutor) that calls +/// [`apply_buffers`](crate::system::System::apply_buffers) immediately after running each system. +#[derive(Default)] +pub struct SimpleExecutor { + /// Systems sets whose conditions have been evaluated. + evaluated_sets: FixedBitSet, + /// Systems that have run or been skipped. + completed_systems: FixedBitSet, +} + +impl SystemExecutor for SimpleExecutor { + fn kind(&self) -> ExecutorKind { + ExecutorKind::Simple + } + + fn init(&mut self, schedule: &SystemSchedule) { + let sys_count = schedule.system_ids.len(); + let set_count = schedule.set_ids.len(); + self.evaluated_sets = FixedBitSet::with_capacity(set_count); + self.completed_systems = FixedBitSet::with_capacity(sys_count); + } + + fn run(&mut self, schedule: &mut SystemSchedule, world: &mut World) { + for system_index in 0..schedule.systems.len() { + #[cfg(feature = "trace")] + let name = schedule.systems[system_index].name(); + #[cfg(feature = "trace")] + let should_run_span = info_span!("check_conditions", name = &*name).entered(); + + let mut should_run = !self.completed_systems.contains(system_index); + for set_idx in schedule.sets_of_systems[system_index].ones() { + if self.evaluated_sets.contains(set_idx) { + continue; + } + + // evaluate system set's conditions + let set_conditions_met = + evaluate_and_fold_conditions(&mut schedule.set_conditions[set_idx], world); + + if !set_conditions_met { + self.completed_systems + .union_with(&schedule.systems_in_sets[set_idx]); + } + + should_run &= set_conditions_met; + self.evaluated_sets.insert(set_idx); + } + + // evaluate system's conditions + let system_conditions_met = + evaluate_and_fold_conditions(&mut schedule.system_conditions[system_index], world); + + should_run &= system_conditions_met; + + #[cfg(feature = "trace")] + should_run_span.exit(); + + // system has either been skipped or will run + self.completed_systems.insert(system_index); + + if !should_run { + continue; + } + + let system = &mut schedule.systems[system_index]; + #[cfg(feature = "trace")] + let system_span = info_span!("system", name = &*name).entered(); + system.run((), world); + #[cfg(feature = "trace")] + system_span.exit(); + + #[cfg(feature = "trace")] + let _apply_buffers_span = info_span!("apply_buffers", name = &*name).entered(); + system.apply_buffers(world); + } + + self.evaluated_sets.clear(); + self.completed_systems.clear(); + } +} + +impl SimpleExecutor { + pub const fn new() -> Self { + Self { + evaluated_sets: FixedBitSet::new(), + completed_systems: FixedBitSet::new(), + } + } +} + +fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool { + // not short-circuiting is intentional + #[allow(clippy::unnecessary_fold)] + conditions + .iter_mut() + .map(|condition| { + #[cfg(feature = "trace")] + let _condition_span = info_span!("condition", name = &*condition.name()).entered(); + condition.run((), world) + }) + .fold(true, |acc, res| acc && res) +} diff --git a/crates/bevy_ecs/src/schedule_v3/executor/single_threaded.rs b/crates/bevy_ecs/src/schedule_v3/executor/single_threaded.rs new file mode 100644 index 0000000000000..289b05b8c1e78 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/executor/single_threaded.rs @@ -0,0 +1,137 @@ +#[cfg(feature = "trace")] +use bevy_utils::tracing::info_span; +use fixedbitset::FixedBitSet; + +use crate::{ + schedule_v3::{ + is_apply_system_buffers, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, + }, + world::World, +}; + +/// Runs the schedule using a single thread. +/// +/// Useful if you're dealing with a single-threaded environment, saving your threads for +/// other things, or just trying minimize overhead. +#[derive(Default)] +pub struct SingleThreadedExecutor { + /// System sets whose conditions have been evaluated. + evaluated_sets: FixedBitSet, + /// Systems that have run or been skipped. + completed_systems: FixedBitSet, + /// Systems that have run but have not had their buffers applied. + unapplied_systems: FixedBitSet, +} + +impl SystemExecutor for SingleThreadedExecutor { + fn kind(&self) -> ExecutorKind { + ExecutorKind::SingleThreaded + } + + fn init(&mut self, schedule: &SystemSchedule) { + // pre-allocate space + let sys_count = schedule.system_ids.len(); + let set_count = schedule.set_ids.len(); + self.evaluated_sets = FixedBitSet::with_capacity(set_count); + self.completed_systems = FixedBitSet::with_capacity(sys_count); + self.unapplied_systems = FixedBitSet::with_capacity(sys_count); + } + + fn run(&mut self, schedule: &mut SystemSchedule, world: &mut World) { + for system_index in 0..schedule.systems.len() { + #[cfg(feature = "trace")] + let name = schedule.systems[system_index].name(); + #[cfg(feature = "trace")] + let should_run_span = info_span!("check_conditions", name = &*name).entered(); + + let mut should_run = !self.completed_systems.contains(system_index); + for set_idx in schedule.sets_of_systems[system_index].ones() { + if self.evaluated_sets.contains(set_idx) { + continue; + } + + // evaluate system set's conditions + let set_conditions_met = + evaluate_and_fold_conditions(&mut schedule.set_conditions[set_idx], world); + + if !set_conditions_met { + self.completed_systems + .union_with(&schedule.systems_in_sets[set_idx]); + } + + should_run &= set_conditions_met; + self.evaluated_sets.insert(set_idx); + } + + // evaluate system's conditions + let system_conditions_met = + evaluate_and_fold_conditions(&mut schedule.system_conditions[system_index], world); + + should_run &= system_conditions_met; + + #[cfg(feature = "trace")] + should_run_span.exit(); + + // system has either been skipped or will run + self.completed_systems.insert(system_index); + + if !should_run { + continue; + } + + let system = &mut schedule.systems[system_index]; + if is_apply_system_buffers(system) { + #[cfg(feature = "trace")] + let system_span = info_span!("system", name = &*name).entered(); + self.apply_system_buffers(schedule, world); + #[cfg(feature = "trace")] + system_span.exit(); + } else { + #[cfg(feature = "trace")] + let system_span = info_span!("system", name = &*name).entered(); + system.run((), world); + #[cfg(feature = "trace")] + system_span.exit(); + self.unapplied_systems.insert(system_index); + } + } + + self.apply_system_buffers(schedule, world); + self.evaluated_sets.clear(); + self.completed_systems.clear(); + } +} + +impl SingleThreadedExecutor { + pub const fn new() -> Self { + Self { + evaluated_sets: FixedBitSet::new(), + completed_systems: FixedBitSet::new(), + unapplied_systems: FixedBitSet::new(), + } + } + + fn apply_system_buffers(&mut self, schedule: &mut SystemSchedule, world: &mut World) { + for system_index in self.unapplied_systems.ones() { + let system = &mut schedule.systems[system_index]; + #[cfg(feature = "trace")] + let _apply_buffers_span = info_span!("apply_buffers", name = &*system.name()).entered(); + system.apply_buffers(world); + } + + self.unapplied_systems.clear(); + } +} + +fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &mut World) -> bool { + // not short-circuiting is intentional + #[allow(clippy::unnecessary_fold)] + conditions + .iter_mut() + .map(|condition| { + #[cfg(feature = "trace")] + let _condition_span = info_span!("condition", name = &*condition.name()).entered(); + condition.run((), world) + }) + .fold(true, |acc, res| acc && res) +} diff --git a/crates/bevy_ecs/src/schedule_v3/graph_utils.rs b/crates/bevy_ecs/src/schedule_v3/graph_utils.rs new file mode 100644 index 0000000000000..b58bad317a959 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/graph_utils.rs @@ -0,0 +1,233 @@ +use std::fmt::Debug; + +use bevy_utils::{ + petgraph::{graphmap::NodeTrait, prelude::*}, + HashMap, HashSet, +}; +use fixedbitset::FixedBitSet; + +use crate::schedule_v3::set::*; + +/// Unique identifier for a system or system set. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub(crate) enum NodeId { + System(usize), + Set(usize), +} + +impl NodeId { + /// Returns the internal integer value. + pub fn index(&self) -> usize { + match self { + NodeId::System(index) | NodeId::Set(index) => *index, + } + } + + /// Returns `true` if the identified node is a system. + pub const fn is_system(&self) -> bool { + matches!(self, NodeId::System(_)) + } + + /// Returns `true` if the identified node is a system set. + pub const fn is_set(&self) -> bool { + matches!(self, NodeId::Set(_)) + } +} + +/// Specifies what kind of edge should be added to the dependency graph. +#[derive(Debug, Clone, Copy, Eq, PartialEq, PartialOrd, Ord, Hash)] +pub(crate) enum DependencyKind { + /// A node that should be preceded. + Before, + /// A node that should be succeeded. + After, +} + +/// An edge to be added to the dependency graph. +#[derive(Clone)] +pub(crate) struct Dependency { + pub(crate) kind: DependencyKind, + pub(crate) set: BoxedSystemSet, +} + +impl Dependency { + pub fn new(kind: DependencyKind, set: BoxedSystemSet) -> Self { + Self { kind, set } + } +} + +/// Configures ambiguity detection for a single system. +#[derive(Clone, Debug, Default)] +pub(crate) enum Ambiguity { + #[default] + Check, + /// Ignore warnings with systems in any of these system sets. May contain duplicates. + IgnoreWithSet(Vec), + /// Ignore all warnings. + IgnoreAll, +} + +#[derive(Clone)] +pub(crate) struct GraphInfo { + pub(crate) sets: Vec, + pub(crate) dependencies: Vec, + pub(crate) ambiguous_with: Ambiguity, +} + +/// Converts 2D row-major pair of indices into a 1D array index. +pub(crate) fn index(row: usize, col: usize, num_cols: usize) -> usize { + debug_assert!(col < num_cols); + (row * num_cols) + col +} + +/// Converts a 1D array index into a 2D row-major pair of indices. +pub(crate) fn row_col(index: usize, num_cols: usize) -> (usize, usize) { + (index / num_cols, index % num_cols) +} + +/// Stores the results of the graph analysis. +pub(crate) struct CheckGraphResults { + /// Boolean reachability matrix for the graph. + pub(crate) reachable: FixedBitSet, + /// Pairs of nodes that have a path connecting them. + pub(crate) connected: HashSet<(V, V)>, + /// Pairs of nodes that don't have a path connecting them. + pub(crate) disconnected: HashSet<(V, V)>, + /// Edges that are redundant because a longer path exists. + pub(crate) transitive_edges: Vec<(V, V)>, + /// Variant of the graph with no transitive edges. + pub(crate) transitive_reduction: DiGraphMap, + /// Variant of the graph with all possible transitive edges. + // TODO: this will very likely be used by "if-needed" ordering + #[allow(dead_code)] + pub(crate) transitive_closure: DiGraphMap, +} + +impl Default for CheckGraphResults { + fn default() -> Self { + Self { + reachable: FixedBitSet::new(), + connected: HashSet::new(), + disconnected: HashSet::new(), + transitive_edges: Vec::new(), + transitive_reduction: DiGraphMap::new(), + transitive_closure: DiGraphMap::new(), + } + } +} + +/// Processes a DAG and computes its: +/// - transitive reduction (along with the set of removed edges) +/// - transitive closure +/// - reachability matrix (as a bitset) +/// - pairs of nodes connected by a path +/// - pairs of nodes not connected by a path +/// +/// The algorithm implemented comes from +/// ["On the calculation of transitive reduction-closure of orders"][1] by Habib, Morvan and Rampon. +/// +/// [1]: https://doi.org/10.1016/0012-365X(93)90164-O +pub(crate) fn check_graph( + graph: &DiGraphMap, + topological_order: &[V], +) -> CheckGraphResults +where + V: NodeTrait + Debug, +{ + if graph.node_count() == 0 { + return CheckGraphResults::default(); + } + + let n = graph.node_count(); + + // build a copy of the graph where the nodes and edges appear in topsorted order + let mut map = HashMap::with_capacity(n); + let mut topsorted = DiGraphMap::::new(); + // iterate nodes in topological order + for (i, &node) in topological_order.iter().enumerate() { + map.insert(node, i); + topsorted.add_node(node); + // insert nodes as successors to their predecessors + for pred in graph.neighbors_directed(node, Direction::Incoming) { + topsorted.add_edge(pred, node, ()); + } + } + + let mut reachable = FixedBitSet::with_capacity(n * n); + let mut connected = HashSet::new(); + let mut disconnected = HashSet::new(); + + let mut transitive_edges = Vec::new(); + let mut transitive_reduction = DiGraphMap::::new(); + let mut transitive_closure = DiGraphMap::::new(); + + let mut visited = FixedBitSet::with_capacity(n); + + // iterate nodes in topological order + for node in topsorted.nodes() { + transitive_reduction.add_node(node); + transitive_closure.add_node(node); + } + + // iterate nodes in reverse topological order + for a in topsorted.nodes().rev() { + let index_a = *map.get(&a).unwrap(); + // iterate their successors in topological order + for b in topsorted.neighbors_directed(a, Direction::Outgoing) { + let index_b = *map.get(&b).unwrap(); + debug_assert!(index_a < index_b); + if !visited[index_b] { + // edge is not redundant + transitive_reduction.add_edge(a, b, ()); + transitive_closure.add_edge(a, b, ()); + reachable.insert(index(index_a, index_b, n)); + + let successors = transitive_closure + .neighbors_directed(b, Direction::Outgoing) + .collect::>(); + for c in successors { + let index_c = *map.get(&c).unwrap(); + debug_assert!(index_b < index_c); + if !visited[index_c] { + visited.insert(index_c); + transitive_closure.add_edge(a, c, ()); + reachable.insert(index(index_a, index_c, n)); + } + } + } else { + // edge is redundant + transitive_edges.push((a, b)); + } + } + + visited.clear(); + } + + // partition pairs of nodes into "connected by path" and "not connected by path" + for i in 0..(n - 1) { + // reachable is upper triangular because the nodes were topsorted + for index in index(i, i + 1, n)..=index(i, n - 1, n) { + let (a, b) = row_col(index, n); + let pair = (topological_order[a], topological_order[b]); + if reachable[index] { + connected.insert(pair); + } else { + disconnected.insert(pair); + } + } + } + + // fill diagonal (nodes reach themselves) + // for i in 0..n { + // reachable.set(index(i, i, n), true); + // } + + CheckGraphResults { + reachable, + connected, + disconnected, + transitive_edges, + transitive_reduction, + transitive_closure, + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/migration.rs b/crates/bevy_ecs/src/schedule_v3/migration.rs new file mode 100644 index 0000000000000..c4932c2fbca7c --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/migration.rs @@ -0,0 +1,38 @@ +use crate::schedule_v3::*; +use crate::world::World; + +/// Temporary "stageless" `App` methods. +pub trait AppExt { + /// Sets the [`Schedule`] that will be modified by default when you call `App::add_system` + /// and similar methods. + /// + /// **Note:** This will create the schedule if it does not already exist. + fn set_default_schedule(&mut self, label: impl ScheduleLabel) -> &mut Self; + /// Applies the function to the [`Schedule`] associated with `label`. + /// + /// **Note:** This will create the schedule if it does not already exist. + fn edit_schedule( + &mut self, + label: impl ScheduleLabel, + f: impl FnMut(&mut Schedule), + ) -> &mut Self; + /// Adds [`State`] and [`NextState`] resources, [`OnEnter`] and [`OnExit`] schedules + /// for each state variant, and an instance of [`apply_state_transition::`] in + /// \ so that transitions happen before `Update`. + fn add_state(&mut self) -> &mut Self; +} + +/// Temporary "stageless" [`World`] methods. +pub trait WorldExt { + /// Runs the [`Schedule`] associated with `label`. + fn run_schedule(&mut self, label: impl ScheduleLabel); +} + +impl WorldExt for World { + fn run_schedule(&mut self, label: impl ScheduleLabel) { + if let Some(mut schedule) = self.resource_mut::().remove(&label) { + schedule.run(self); + self.resource_mut::().insert(label, schedule); + } + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/mod.rs b/crates/bevy_ecs/src/schedule_v3/mod.rs new file mode 100644 index 0000000000000..6a95e28e4a8e0 --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/mod.rs @@ -0,0 +1,471 @@ +mod condition; +mod config; +mod executor; +mod graph_utils; +mod migration; +mod schedule; +mod set; +mod state; + +pub use self::condition::*; +pub use self::config::*; +pub use self::executor::*; +use self::graph_utils::*; +pub use self::migration::*; +pub use self::schedule::*; +pub use self::set::*; +pub use self::state::*; + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::{AtomicU32, Ordering}; + + pub use crate as bevy_ecs; + pub use crate::schedule_v3::{IntoSystemConfig, IntoSystemSetConfig, Schedule, SystemSet}; + pub use crate::system::{Res, ResMut}; + pub use crate::{prelude::World, system::Resource}; + + #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)] + enum TestSet { + A, + B, + C, + D, + X, + } + + #[derive(Resource, Default)] + struct SystemOrder(Vec); + + #[derive(Resource, Default)] + struct RunConditionBool(pub bool); + + #[derive(Resource, Default)] + struct Counter(pub AtomicU32); + + fn make_exclusive_system(tag: u32) -> impl FnMut(&mut World) { + move |world| world.resource_mut::().0.push(tag) + } + + fn make_function_system(tag: u32) -> impl FnMut(ResMut) { + move |mut resource: ResMut| resource.0.push(tag) + } + + fn named_system(mut resource: ResMut) { + resource.0.push(u32::MAX); + } + + fn named_exclusive_system(world: &mut World) { + world.resource_mut::().0.push(u32::MAX); + } + + fn counting_system(counter: Res) { + counter.0.fetch_add(1, Ordering::Relaxed); + } + + mod system_execution { + use super::*; + + #[test] + fn run_system() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_system(make_function_system(0)); + schedule.run(&mut world); + + assert_eq!(world.resource::().0, vec![0]); + } + + #[test] + fn run_exclusive_system() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_system(make_exclusive_system(0)); + schedule.run(&mut world); + + assert_eq!(world.resource::().0, vec![0]); + } + + #[test] + #[cfg(not(miri))] + fn parallel_execution() { + use bevy_tasks::{ComputeTaskPool, TaskPool}; + use std::sync::{Arc, Barrier}; + + let mut world = World::default(); + let mut schedule = Schedule::default(); + let thread_count = ComputeTaskPool::init(TaskPool::default).thread_num(); + + let barrier = Arc::new(Barrier::new(thread_count)); + + for _ in 0..thread_count { + let inner = barrier.clone(); + schedule.add_system(move || { + inner.wait(); + }); + } + + schedule.run(&mut world); + } + } + + mod system_ordering { + use super::*; + + #[test] + fn order_systems() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_system(named_system); + schedule.add_system(make_function_system(1).before(named_system)); + schedule.add_system( + make_function_system(0) + .after(named_system) + .in_set(TestSet::A), + ); + schedule.run(&mut world); + + assert_eq!(world.resource::().0, vec![1, u32::MAX, 0]); + + world.insert_resource(SystemOrder::default()); + + assert_eq!(world.resource::().0, vec![]); + + // modify the schedule after it's been initialized and test ordering with sets + schedule.configure_set(TestSet::A.after(named_system)); + schedule.add_system( + make_function_system(3) + .before(TestSet::A) + .after(named_system), + ); + schedule.add_system(make_function_system(4).after(TestSet::A)); + schedule.run(&mut world); + + assert_eq!( + world.resource::().0, + vec![1, u32::MAX, 3, 0, 4] + ); + } + + #[test] + fn order_exclusive_systems() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_system(named_exclusive_system); + schedule.add_system(make_exclusive_system(1).before(named_exclusive_system)); + schedule.add_system(make_exclusive_system(0).after(named_exclusive_system)); + schedule.run(&mut world); + + assert_eq!(world.resource::().0, vec![1, u32::MAX, 0]); + } + + #[test] + fn add_systems_correct_order() { + #[derive(Resource)] + struct X(Vec); + + let mut world = World::new(); + world.init_resource::(); + + let mut schedule = Schedule::new(); + schedule.add_systems( + ( + make_function_system(0), + make_function_system(1), + make_exclusive_system(2), + make_function_system(3), + ) + .chain(), + ); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![0, 1, 2, 3]); + } + } + + mod conditions { + use super::*; + + #[test] + fn system_with_condition() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + world.init_resource::(); + + schedule.add_system( + make_function_system(0).run_if(|condition: Res| condition.0), + ); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![]); + + world.resource_mut::().0 = true; + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![0]); + } + + #[test] + fn run_exclusive_system_with_condition() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + world.init_resource::(); + + schedule.add_system( + make_exclusive_system(0).run_if(|condition: Res| condition.0), + ); + + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![]); + + world.resource_mut::().0 = true; + schedule.run(&mut world); + assert_eq!(world.resource::().0, vec![0]); + } + + #[test] + fn multiple_conditions_on_system() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.add_system(counting_system.run_if(|| false).run_if(|| false)); + schedule.add_system(counting_system.run_if(|| true).run_if(|| false)); + schedule.add_system(counting_system.run_if(|| false).run_if(|| true)); + schedule.add_system(counting_system.run_if(|| true).run_if(|| true)); + + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + } + + #[test] + fn multiple_conditions_on_system_sets() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.configure_set(TestSet::A.run_if(|| false).run_if(|| false)); + schedule.add_system(counting_system.in_set(TestSet::A)); + schedule.configure_set(TestSet::B.run_if(|| true).run_if(|| false)); + schedule.add_system(counting_system.in_set(TestSet::B)); + schedule.configure_set(TestSet::C.run_if(|| false).run_if(|| true)); + schedule.add_system(counting_system.in_set(TestSet::C)); + schedule.configure_set(TestSet::D.run_if(|| true).run_if(|| true)); + schedule.add_system(counting_system.in_set(TestSet::D)); + + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + } + + #[test] + fn systems_nested_in_system_sets() { + let mut world = World::default(); + let mut schedule = Schedule::default(); + + world.init_resource::(); + + schedule.configure_set(TestSet::A.run_if(|| false)); + schedule.add_system(counting_system.in_set(TestSet::A).run_if(|| false)); + schedule.configure_set(TestSet::B.run_if(|| true)); + schedule.add_system(counting_system.in_set(TestSet::B).run_if(|| false)); + schedule.configure_set(TestSet::C.run_if(|| false)); + schedule.add_system(counting_system.in_set(TestSet::C).run_if(|| true)); + schedule.configure_set(TestSet::D.run_if(|| true)); + schedule.add_system(counting_system.in_set(TestSet::D).run_if(|| true)); + + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + } + } + + mod schedule_build_errors { + use super::*; + + #[test] + #[should_panic] + fn dependency_loop() { + let mut schedule = Schedule::new(); + schedule.configure_set(TestSet::X.after(TestSet::X)); + } + + #[test] + fn dependency_cycle() { + let mut world = World::new(); + let mut schedule = Schedule::new(); + + schedule.configure_set(TestSet::A.after(TestSet::B)); + schedule.configure_set(TestSet::B.after(TestSet::A)); + + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::DependencyCycle))); + + fn foo() {} + fn bar() {} + + let mut world = World::new(); + let mut schedule = Schedule::new(); + + schedule.add_systems((foo.after(bar), bar.after(foo))); + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::DependencyCycle))); + } + + #[test] + #[should_panic] + fn hierarchy_loop() { + let mut schedule = Schedule::new(); + schedule.configure_set(TestSet::X.in_set(TestSet::X)); + } + + #[test] + fn hierarchy_cycle() { + let mut world = World::new(); + let mut schedule = Schedule::new(); + + schedule.configure_set(TestSet::A.in_set(TestSet::B)); + schedule.configure_set(TestSet::B.in_set(TestSet::A)); + + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::HierarchyCycle))); + } + + #[test] + fn system_type_set_ambiguity() { + // Define some systems. + fn foo() {} + fn bar() {} + + let mut world = World::new(); + let mut schedule = Schedule::new(); + + // Schedule `bar` to run after `foo`. + schedule.add_system(foo); + schedule.add_system(bar.after(foo)); + + // There's only one `foo`, so it's fine. + let result = schedule.initialize(&mut world); + assert!(result.is_ok()); + + // Schedule another `foo`. + schedule.add_system(foo); + + // When there are multiple instances of `foo`, dependencies on + // `foo` are no longer allowed. Too much ambiguity. + let result = schedule.initialize(&mut world); + assert!(matches!( + result, + Err(ScheduleBuildError::SystemTypeSetAmbiguity(_)) + )); + + // same goes for `ambiguous_with` + let mut schedule = Schedule::new(); + schedule.add_system(foo); + schedule.add_system(bar.ambiguous_with(foo)); + let result = schedule.initialize(&mut world); + assert!(result.is_ok()); + schedule.add_system(foo); + let result = schedule.initialize(&mut world); + assert!(matches!( + result, + Err(ScheduleBuildError::SystemTypeSetAmbiguity(_)) + )); + } + + #[test] + #[should_panic] + fn in_system_type_set() { + fn foo() {} + fn bar() {} + + let mut schedule = Schedule::new(); + schedule.add_system(foo.in_set(bar.into_system_set())); + } + + #[test] + #[should_panic] + fn configure_system_type_set() { + fn foo() {} + let mut schedule = Schedule::new(); + schedule.configure_set(foo.into_system_set()); + } + + #[test] + fn hierarchy_redundancy() { + let mut world = World::new(); + let mut schedule = Schedule::new(); + + schedule.set_build_settings( + ScheduleBuildSettings::new().with_hierarchy_detection(LogLevel::Error), + ); + + // Add `A`. + schedule.configure_set(TestSet::A); + + // Add `B` as child of `A`. + schedule.configure_set(TestSet::B.in_set(TestSet::A)); + + // Add `X` as child of both `A` and `B`. + schedule.configure_set(TestSet::X.in_set(TestSet::A).in_set(TestSet::B)); + + // `X` cannot be the `A`'s child and grandchild at the same time. + let result = schedule.initialize(&mut world); + assert!(matches!( + result, + Err(ScheduleBuildError::HierarchyRedundancy) + )); + } + + #[test] + fn cross_dependency() { + let mut world = World::new(); + let mut schedule = Schedule::new(); + + // Add `B` and give it both kinds of relationships with `A`. + schedule.configure_set(TestSet::B.in_set(TestSet::A)); + schedule.configure_set(TestSet::B.after(TestSet::A)); + let result = schedule.initialize(&mut world); + assert!(matches!( + result, + Err(ScheduleBuildError::CrossDependency(_, _)) + )); + } + + #[test] + fn ambiguity() { + #[derive(Resource)] + struct X; + + fn res_ref(_x: Res) {} + fn res_mut(_x: ResMut) {} + + let mut world = World::new(); + let mut schedule = Schedule::new(); + + schedule.set_build_settings( + ScheduleBuildSettings::new().with_ambiguity_detection(LogLevel::Error), + ); + + schedule.add_systems((res_ref, res_mut)); + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::Ambiguity))); + } + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/schedule.rs b/crates/bevy_ecs/src/schedule_v3/schedule.rs new file mode 100644 index 0000000000000..2bd7b00eca72f --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/schedule.rs @@ -0,0 +1,1099 @@ +use std::{ + fmt::{Debug, Write}, + result::Result, +}; + +use bevy_utils::default; +#[cfg(feature = "trace")] +use bevy_utils::tracing::info_span; +use bevy_utils::{ + petgraph::{algo::tarjan_scc, prelude::*}, + thiserror::Error, + tracing::{error, info, warn}, + HashMap, HashSet, +}; + +use fixedbitset::FixedBitSet; + +use crate::{ + self as bevy_ecs, + component::ComponentId, + schedule_v3::*, + system::{BoxedSystem, Resource}, + world::World, +}; + +/// Resource that stores [`Schedule`]s mapped to [`ScheduleLabel`]s. +#[derive(Default, Resource)] +pub struct Schedules { + inner: HashMap, +} + +impl Schedules { + /// Constructs an empty `Schedules` with zero initial capacity. + pub fn new() -> Self { + Self { + inner: HashMap::new(), + } + } + + /// Inserts a labeled schedule into the map. + /// + /// If the map already had an entry for `label`, `schedule` is inserted, + /// and the old schedule is returned. Otherwise, `None` is returned. + pub fn insert(&mut self, label: impl ScheduleLabel, schedule: Schedule) -> Option { + let label: Box = Box::new(label); + if self.inner.contains_key(&label) { + warn!("schedule with label {:?} already exists", label); + } + self.inner.insert(label, schedule) + } + + /// Removes the schedule corresponding to the `label` from the map, returning it if it existed. + pub fn remove(&mut self, label: &dyn ScheduleLabel) -> Option { + if !self.inner.contains_key(label) { + warn!("schedule with label {:?} not found", label); + } + self.inner.remove(label) + } + + /// Returns a reference to the schedule associated with `label`, if it exists. + pub fn get(&self, label: &dyn ScheduleLabel) -> Option<&Schedule> { + self.inner.get(label) + } + + /// Returns a mutable reference to the schedule associated with `label`, if it exists. + pub fn get_mut(&mut self, label: &dyn ScheduleLabel) -> Option<&mut Schedule> { + self.inner.get_mut(label) + } + + /// Iterates the change ticks of all systems in all stored schedules and clamps any older than + /// [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE). + /// This prevents overflow and thus prevents false positives. + pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { + #[cfg(feature = "trace")] + let _all_span = info_span!("check stored schedule ticks").entered(); + // label used when trace feature is enabled + #[allow(unused_variables)] + for (label, schedule) in self.inner.iter_mut() { + #[cfg(feature = "trace")] + let name = format!("{:?}", label); + #[cfg(feature = "trace")] + let _one_span = info_span!("check schedule ticks", name = &name).entered(); + schedule.check_change_ticks(change_tick); + } + } +} + +/// A collection of systems, and the metadata and executor needed to run them +/// in a certain order under certain conditions. +pub struct Schedule { + graph: ScheduleGraph, + executable: SystemSchedule, + executor: Box, + executor_initialized: bool, +} + +impl Default for Schedule { + fn default() -> Self { + Self::new() + } +} + +impl Schedule { + /// Constructs an empty `Schedule`. + pub fn new() -> Self { + Self { + graph: ScheduleGraph::new(), + executable: SystemSchedule::new(), + executor: Box::new(MultiThreadedExecutor::new()), + executor_initialized: false, + } + } + + /// Add a system to the schedule. + pub fn add_system

(&mut self, system: impl IntoSystemConfig

) -> &mut Self { + self.graph.add_system(system); + self + } + + /// Add a collection of systems to the schedule. + pub fn add_systems

(&mut self, systems: impl IntoSystemConfigs

) -> &mut Self { + self.graph.add_systems(systems); + self + } + + /// Configure a system set in this schedule. + pub fn configure_set(&mut self, set: impl IntoSystemSetConfig) -> &mut Self { + self.graph.configure_set(set); + self + } + + /// Configure a collection of system sets in this schedule. + pub fn configure_sets(&mut self, sets: impl IntoSystemSetConfigs) -> &mut Self { + self.graph.configure_sets(sets); + self + } + + /// Changes the system set that new systems and system sets will join by default + /// if they aren't already part of one. + pub fn set_default_set(&mut self, set: impl SystemSet) -> &mut Self { + self.graph.set_default_set(set); + self + } + + /// Changes miscellaneous build settings. + pub fn set_build_settings(&mut self, settings: ScheduleBuildSettings) -> &mut Self { + self.graph.settings = settings; + self + } + + /// Returns the schedule's current execution strategy. + pub fn get_executor_kind(&self) -> ExecutorKind { + self.executor.kind() + } + + /// Sets the schedule's execution strategy. + pub fn set_executor_kind(&mut self, executor: ExecutorKind) -> &mut Self { + if executor != self.executor.kind() { + self.executor = match executor { + ExecutorKind::Simple => Box::new(SimpleExecutor::new()), + ExecutorKind::SingleThreaded => Box::new(SingleThreadedExecutor::new()), + ExecutorKind::MultiThreaded => Box::new(MultiThreadedExecutor::new()), + }; + self.executor_initialized = false; + } + self + } + + /// Runs all systems in this schedule on the `world`, using its current execution strategy. + pub fn run(&mut self, world: &mut World) { + world.check_change_ticks(); + self.initialize(world).unwrap(); + // TODO: label + #[cfg(feature = "trace")] + let _span = info_span!("schedule").entered(); + self.executor.run(&mut self.executable, world); + } + + /// Initializes any newly-added systems and conditions, rebuilds the executable schedule, + /// and re-initializes the executor. + 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)?; + self.graph.changed = false; + self.executor_initialized = false; + } + + if !self.executor_initialized { + self.executor.init(&self.executable); + self.executor_initialized = true; + } + + Ok(()) + } + + /// Iterates the change ticks of all systems in the schedule and clamps any older than + /// [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE). + /// This prevents overflow and thus prevents false positives. + pub(crate) fn check_change_ticks(&mut self, change_tick: u32) { + for system in &mut self.executable.systems { + system.check_change_tick(change_tick); + } + + for conditions in &mut self.executable.system_conditions { + for system in conditions.iter_mut() { + system.check_change_tick(change_tick); + } + } + + for conditions in &mut self.executable.set_conditions { + for system in conditions.iter_mut() { + system.check_change_tick(change_tick); + } + } + } +} + +/// A directed acylic graph structure. +#[derive(Default)] +struct Dag { + /// A directed graph. + graph: DiGraphMap, + /// A cached topological ordering of the graph. + topsort: Vec, +} + +impl Dag { + fn new() -> Self { + Self { + graph: DiGraphMap::new(), + topsort: Vec::new(), + } + } +} + +/// A [`SystemSet`] with metadata, stored in a [`ScheduleGraph`]. +struct SystemSetNode { + inner: BoxedSystemSet, + /// `true` if this system set was modified with `configure_set` + configured: bool, +} + +impl SystemSetNode { + pub fn new(set: BoxedSystemSet) -> Self { + Self { + inner: set, + configured: false, + } + } + + pub fn name(&self) -> String { + format!("{:?}", &self.inner) + } + + pub fn is_system_type(&self) -> bool { + self.inner.is_system_type() + } +} + +/// Metadata for a [`Schedule`]. +#[derive(Default)] +struct ScheduleGraph { + systems: Vec>, + system_conditions: Vec>>, + system_sets: Vec, + system_set_conditions: Vec>>, + system_set_ids: HashMap, + uninit: Vec<(NodeId, usize)>, + hierarchy: Dag, + dependency: Dag, + dependency_flattened: Dag, + ambiguous_with: UnGraphMap, + ambiguous_with_flattened: UnGraphMap, + ambiguous_with_all: HashSet, + default_set: Option, + changed: bool, + settings: ScheduleBuildSettings, +} + +impl ScheduleGraph { + pub fn new() -> Self { + Self { + systems: Vec::new(), + system_conditions: Vec::new(), + system_sets: Vec::new(), + system_set_conditions: Vec::new(), + system_set_ids: HashMap::new(), + uninit: Vec::new(), + hierarchy: Dag::new(), + dependency: Dag::new(), + dependency_flattened: Dag::new(), + ambiguous_with: UnGraphMap::new(), + ambiguous_with_flattened: UnGraphMap::new(), + ambiguous_with_all: HashSet::new(), + default_set: None, + changed: false, + settings: default(), + } + } + + fn set_default_set(&mut self, set: impl SystemSet) { + assert!( + !set.is_system_type(), + "adding arbitrary systems to a system type set is not allowed" + ); + self.default_set = Some(Box::new(set)); + } + + fn add_systems

(&mut self, systems: impl IntoSystemConfigs

) { + let SystemConfigs { systems, chained } = systems.into_configs(); + let mut system_iter = systems.into_iter(); + if chained { + let Some(prev) = system_iter.next() else { return }; + let mut prev_id = self.add_system_inner(prev).unwrap(); + for next in system_iter { + let next_id = self.add_system_inner(next).unwrap(); + self.dependency.graph.add_edge(prev_id, next_id, ()); + prev_id = next_id; + } + } else { + for system in system_iter { + self.add_system_inner(system).unwrap(); + } + } + } + + fn add_system

(&mut self, system: impl IntoSystemConfig

) { + self.add_system_inner(system).unwrap(); + } + + fn add_system_inner

( + &mut self, + system: impl IntoSystemConfig

, + ) -> Result { + let SystemConfig { + system, + mut graph_info, + conditions, + } = system.into_config(); + + let id = NodeId::System(self.systems.len()); + + if graph_info.sets.is_empty() { + if let Some(default) = self.default_set.as_ref() { + graph_info.sets.push(default.dyn_clone()); + } + } + + // graph updates are immediate + self.update_graphs(id, graph_info)?; + + // system init has to be deferred (need `&mut World`) + self.uninit.push((id, 0)); + self.systems.push(Some(system)); + self.system_conditions.push(Some(conditions)); + + Ok(id) + } + + fn configure_sets(&mut self, sets: impl IntoSystemSetConfigs) { + let SystemSetConfigs { sets, chained } = sets.into_configs(); + let mut set_iter = sets.into_iter(); + if chained { + let Some(prev) = set_iter.next() else { return }; + let mut prev_id = self.configure_set_inner(prev).unwrap(); + for next in set_iter { + let next_id = self.configure_set_inner(next).unwrap(); + self.dependency.graph.add_edge(prev_id, next_id, ()); + prev_id = next_id; + } + } else { + for set in set_iter { + self.configure_set_inner(set).unwrap(); + } + } + } + + fn configure_set(&mut self, set: impl IntoSystemSetConfig) { + self.configure_set_inner(set).unwrap(); + } + + fn configure_set_inner( + &mut self, + set: impl IntoSystemSetConfig, + ) -> Result { + let SystemSetConfig { + set, + mut graph_info, + mut conditions, + } = set.into_config(); + + let id = match self.system_set_ids.get(&set) { + Some(&id) => id, + None => self.add_set(set.dyn_clone()), + }; + + let meta = &mut self.system_sets[id.index()]; + let already_configured = std::mem::replace(&mut meta.configured, true); + + // a system set can be configured multiple times, so this "default check" + // should only happen the first time `configure_set` is called on it + if !already_configured && graph_info.sets.is_empty() { + if let Some(default) = self.default_set.as_ref() { + info!("adding system set `{:?}` to default: `{:?}`", set, default); + graph_info.sets.push(default.dyn_clone()); + } + } + + // graph updates are immediate + self.update_graphs(id, graph_info)?; + + // system init has to be deferred (need `&mut World`) + let system_set_conditions = + self.system_set_conditions[id.index()].get_or_insert_with(Vec::new); + self.uninit.push((id, system_set_conditions.len())); + system_set_conditions.append(&mut conditions); + + Ok(id) + } + + fn add_set(&mut self, set: BoxedSystemSet) -> NodeId { + let id = NodeId::Set(self.system_sets.len()); + self.system_sets.push(SystemSetNode::new(set.dyn_clone())); + self.system_set_conditions.push(None); + self.system_set_ids.insert(set, id); + id + } + + fn check_sets( + &mut self, + id: &NodeId, + graph_info: &GraphInfo, + ) -> Result<(), ScheduleBuildError> { + for set in &graph_info.sets { + match self.system_set_ids.get(set) { + Some(set_id) => { + if id == set_id { + return Err(ScheduleBuildError::HierarchyLoop(set.dyn_clone())); + } + } + None => { + self.add_set(set.dyn_clone()); + } + } + } + + Ok(()) + } + + fn check_edges( + &mut self, + id: &NodeId, + graph_info: &GraphInfo, + ) -> Result<(), ScheduleBuildError> { + for Dependency { kind: _, set } in &graph_info.dependencies { + match self.system_set_ids.get(set) { + Some(set_id) => { + if id == set_id { + return Err(ScheduleBuildError::DependencyLoop(set.dyn_clone())); + } + } + None => { + self.add_set(set.dyn_clone()); + } + } + } + + if let Ambiguity::IgnoreWithSet(ambiguous_with) = &graph_info.ambiguous_with { + for set in ambiguous_with { + if !self.system_set_ids.contains_key(set) { + self.add_set(set.dyn_clone()); + } + } + } + + Ok(()) + } + + fn update_graphs( + &mut self, + id: NodeId, + graph_info: GraphInfo, + ) -> Result<(), ScheduleBuildError> { + self.check_sets(&id, &graph_info)?; + self.check_edges(&id, &graph_info)?; + self.changed = true; + + let GraphInfo { + sets, + dependencies, + ambiguous_with, + } = graph_info; + + if !self.hierarchy.graph.contains_node(id) { + self.hierarchy.graph.add_node(id); + } + + for set in sets.into_iter().map(|set| self.system_set_ids[&set]) { + self.hierarchy.graph.add_edge(set, id, ()); + } + + if !self.dependency.graph.contains_node(id) { + self.dependency.graph.add_node(id); + } + + for (kind, set) in dependencies + .into_iter() + .map(|Dependency { kind, set }| (kind, self.system_set_ids[&set])) + { + let (lhs, rhs) = match kind { + DependencyKind::Before => (id, set), + DependencyKind::After => (set, id), + }; + self.dependency.graph.add_edge(lhs, rhs, ()); + } + + match ambiguous_with { + Ambiguity::Check => (), + Ambiguity::IgnoreWithSet(ambigous_with) => { + for set in ambigous_with + .into_iter() + .map(|set| self.system_set_ids[&set]) + { + self.ambiguous_with.add_edge(id, set, ()); + } + } + Ambiguity::IgnoreAll => { + self.ambiguous_with_all.insert(id); + } + } + + Ok(()) + } + + fn initialize(&mut self, world: &mut World) { + for (id, i) in self.uninit.drain(..) { + match id { + NodeId::System(index) => { + self.systems[index].as_mut().unwrap().initialize(world); + if let Some(v) = self.system_conditions[index].as_mut() { + for condition in v.iter_mut() { + condition.initialize(world); + } + } + } + NodeId::Set(index) => { + if let Some(v) = self.system_set_conditions[index].as_mut() { + for condition in v.iter_mut().skip(i) { + condition.initialize(world); + } + } + } + } + } + } + + fn build_schedule(&mut self) -> Result { + // check hierarchy for cycles + let hier_scc = tarjan_scc(&self.hierarchy.graph); + if self.contains_cycles(&hier_scc) { + self.report_cycles(&hier_scc); + return Err(ScheduleBuildError::HierarchyCycle); + } + + self.hierarchy.topsort = hier_scc.into_iter().flatten().rev().collect::>(); + + let hier_results = check_graph(&self.hierarchy.graph, &self.hierarchy.topsort); + if self.contains_hierarchy_conflicts(&hier_results.transitive_edges) { + self.report_hierarchy_conflicts(&hier_results.transitive_edges); + if matches!(self.settings.hierarchy_detection, LogLevel::Error) { + return Err(ScheduleBuildError::HierarchyRedundancy); + } + } + + // remove redundant edges + self.hierarchy.graph = hier_results.transitive_reduction; + + // check dependencies for cycles + let dep_scc = tarjan_scc(&self.dependency.graph); + if self.contains_cycles(&dep_scc) { + self.report_cycles(&dep_scc); + return Err(ScheduleBuildError::DependencyCycle); + } + + self.dependency.topsort = dep_scc.into_iter().flatten().rev().collect::>(); + + // nodes can have dependent XOR hierarchical relationship + let dep_results = check_graph(&self.dependency.graph, &self.dependency.topsort); + for &(a, b) in dep_results.connected.iter() { + if hier_results.connected.contains(&(a, b)) || hier_results.connected.contains(&(b, a)) + { + let name_a = self.get_node_name(&a); + let name_b = self.get_node_name(&b); + return Err(ScheduleBuildError::CrossDependency(name_a, name_b)); + } + } + + // map system sets to all their member systems + let mut systems_in_sets = HashMap::with_capacity(self.system_sets.len()); + // iterate in reverse topological order (bottom-up) + for &id in self.hierarchy.topsort.iter().rev() { + if id.is_system() { + continue; + } + + let set = id; + systems_in_sets.insert(set, Vec::new()); + + for child in self + .hierarchy + .graph + .neighbors_directed(set, Direction::Outgoing) + { + match child { + NodeId::System(_) => { + systems_in_sets.get_mut(&set).unwrap().push(child); + } + NodeId::Set(_) => { + let [sys, child_sys] = + systems_in_sets.get_many_mut([&set, &child]).unwrap(); + sys.extend_from_slice(child_sys); + } + } + } + } + + // can't depend on or be ambiguous with system type sets that have many instances + for (&set, systems) in systems_in_sets.iter() { + let node = &self.system_sets[set.index()]; + if node.is_system_type() { + let ambiguities = self.ambiguous_with.edges(set).count(); + let mut dependencies = 0; + dependencies += self + .dependency + .graph + .edges_directed(set, Direction::Incoming) + .count(); + dependencies += self + .dependency + .graph + .edges_directed(set, Direction::Outgoing) + .count(); + if systems.len() > 1 && (ambiguities > 0 || dependencies > 0) { + return Err(ScheduleBuildError::SystemTypeSetAmbiguity( + node.inner.dyn_clone(), + )); + } + } + } + + // flatten dependency graph + let mut dependency_flattened = DiGraphMap::new(); + for id in self.dependency.graph.nodes() { + if id.is_system() { + dependency_flattened.add_node(id); + } + } + + for (lhs, rhs, _) in self.dependency.graph.all_edges() { + match (lhs, rhs) { + (NodeId::System(_), NodeId::System(_)) => { + dependency_flattened.add_edge(lhs, rhs, ()); + } + (NodeId::Set(_), NodeId::System(_)) => { + for &lhs_ in &systems_in_sets[&lhs] { + dependency_flattened.add_edge(lhs_, rhs, ()); + } + } + (NodeId::System(_), NodeId::Set(_)) => { + for &rhs_ in &systems_in_sets[&rhs] { + dependency_flattened.add_edge(lhs, rhs_, ()); + } + } + (NodeId::Set(_), NodeId::Set(_)) => { + for &lhs_ in &systems_in_sets[&lhs] { + for &rhs_ in &systems_in_sets[&rhs] { + dependency_flattened.add_edge(lhs_, rhs_, ()); + } + } + } + } + } + + // check flattened dependencies for cycles + let flat_scc = tarjan_scc(&dependency_flattened); + if self.contains_cycles(&flat_scc) { + self.report_cycles(&flat_scc); + return Err(ScheduleBuildError::DependencyCycle); + } + + self.dependency_flattened.graph = dependency_flattened; + self.dependency_flattened.topsort = + flat_scc.into_iter().flatten().rev().collect::>(); + + let flat_results = check_graph( + &self.dependency_flattened.graph, + &self.dependency_flattened.topsort, + ); + + // remove redundant edges + self.dependency_flattened.graph = flat_results.transitive_reduction; + + // flatten allowed ambiguities + let mut ambiguous_with_flattened = UnGraphMap::new(); + for (lhs, rhs, _) in self.ambiguous_with.all_edges() { + match (lhs, rhs) { + (NodeId::System(_), NodeId::System(_)) => { + ambiguous_with_flattened.add_edge(lhs, rhs, ()); + } + (NodeId::Set(_), NodeId::System(_)) => { + for &lhs_ in &systems_in_sets[&lhs] { + ambiguous_with_flattened.add_edge(lhs_, rhs, ()); + } + } + (NodeId::System(_), NodeId::Set(_)) => { + for &rhs_ in &systems_in_sets[&rhs] { + ambiguous_with_flattened.add_edge(lhs, rhs_, ()); + } + } + (NodeId::Set(_), NodeId::Set(_)) => { + for &lhs_ in &systems_in_sets[&lhs] { + for &rhs_ in &systems_in_sets[&rhs] { + ambiguous_with_flattened.add_edge(lhs_, rhs_, ()); + } + } + } + } + } + + self.ambiguous_with_flattened = ambiguous_with_flattened; + + // check for conflicts + let mut conflicting_systems = Vec::new(); + for &(a, b) in flat_results.disconnected.iter() { + if self.ambiguous_with_flattened.contains_edge(a, b) + || self.ambiguous_with_all.contains(&a) + || self.ambiguous_with_all.contains(&b) + { + continue; + } + + let system_a = self.systems[a.index()].as_ref().unwrap(); + let system_b = self.systems[b.index()].as_ref().unwrap(); + if system_a.is_exclusive() || system_b.is_exclusive() { + conflicting_systems.push((a, b, Vec::new())); + } else { + 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)); + } + } + } + + if self.contains_conflicts(&conflicting_systems) { + self.report_conflicts(&conflicting_systems); + if matches!(self.settings.ambiguity_detection, LogLevel::Error) { + return Err(ScheduleBuildError::Ambiguity); + } + } + + // build the schedule + let dg_system_ids = self.dependency_flattened.topsort.clone(); + let dg_system_idx_map = dg_system_ids + .iter() + .cloned() + .enumerate() + .map(|(i, id)| (id, i)) + .collect::>(); + + let hg_systems = self + .hierarchy + .topsort + .iter() + .cloned() + .enumerate() + .filter(|&(_i, id)| id.is_system()) + .collect::>(); + + let (hg_set_idxs, hg_set_ids): (Vec<_>, Vec<_>) = self + .hierarchy + .topsort + .iter() + .cloned() + .enumerate() + .filter(|&(_i, id)| { + // ignore system sets that have no conditions + // ignore system type sets (already covered, they don't have conditions) + id.is_set() + && self.system_set_conditions[id.index()] + .as_ref() + .filter(|v| !v.is_empty()) + .is_some() + }) + .unzip(); + + let sys_count = self.systems.len(); + let set_count = hg_set_ids.len(); + let node_count = self.systems.len() + self.system_sets.len(); + + // get the number of dependencies and the immediate dependents of each system + // (needed by multi-threaded executor to run systems in the correct order) + let mut system_dependencies = Vec::with_capacity(sys_count); + let mut system_dependents = Vec::with_capacity(sys_count); + for &sys_id in &dg_system_ids { + let num_dependencies = self + .dependency_flattened + .graph + .neighbors_directed(sys_id, Direction::Incoming) + .count(); + + let dependents = self + .dependency_flattened + .graph + .neighbors_directed(sys_id, Direction::Outgoing) + .map(|dep_id| dg_system_idx_map[&dep_id]) + .collect::>(); + + system_dependencies.push(num_dependencies); + system_dependents.push(dependents); + } + + // get the rows and columns of the hierarchy graph's reachability matrix + // (needed to we can evaluate conditions in the correct order) + let mut systems_in_sets = vec![FixedBitSet::with_capacity(sys_count); set_count]; + for (i, &row) in hg_set_idxs.iter().enumerate() { + let bitset = &mut systems_in_sets[i]; + for &(col, sys_id) in &hg_systems { + let idx = dg_system_idx_map[&sys_id]; + let is_descendant = hier_results.reachable[index(row, col, node_count)]; + bitset.set(idx, is_descendant); + } + } + + let mut sets_of_systems = vec![FixedBitSet::with_capacity(set_count); sys_count]; + for &(col, sys_id) in &hg_systems { + let i = dg_system_idx_map[&sys_id]; + let bitset = &mut sets_of_systems[i]; + for (idx, &row) in hg_set_idxs + .iter() + .enumerate() + .take_while(|&(_idx, &row)| row < col) + { + let is_ancestor = hier_results.reachable[index(row, col, node_count)]; + bitset.set(idx, is_ancestor); + } + } + + Ok(SystemSchedule { + systems: Vec::with_capacity(sys_count), + system_conditions: Vec::with_capacity(sys_count), + set_conditions: Vec::with_capacity(set_count), + system_ids: dg_system_ids, + set_ids: hg_set_ids, + system_dependencies, + system_dependents, + sets_of_systems, + systems_in_sets, + }) + } + + fn update_schedule(&mut self, schedule: &mut SystemSchedule) -> Result<(), ScheduleBuildError> { + if !self.uninit.is_empty() { + return Err(ScheduleBuildError::Uninitialized); + } + + // move systems out of old schedule + for ((id, system), conditions) in schedule + .system_ids + .drain(..) + .zip(schedule.systems.drain(..)) + .zip(schedule.system_conditions.drain(..)) + { + self.systems[id.index()] = Some(system); + self.system_conditions[id.index()] = Some(conditions); + } + + for (id, conditions) in schedule + .set_ids + .drain(..) + .zip(schedule.set_conditions.drain(..)) + { + self.system_set_conditions[id.index()] = Some(conditions); + } + + *schedule = self.build_schedule()?; + + // move systems into new schedule + for &id in &schedule.system_ids { + let system = self.systems[id.index()].take().unwrap(); + let conditions = self.system_conditions[id.index()].take().unwrap(); + schedule.systems.push(system); + schedule.system_conditions.push(conditions); + } + + for &id in &schedule.set_ids { + let conditions = self.system_set_conditions[id.index()].take().unwrap(); + schedule.set_conditions.push(conditions); + } + + Ok(()) + } +} + +// methods for reporting errors +impl ScheduleGraph { + fn get_node_name(&self, id: &NodeId) -> String { + match id { + NodeId::System(_) => self.systems[id.index()] + .as_ref() + .unwrap() + .name() + .to_string(), + NodeId::Set(_) => self.system_sets[id.index()].name(), + } + } + + fn get_node_kind(id: &NodeId) -> &'static str { + match id { + NodeId::System(_) => "system", + NodeId::Set(_) => "system set", + } + } + + fn contains_hierarchy_conflicts(&self, transitive_edges: &[(NodeId, NodeId)]) -> bool { + if transitive_edges.is_empty() { + return false; + } + + true + } + + fn report_hierarchy_conflicts(&self, transitive_edges: &[(NodeId, NodeId)]) { + let mut message = String::from("hierarchy contains redundant edge(s)"); + for (parent, child) in transitive_edges { + writeln!( + message, + " -- {:?} '{:?}' cannot be child of set '{:?}', longer path exists", + Self::get_node_kind(child), + self.get_node_name(child), + self.get_node_name(parent), + ) + .unwrap(); + } + + error!("{}", message); + } + + fn contains_cycles(&self, strongly_connected_components: &[Vec]) -> bool { + if strongly_connected_components + .iter() + .all(|scc| scc.len() == 1) + { + return false; + } + + true + } + + fn report_cycles(&self, strongly_connected_components: &[Vec]) { + let components_with_cycles = strongly_connected_components + .iter() + .filter(|scc| scc.len() > 1) + .cloned() + .collect::>(); + + let mut message = format!( + "schedule contains at least {} cycle(s)", + components_with_cycles.len() + ); + + writeln!(message, " -- cycle(s) found within:").unwrap(); + for (i, scc) in components_with_cycles.into_iter().enumerate() { + let names = scc + .iter() + .map(|id| self.get_node_name(id)) + .collect::>(); + writeln!(message, " ---- {i}: {names:?}").unwrap(); + } + + error!("{}", message); + } + + fn contains_conflicts(&self, conflicts: &[(NodeId, NodeId, Vec)]) -> bool { + if conflicts.is_empty() { + return false; + } + + true + } + + fn report_conflicts(&self, ambiguities: &[(NodeId, NodeId, Vec)]) { + let mut string = String::from( + "Some systems with conflicting access have indeterminate execution order. \ + Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n", + ); + + for (system_a, system_b, conflicts) in ambiguities { + debug_assert!(system_a.is_system()); + debug_assert!(system_b.is_system()); + let name_a = self.get_node_name(system_a); + let name_b = self.get_node_name(system_b); + + writeln!(string, " -- {name_a} and {name_b}").unwrap(); + if !conflicts.is_empty() { + writeln!(string, " conflict on: {conflicts:?}").unwrap(); + } else { + // one or both systems must be exclusive + let world = std::any::type_name::(); + writeln!(string, " conflict on: {world}").unwrap(); + } + } + + warn!("{}", string); + } +} + +/// Category of errors encountered during schedule construction. +#[derive(Error, Debug)] +#[non_exhaustive] +pub enum ScheduleBuildError { + /// A system set contains itself. + #[error("`{0:?}` contains itself.")] + HierarchyLoop(BoxedSystemSet), + /// The hierarchy of system sets contains a cycle. + #[error("System set hierarchy contains cycle(s).")] + HierarchyCycle, + /// The hierarchy of system sets contains redundant edges. + /// + /// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`]. + #[error("System set hierarchy contains redundant edges.")] + HierarchyRedundancy, + /// A system (set) has been told to run before itself. + #[error("`{0:?}` depends on itself.")] + DependencyLoop(BoxedSystemSet), + /// The dependency graph contains a cycle. + #[error("System dependencies contain cycle(s).")] + DependencyCycle, + /// Tried to order a system (set) relative to a system set it belongs to. + #[error("`{0:?}` and `{1:?}` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.")] + CrossDependency(String, String), + /// Tried to order a system (set) relative to all instances of some system function. + #[error("Tried to order against `fn {0:?}` in a schedule that has more than one `{0:?}` instance. `fn {0:?}` is a `SystemTypeSet` and cannot be used for ordering if ambiguous. Use a different set without this restriction.")] + SystemTypeSetAmbiguity(BoxedSystemSet), + /// Systems with conflicting access have indeterminate run order. + /// + /// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`]. + #[error("Systems with conflicting access have indeterminate run order.")] + Ambiguity, + /// Tried to run a schedule before all of its systems have been initialized. + #[error("Systems in schedule have not been initialized.")] + Uninitialized, +} + +/// Specifies how schedule construction should respond to detecting a certain kind of issue. +pub enum LogLevel { + /// Occurrences are logged only. + Warn, + /// Occurrences are logged and result in errors. + Error, +} + +/// Specifies miscellaneous settings for schedule construction. +pub struct ScheduleBuildSettings { + ambiguity_detection: LogLevel, + hierarchy_detection: LogLevel, +} + +impl Default for ScheduleBuildSettings { + fn default() -> Self { + Self::new() + } +} + +impl ScheduleBuildSettings { + pub const fn new() -> Self { + Self { + ambiguity_detection: LogLevel::Warn, + hierarchy_detection: LogLevel::Warn, + } + } + + /// Determines whether the presence of ambiguities (systems with conflicting access but indeterminate order) + /// is only logged or also results in an [`Ambiguity`](ScheduleBuildError::Ambiguity) error. + pub fn with_ambiguity_detection(mut self, level: LogLevel) -> Self { + self.ambiguity_detection = level; + self + } + + /// Determines whether the presence of redundant edges in the hierarchy of system sets is only + /// logged or also results in a [`HierarchyRedundancy`](ScheduleBuildError::HierarchyRedundancy) + /// error. + pub fn with_hierarchy_detection(mut self, level: LogLevel) -> Self { + self.hierarchy_detection = level; + self + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/set.rs b/crates/bevy_ecs/src/schedule_v3/set.rs new file mode 100644 index 0000000000000..5b29f31e6a3ae --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/set.rs @@ -0,0 +1,149 @@ +use std::fmt::Debug; +use std::hash::{Hash, Hasher}; +use std::marker::PhantomData; + +pub use bevy_ecs_macros::{ScheduleLabel, SystemSet}; +use bevy_utils::define_boxed_label; +use bevy_utils::label::DynHash; + +use crate::system::{ + ExclusiveSystemParam, ExclusiveSystemParamFunction, IsExclusiveFunctionSystem, + IsFunctionSystem, SystemParam, SystemParamFunction, +}; + +define_boxed_label!(ScheduleLabel); + +pub type BoxedSystemSet = Box; +pub type BoxedScheduleLabel = Box; + +/// Types that identify logical groups of systems. +pub trait SystemSet: DynHash + Debug + Send + Sync + 'static { + /// Returns `true` if this system set is a [`SystemTypeSet`]. + fn is_system_type(&self) -> bool { + false + } + + #[doc(hidden)] + fn dyn_clone(&self) -> Box; +} + +impl PartialEq for dyn SystemSet { + fn eq(&self, other: &Self) -> bool { + self.dyn_eq(other.as_dyn_eq()) + } +} + +impl Eq for dyn SystemSet {} + +impl Hash for dyn SystemSet { + fn hash(&self, state: &mut H) { + self.dyn_hash(state); + } +} + +impl Clone for Box { + fn clone(&self) -> Self { + self.dyn_clone() + } +} + +/// A [`SystemSet`] grouping instances of the same function. +/// +/// This kind of set is automatically populated and thus has some special rules: +/// - You cannot manually add members. +/// - You cannot configure them. +/// - You cannot order something relative to one if it has more than one member. +pub struct SystemTypeSet(PhantomData T>); + +impl SystemTypeSet { + pub(crate) fn new() -> Self { + Self(PhantomData) + } +} + +impl Debug for SystemTypeSet { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("SystemTypeSet") + .field(&std::any::type_name::()) + .finish() + } +} + +impl Hash for SystemTypeSet { + fn hash(&self, _state: &mut H) { + // all systems of a given type are the same + } +} +impl Clone for SystemTypeSet { + fn clone(&self) -> Self { + Self(PhantomData) + } +} + +impl Copy for SystemTypeSet {} + +impl PartialEq for SystemTypeSet { + #[inline] + fn eq(&self, _other: &Self) -> bool { + // all systems of a given type are the same + true + } +} + +impl Eq for SystemTypeSet {} + +impl SystemSet for SystemTypeSet { + fn is_system_type(&self) -> bool { + true + } + + fn dyn_clone(&self) -> Box { + Box::new(*self) + } +} + +/// Types that can be converted into a [`SystemSet`]. +pub trait IntoSystemSet: Sized { + type Set: SystemSet; + + fn into_system_set(self) -> Self::Set; +} + +// systems sets +impl IntoSystemSet<()> for S { + type Set = Self; + + #[inline] + fn into_system_set(self) -> Self::Set { + self + } +} + +// systems +impl IntoSystemSet<(IsFunctionSystem, In, Out, Param, Marker)> for F +where + Param: SystemParam, + F: SystemParamFunction, +{ + type Set = SystemTypeSet; + + #[inline] + fn into_system_set(self) -> Self::Set { + SystemTypeSet::new() + } +} + +// exclusive systems +impl IntoSystemSet<(IsExclusiveFunctionSystem, In, Out, Param, Marker)> + for F +where + Param: ExclusiveSystemParam, + F: ExclusiveSystemParamFunction, +{ + type Set = SystemTypeSet; + + #[inline] + fn into_system_set(self) -> Self::Set { + SystemTypeSet::new() + } +} diff --git a/crates/bevy_ecs/src/schedule_v3/state.rs b/crates/bevy_ecs/src/schedule_v3/state.rs new file mode 100644 index 0000000000000..85f357c670f7f --- /dev/null +++ b/crates/bevy_ecs/src/schedule_v3/state.rs @@ -0,0 +1,64 @@ +use std::fmt::Debug; +use std::hash::Hash; +use std::mem; + +use crate as bevy_ecs; +use crate::schedule_v3::{ScheduleLabel, SystemSet, WorldExt}; +use crate::system::Resource; +use crate::world::World; + +/// Types that can define states in a finite-state machine. +pub trait States: 'static + Send + Sync + Clone + PartialEq + Eq + Hash + Debug { + type Iter: Iterator; + + /// Returns an iterator over all the state variants. + fn states() -> Self::Iter; +} + +/// The label of a [`Schedule`](super::Schedule) that runs whenever [`State`] +/// enters this state. +#[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)] +pub struct OnEnter(pub S); + +/// The label of a [`Schedule`](super::Schedule) that runs whenever [`State`] +/// exits this state. +#[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)] +pub struct OnExit(pub S); + +/// A [`SystemSet`] that will run within \ when this state is active. +/// +/// This is provided for convenience. A more general [`state_equals`](super::state_equals) +/// [condition](super::Condition) also exists for systems that need to run elsewhere. +#[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)] +pub struct OnUpdate(pub S); + +/// A finite-state machine whose transitions have associated schedules +/// ([`OnEnter(state)`] and [`OnExit(state)`]). +/// +/// The current state value can be accessed through this resource. To *change* the state, +/// queue a transition in the [`NextState`] resource, and it will be applied by the next +/// [`apply_state_transition::`] system. +#[derive(Resource)] +pub struct State(pub S); + +/// The next state of [`State`]. +/// +/// To queue a transition, just set the contained value to `Some(next_state)`. +#[derive(Resource)] +pub struct NextState(pub Option); + +/// If a new state is queued in [`NextState`], this system: +/// - Takes the new state value from [`NextState`] and updates [`State`]. +/// - Runs the [`OnExit(exited_state)`] schedule. +/// - Runs the [`OnEnter(entered_state)`] schedule. +pub fn apply_state_transition(world: &mut World) { + if world.resource::>().0.is_some() { + let entered_state = world.resource_mut::>().0.take().unwrap(); + let exited_state = mem::replace( + &mut world.resource_mut::>().0, + entered_state.clone(), + ); + world.run_schedule(OnExit(exited_state)); + world.run_schedule(OnEnter(entered_state)); + } +} diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index cda67f67ce53e..1d5c7fca36781 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -11,7 +11,7 @@ use crate::{ world::{World, WorldId}, }; use bevy_ecs_macros::all_tuples; -use std::{borrow::Cow, marker::PhantomData}; +use std::{any::TypeId, borrow::Cow, marker::PhantomData}; /// A function system that runs with exclusive [`World`] access. /// @@ -72,6 +72,11 @@ where self.system_meta.name.clone() } + #[inline] + fn type_id(&self) -> TypeId { + TypeId::of::() + } + #[inline] fn component_access(&self) -> &Access { self.system_meta.component_access_set.combined_access() @@ -149,9 +154,15 @@ where self.system_meta.name.as_ref(), ); } + fn default_labels(&self) -> Vec { vec![self.func.as_system_label().as_label()] } + + fn default_system_sets(&self) -> Vec> { + let set = crate::schedule_v3::SystemTypeSet::::new(); + vec![Box::new(set)] + } } impl AsSystemLabel<(In, Out, Param, Marker, IsExclusiveFunctionSystem)> diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 7ec7c24489380..c46cab36cb215 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -9,7 +9,7 @@ use crate::{ world::{World, WorldId}, }; use bevy_ecs_macros::all_tuples; -use std::{borrow::Cow, fmt::Debug, marker::PhantomData}; +use std::{any::TypeId, borrow::Cow, fmt::Debug, marker::PhantomData}; /// The metadata of a [`System`]. #[derive(Clone)] @@ -368,6 +368,11 @@ where self.system_meta.name.clone() } + #[inline] + fn type_id(&self) -> TypeId { + TypeId::of::() + } + #[inline] fn component_access(&self) -> &Access { self.system_meta.component_access_set.combined_access() @@ -453,9 +458,15 @@ where self.system_meta.name.as_ref(), ); } + fn default_labels(&self) -> Vec { vec![self.func.as_system_label().as_label()] } + + fn default_system_sets(&self) -> Vec> { + let set = crate::schedule_v3::SystemTypeSet::::new(); + vec![Box::new(set)] + } } /// A [`SystemLabel`] that was automatically generated for a system on the basis of its `TypeId`. diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 43fcdb41dd3cb..a6034f0d53829 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -5,6 +5,8 @@ use crate::{ archetype::ArchetypeComponentId, change_detection::MAX_CHANGE_AGE, component::ComponentId, query::Access, schedule::SystemLabelId, world::World, }; + +use std::any::TypeId; use std::borrow::Cow; /// An ECS system that can be added to a [`Schedule`](crate::schedule::Schedule) @@ -26,6 +28,8 @@ pub trait System: Send + Sync + 'static { type Out; /// Returns the system's name. fn name(&self) -> Cow<'static, str>; + /// Returns the [`TypeId`] of the underlying system type. + fn type_id(&self) -> TypeId; /// Returns the system's component [`Access`]. fn component_access(&self) -> &Access; /// Returns the system's archetype component [`Access`]. @@ -64,6 +68,10 @@ pub trait System: Send + Sync + 'static { fn default_labels(&self) -> Vec { Vec::new() } + /// Returns the system's default [system sets](crate::schedule_v3::SystemSet). + fn default_system_sets(&self) -> Vec> { + Vec::new() + } /// Gets the system's last change tick fn get_last_change_tick(&self) -> u32; /// Sets the system's last change tick diff --git a/crates/bevy_ecs/src/system/system_piping.rs b/crates/bevy_ecs/src/system/system_piping.rs index 0af8caa5eed6e..efd545a46a30f 100644 --- a/crates/bevy_ecs/src/system/system_piping.rs +++ b/crates/bevy_ecs/src/system/system_piping.rs @@ -5,7 +5,7 @@ use crate::{ system::{IntoSystem, System}, world::World, }; -use std::borrow::Cow; +use std::{any::TypeId, borrow::Cow}; /// A [`System`] created by piping the output of the first system into the input of the second. /// @@ -77,6 +77,10 @@ impl> System for PipeSystem< self.name.clone() } + fn type_id(&self) -> TypeId { + TypeId::of::<(SystemA, SystemB)>() + } + fn archetype_component_access(&self) -> &Access { &self.archetype_component_access } @@ -141,6 +145,18 @@ impl> System for PipeSystem< self.system_a.set_last_change_tick(last_change_tick); self.system_b.set_last_change_tick(last_change_tick); } + + fn default_labels(&self) -> Vec { + let mut labels = self.system_a.default_labels(); + labels.extend(&self.system_b.default_labels()); + labels + } + + fn default_system_sets(&self) -> Vec> { + let mut system_sets = self.system_a.default_system_sets(); + system_sets.extend_from_slice(&self.system_b.default_system_sets()); + system_sets + } } /// An extension trait providing the [`IntoPipeSystem::pipe`] method to pass input from one system into the next. diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index af4807344660c..f8b41c8b34c43 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -2,7 +2,7 @@ mod entity_ref; mod spawn_batch; mod world_cell; -pub use crate::change_detection::{Mut, Ref}; +pub use crate::change_detection::{Mut, Ref, CHECK_TICK_THRESHOLD}; pub use entity_ref::{EntityMut, EntityRef}; pub use spawn_batch::*; pub use world_cell::*; @@ -64,6 +64,7 @@ pub struct World { pub(crate) archetype_component_access: ArchetypeComponentAccess, pub(crate) change_tick: AtomicU32, pub(crate) last_change_tick: u32, + pub(crate) last_check_tick: u32, } impl Default for World { @@ -81,6 +82,7 @@ impl Default for World { // are detected on first system runs and for direct world queries. change_tick: AtomicU32::new(1), last_change_tick: 0, + last_check_tick: 0, } } } @@ -1589,20 +1591,37 @@ impl World { self.last_change_tick } + /// Iterates all component change ticks and clamps any older than [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE). + /// This prevents overflow and thus prevents false positives. + /// + /// **Note:** Does nothing if the [`World`] counter has not been incremented at least [`CHECK_TICK_THRESHOLD`](crate::change_detection::CHECK_TICK_THRESHOLD) + /// times since the previous pass. + // TODO: benchmark and optimize pub fn check_change_ticks(&mut self) { - // Iterate over all component change ticks, clamping their age to max age - // PERF: parallelize let change_tick = self.change_tick(); + if change_tick.wrapping_sub(self.last_check_tick) < CHECK_TICK_THRESHOLD { + return; + } + let Storages { ref mut tables, ref mut sparse_sets, ref mut resources, ref mut non_send_resources, } = self.storages; + + #[cfg(feature = "trace")] + let _span = bevy_utils::tracing::info_span!("check component ticks").entered(); tables.check_change_ticks(change_tick); sparse_sets.check_change_ticks(change_tick); resources.check_change_ticks(change_tick); non_send_resources.check_change_ticks(change_tick); + + if let Some(mut schedules) = self.get_resource_mut::() { + schedules.check_change_ticks(change_tick); + } + + self.last_check_tick = change_tick; } pub fn clear_entities(&mut self) { diff --git a/crates/bevy_macro_utils/src/lib.rs b/crates/bevy_macro_utils/src/lib.rs index 29f15461221c5..890e3b468a310 100644 --- a/crates/bevy_macro_utils/src/lib.rs +++ b/crates/bevy_macro_utils/src/lib.rs @@ -117,6 +117,70 @@ impl BevyManifest { } } +/// Derive a label trait +/// +/// # Args +/// +/// - `input`: The [`syn::DeriveInput`] for struct that is deriving the label trait +/// - `trait_path`: The path [`syn::Path`] to the label trait +pub fn derive_boxed_label(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStream { + let ident = input.ident; + let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); + let mut where_clause = where_clause.cloned().unwrap_or_else(|| syn::WhereClause { + where_token: Default::default(), + predicates: Default::default(), + }); + where_clause.predicates.push( + syn::parse2(quote! { + Self: 'static + Send + Sync + Clone + Eq + ::std::fmt::Debug + ::std::hash::Hash + }) + .unwrap(), + ); + + (quote! { + impl #impl_generics #trait_path for #ident #ty_generics #where_clause { + fn dyn_clone(&self) -> std::boxed::Box { + std::boxed::Box::new(std::clone::Clone::clone(self)) + } + } + }) + .into() +} + +/// Derive a label trait +/// +/// # Args +/// +/// - `input`: The [`syn::DeriveInput`] for struct that is deriving the label trait +/// - `trait_path`: The path [`syn::Path`] to the label trait +pub fn derive_set(input: syn::DeriveInput, trait_path: &syn::Path) -> TokenStream { + let ident = input.ident; + let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); + let mut where_clause = where_clause.cloned().unwrap_or_else(|| syn::WhereClause { + where_token: Default::default(), + predicates: Default::default(), + }); + where_clause.predicates.push( + syn::parse2(quote! { + Self: 'static + Send + Sync + Clone + Eq + ::std::fmt::Debug + ::std::hash::Hash + }) + .unwrap(), + ); + + (quote! { + impl #impl_generics #trait_path for #ident #ty_generics #where_clause { + fn is_system_type(&self) -> bool { + false + } + + fn dyn_clone(&self) -> std::boxed::Box { + std::boxed::Box::new(std::clone::Clone::clone(self)) + } + } + }) + .into() +} + /// Derive a label trait /// /// # Args diff --git a/crates/bevy_time/src/fixed_timestep.rs b/crates/bevy_time/src/fixed_timestep.rs index 8a63f67f92bb6..fa355706581bb 100644 --- a/crates/bevy_time/src/fixed_timestep.rs +++ b/crates/bevy_time/src/fixed_timestep.rs @@ -177,6 +177,10 @@ impl System for FixedTimestep { Cow::Borrowed(std::any::type_name::()) } + fn type_id(&self) -> std::any::TypeId { + std::any::TypeId::of::() + } + fn archetype_component_access(&self) -> &Access { self.internal_system.archetype_component_access() } diff --git a/crates/bevy_utils/Cargo.toml b/crates/bevy_utils/Cargo.toml index a61ce8d16594d..5a019dbc38455 100644 --- a/crates/bevy_utils/Cargo.toml +++ b/crates/bevy_utils/Cargo.toml @@ -14,6 +14,8 @@ tracing = { version = "0.1", default-features = false, features = ["std"] } instant = { version = "0.1", features = ["wasm-bindgen"] } uuid = { version = "1.1", features = ["v4", "serde"] } hashbrown = { version = "0.12", features = ["serde"] } +petgraph = "0.6" +thiserror = "1.0" [target.'cfg(target_arch = "wasm32")'.dependencies] getrandom = {version = "0.2.0", features = ["js"]} diff --git a/crates/bevy_utils/src/label.rs b/crates/bevy_utils/src/label.rs index 6c504b0283a30..d3e816552bb1e 100644 --- a/crates/bevy_utils/src/label.rs +++ b/crates/bevy_utils/src/label.rs @@ -60,6 +60,47 @@ where } } +/// Macro to define a new label trait +/// +/// # Example +/// +/// ``` +/// # use bevy_utils::define_boxed_label; +/// define_boxed_label!(MyNewLabelTrait); +/// ``` +#[macro_export] +macro_rules! define_boxed_label { + ($label_trait_name:ident) => { + /// A strongly-typed label. + pub trait $label_trait_name: + 'static + Send + Sync + ::std::fmt::Debug + ::bevy_utils::label::DynHash + { + #[doc(hidden)] + fn dyn_clone(&self) -> Box; + } + + impl PartialEq for dyn $label_trait_name { + fn eq(&self, other: &Self) -> bool { + self.dyn_eq(other.as_dyn_eq()) + } + } + + impl Eq for dyn $label_trait_name {} + + impl ::std::hash::Hash for dyn $label_trait_name { + fn hash(&self, state: &mut H) { + self.dyn_hash(state); + } + } + + impl Clone for Box { + fn clone(&self) -> Self { + self.dyn_clone() + } + } + }; +} + /// Macro to define a new label trait /// /// # Example diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index e54065eb5f6bf..a6a4aebcb254c 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -15,6 +15,7 @@ pub mod label; mod short_names; pub use short_names::get_short_name; pub mod synccell; +pub mod syncunsafecell; mod default; mod float_ord; @@ -24,6 +25,8 @@ pub use default::default; pub use float_ord::*; pub use hashbrown; pub use instant::{Duration, Instant}; +pub use petgraph; +pub use thiserror; pub use tracing; pub use uuid::Uuid; diff --git a/crates/bevy_utils/src/syncunsafecell.rs b/crates/bevy_utils/src/syncunsafecell.rs new file mode 100644 index 0000000000000..07e2422d89a8a --- /dev/null +++ b/crates/bevy_utils/src/syncunsafecell.rs @@ -0,0 +1,122 @@ +//! A reimplementation of the currently unstable [`std::cell::SyncUnsafeCell`] +//! +//! [`std::cell::SyncUnsafeCell`]: https://doc.rust-lang.org/nightly/std/cell/struct.SyncUnsafeCell.html + +pub use core::cell::UnsafeCell; + +/// [`UnsafeCell`], but [`Sync`]. +/// +/// See [tracking issue](https://github.com/rust-lang/rust/issues/95439) for upcoming native impl, +/// which should replace this one entirely (except `from_mut`). +/// +/// This is just an `UnsafeCell`, except it implements `Sync` +/// if `T` implements `Sync`. +/// +/// `UnsafeCell` doesn't implement `Sync`, to prevent accidental mis-use. +/// You can use `SyncUnsafeCell` instead of `UnsafeCell` to allow it to be +/// shared between threads, if that's intentional. +/// Providing proper synchronization is still the task of the user, +/// making this type just as unsafe to use. +/// +/// See [`UnsafeCell`] for details. +#[repr(transparent)] +pub struct SyncUnsafeCell { + value: UnsafeCell, +} + +// SAFETY: `T` is Sync, caller is responsible for upholding rust safety rules +unsafe impl Sync for SyncUnsafeCell {} + +impl SyncUnsafeCell { + /// Constructs a new instance of `SyncUnsafeCell` which will wrap the specified value. + #[inline] + pub const fn new(value: T) -> Self { + Self { + value: UnsafeCell::new(value), + } + } + + /// Unwraps the value. + #[inline] + pub fn into_inner(self) -> T { + self.value.into_inner() + } +} + +impl SyncUnsafeCell { + /// Gets a mutable pointer to the wrapped value. + /// + /// This can be cast to a pointer of any kind. + /// Ensure that the access is unique (no active references, mutable or not) + /// when casting to `&mut T`, and ensure that there are no mutations + /// or mutable aliases going on when casting to `&T` + #[inline] + pub const fn get(&self) -> *mut T { + self.value.get() + } + + /// Returns a mutable reference to the underlying data. + /// + /// This call borrows the `SyncUnsafeCell` mutably (at compile-time) which + /// guarantees that we possess the only reference. + #[inline] + pub fn get_mut(&mut self) -> &mut T { + self.value.get_mut() + } + + /// Gets a mutable pointer to the wrapped value. + /// + /// See [`UnsafeCell::get`] for details. + #[inline] + pub const fn raw_get(this: *const Self) -> *mut T { + // We can just cast the pointer from `SyncUnsafeCell` to `T` because + // of #[repr(transparent)] on both SyncUnsafeCell and UnsafeCell. + // See UnsafeCell::raw_get. + this as *const T as *mut T + } + + #[inline] + /// Returns a `&SyncUnsafeCell` from a `&mut T`. + pub fn from_mut(t: &mut T) -> &SyncUnsafeCell { + // SAFETY: `&mut` ensures unique access, and `UnsafeCell` and `SyncUnsafeCell` + // have #[repr(transparent)] + unsafe { &*(t as *mut T as *const SyncUnsafeCell) } + } +} + +impl SyncUnsafeCell<[T]> { + /// Returns a `&[SyncUnsafeCell]` from a `&SyncUnsafeCell<[T]>`. + /// # Examples + /// + /// ``` + /// # use bevy_utils::syncunsafecell::SyncUnsafeCell; + /// + /// let slice: &mut [i32] = &mut [1, 2, 3]; + /// let cell_slice: &SyncUnsafeCell<[i32]> = SyncUnsafeCell::from_mut(slice); + /// let slice_cell: &[SyncUnsafeCell] = cell_slice.as_slice_of_cells(); + /// + /// assert_eq!(slice_cell.len(), 3); + /// ``` + pub fn as_slice_of_cells(&self) -> &[SyncUnsafeCell] { + // SAFETY: `UnsafeCell` and `SyncUnsafeCell` have #[repr(transparent)] + // therefore: + // - `SyncUnsafeCell` has the same layout as `T` + // - `SyncUnsafeCell<[T]>` has the same layout as `[T]` + // - `SyncUnsafeCell<[T]>` has the same layout as `[SyncUnsafeCell]` + unsafe { &*(self as *const SyncUnsafeCell<[T]> as *const [SyncUnsafeCell]) } + } +} + +impl Default for SyncUnsafeCell { + /// Creates an `SyncUnsafeCell`, with the `Default` value for T. + fn default() -> SyncUnsafeCell { + SyncUnsafeCell::new(Default::default()) + } +} + +impl From for SyncUnsafeCell { + /// Creates a new `SyncUnsafeCell` containing the given value. + fn from(t: T) -> SyncUnsafeCell { + SyncUnsafeCell::new(t) + } +} From 0efe66b081e0e8ea5bf089b7d2394598a9774993 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 17 Jan 2023 01:39:19 +0000 Subject: [PATCH 08/24] Remove an incorrect impl of `ReadOnlySystemParam` for `NonSendMut` (#7243) # Objective The trait `ReadOnlySystemParam` is implemented for `NonSendMut`, when it should not be. This mistake was made in #6919. ## Solution Remove the incorrect impl. --- crates/bevy_ecs/src/system/system_param.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index e08326f8868a7..921bba36b2710 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1012,9 +1012,6 @@ unsafe impl SystemParam for Option> { } } -// SAFETY: Only reads a single non-send resource -unsafe impl<'a, T: 'static> ReadOnlySystemParam for NonSendMut<'a, T> {} - // SAFETY: NonSendMut ComponentId and ArchetypeComponentId access is applied to SystemMeta. If this // NonSendMut conflicts with any prior access, a panic will occur. unsafe impl<'a, T: 'static> SystemParam for NonSendMut<'a, T> { From b5893e570d2a68471d2f3d147751dcc33bac32e0 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 17 Jan 2023 03:29:08 +0000 Subject: [PATCH 09/24] Add a missing impl of `ReadOnlySystemParam` for `Option>` (#7245) # Objective The trait `ReadOnlySystemParam` is not implemented for `Option>`, even though it should be. Follow-up to #7243. This fixes another mistake made in #6919. ## Solution Add the missing impl. --- crates/bevy_ecs/src/system/system_param.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 921bba36b2710..9cb7fb3dfc8b2 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -985,6 +985,9 @@ unsafe impl<'a, T: 'static> SystemParam for NonSend<'a, T> { } } +// SAFETY: Only reads a single World non-send resource +unsafe impl ReadOnlySystemParam for Option> {} + // SAFETY: this impl defers to `NonSend`, which initializes and validates the correct world access. unsafe impl SystemParam for Option> { type State = ComponentId; From 16ff05acdf2f04c37059c76dc87457d36f78c2f8 Mon Sep 17 00:00:00 2001 From: 2ne1ugly Date: Tue, 17 Jan 2023 04:20:42 +0000 Subject: [PATCH 10/24] Add `World::clear_resources` & `World::clear_all` (#3212) # Objective - Fixes #3158 ## Solution - clear columns My implementation of `clear_resources` do not remove the components itself but it clears the columns that keeps the resource data. I'm not sure if the issue meant to clear all resources, even the components and component ids (which I'm not sure if it's possible) Co-authored-by: 2ne1ugly <47616772+2ne1ugly@users.noreply.github.com> --- crates/bevy_ecs/src/storage/resource.rs | 6 ++++++ crates/bevy_ecs/src/storage/sparse_set.rs | 6 ++++++ crates/bevy_ecs/src/world/mod.rs | 20 ++++++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/crates/bevy_ecs/src/storage/resource.rs b/crates/bevy_ecs/src/storage/resource.rs index 4a9b0eb3ce4b8..f1a0e9fb53552 100644 --- a/crates/bevy_ecs/src/storage/resource.rs +++ b/crates/bevy_ecs/src/storage/resource.rs @@ -236,6 +236,12 @@ impl Resources { self.resources.get(component_id) } + /// Clears all resources. + #[inline] + pub fn clear(&mut self) { + self.resources.clear(); + } + /// Gets mutable access to a resource, if it exists. #[inline] pub(crate) fn get_mut(&mut self, component_id: ComponentId) -> Option<&mut ResourceData> { diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index cc3da14682cb9..d145122c33456 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -447,6 +447,12 @@ impl SparseSet { }) } + pub fn clear(&mut self) { + self.dense.clear(); + self.indices.clear(); + self.sparse.clear(); + } + pub(crate) fn into_immutable(self) -> ImmutableSparseSet { ImmutableSparseSet { dense: self.dense.into_boxed_slice(), diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index f8b41c8b34c43..066268526e953 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1624,12 +1624,32 @@ impl World { self.last_check_tick = change_tick; } + /// Runs both [`clear_entities`](Self::clear_entities) and [`clear_resources`](Self::clear_resources), + /// invalidating all [`Entity`] and resource fetches such as [`Res`](crate::system::Res), [`ResMut`](crate::system::ResMut) + pub fn clear_all(&mut self) { + self.clear_entities(); + self.clear_resources(); + } + + /// Despawns all entities in this [`World`]. pub fn clear_entities(&mut self) { self.storages.tables.clear(); self.storages.sparse_sets.clear(); self.archetypes.clear_entities(); self.entities.clear(); } + + /// Clears all resources in this [`World`]. + /// + /// **Note:** Any resource fetch to this [World] will fail unless they are re-initialized, + /// including engine-internal resources that are only initialized on app/world construction. + /// + /// This can easily cause systems expecting certain resources to immediately start panicking. + /// Use with caution. + pub fn clear_resources(&mut self) { + self.storages.resources.clear(); + self.storages.non_send_resources.clear(); + } } impl World { From 1cc663f2909e8d8a989668383bb0c3c5f034a816 Mon Sep 17 00:00:00 2001 From: wyhaya Date: Tue, 17 Jan 2023 13:26:43 +0000 Subject: [PATCH 11/24] Improve `Color::hex` performance (#6940) # Objective Improve `Color::hex` performance #### Bench ```bash running 2 tests test bench_color_hex_after ... bench: 4 ns/iter (+/- 0) test bench_color_hex_before ... bench: 14 ns/iter (+/- 0) ``` ## Solution Use `const fn` decode hex value. --- ## Changelog Rename ```rust HexColorError::Hex(FromHexError) -> HexColorError::Char(char) ``` --- crates/bevy_render/Cargo.toml | 1 - crates/bevy_render/src/color/mod.rs | 145 ++++++++++++++-------------- 2 files changed, 70 insertions(+), 76 deletions(-) diff --git a/crates/bevy_render/Cargo.toml b/crates/bevy_render/Cargo.toml index eaa4d09af6bac..8fca30904838e 100644 --- a/crates/bevy_render/Cargo.toml +++ b/crates/bevy_render/Cargo.toml @@ -62,7 +62,6 @@ thread_local = "1.1" thiserror = "1.0" futures-lite = "1.4.0" anyhow = "1.0" -hex = "0.4.2" hexasphere = "8.0" parking_lot = "0.12.1" regex = "1.5" diff --git a/crates/bevy_render/src/color/mod.rs b/crates/bevy_render/src/color/mod.rs index 3299f7f7b790c..18817aec03f3f 100644 --- a/crates/bevy_render/src/color/mod.rs +++ b/crates/bevy_render/src/color/mod.rs @@ -260,37 +260,29 @@ impl Color { let hex = hex.as_ref(); let hex = hex.strip_prefix('#').unwrap_or(hex); - // RGB - if hex.len() == 3 { - let mut data = [0; 6]; - for (i, ch) in hex.chars().enumerate() { - data[i * 2] = ch as u8; - data[i * 2 + 1] = ch as u8; + match *hex.as_bytes() { + // RGB + [r, g, b] => { + let [r, g, b, ..] = decode_hex([r, r, g, g, b, b])?; + Ok(Color::rgb_u8(r, g, b)) } - return decode_rgb(&data); - } - - // RGBA - if hex.len() == 4 { - let mut data = [0; 8]; - for (i, ch) in hex.chars().enumerate() { - data[i * 2] = ch as u8; - data[i * 2 + 1] = ch as u8; + // RGBA + [r, g, b, a] => { + let [r, g, b, a, ..] = decode_hex([r, r, g, g, b, b, a, a])?; + Ok(Color::rgba_u8(r, g, b, a)) } - return decode_rgba(&data); - } - - // RRGGBB - if hex.len() == 6 { - return decode_rgb(hex.as_bytes()); - } - - // RRGGBBAA - if hex.len() == 8 { - return decode_rgba(hex.as_bytes()); + // RRGGBB + [r1, r2, g1, g2, b1, b2] => { + let [r, g, b, ..] = decode_hex([r1, r2, g1, g2, b1, b2])?; + Ok(Color::rgb_u8(r, g, b)) + } + // RRGGBBAA + [r1, r2, g1, g2, b1, b2, a1, a2] => { + let [r, g, b, a, ..] = decode_hex([r1, r2, g1, g2, b1, b2, a1, a2])?; + Ok(Color::rgba_u8(r, g, b, a)) + } + _ => Err(HexColorError::Length), } - - Err(HexColorError::Length) } /// New `Color` from sRGB colorspace. @@ -1337,38 +1329,49 @@ impl encase::private::CreateFrom for Color { impl encase::ShaderSize for Color {} -#[derive(Debug, Error)] +#[derive(Debug, Error, PartialEq, Eq)] pub enum HexColorError { #[error("Unexpected length of hex string")] Length, - #[error("Error parsing hex value")] - Hex(#[from] hex::FromHexError), + #[error("Invalid hex char")] + Char(char), } -fn decode_rgb(data: &[u8]) -> Result { - let mut buf = [0; 3]; - match hex::decode_to_slice(data, &mut buf) { - Ok(_) => { - let r = buf[0] as f32 / 255.0; - let g = buf[1] as f32 / 255.0; - let b = buf[2] as f32 / 255.0; - Ok(Color::rgb(r, g, b)) - } - Err(err) => Err(HexColorError::Hex(err)), +/// Converts hex bytes to an array of RGB\[A\] components +/// +/// # Example +/// For RGB: *b"ffffff" -> [255, 255, 255, ..] +/// For RGBA: *b"E2E2E2FF" -> [226, 226, 226, 255, ..] +const fn decode_hex(mut bytes: [u8; N]) -> Result<[u8; N], HexColorError> { + let mut i = 0; + while i < bytes.len() { + // Convert single hex digit to u8 + let val = match hex_value(bytes[i]) { + Ok(val) => val, + Err(byte) => return Err(HexColorError::Char(byte as char)), + }; + bytes[i] = val; + i += 1; } + // Modify the original bytes to give an `N / 2` length result + i = 0; + while i < bytes.len() / 2 { + // Convert pairs of u8 to R/G/B/A + // e.g `ff` -> [102, 102] -> [15, 15] = 255 + bytes[i] = bytes[i * 2] * 16 + bytes[i * 2 + 1]; + i += 1; + } + Ok(bytes) } -fn decode_rgba(data: &[u8]) -> Result { - let mut buf = [0; 4]; - match hex::decode_to_slice(data, &mut buf) { - Ok(_) => { - let r = buf[0] as f32 / 255.0; - let g = buf[1] as f32 / 255.0; - let b = buf[2] as f32 / 255.0; - let a = buf[3] as f32 / 255.0; - Ok(Color::rgba(r, g, b, a)) - } - Err(err) => Err(HexColorError::Hex(err)), +/// Parse a single hex digit (a-f/A-F/0-9) as a `u8` +const fn hex_value(b: u8) -> Result { + match b { + b'0'..=b'9' => Ok(b - b'0'), + b'A'..=b'F' => Ok(b - b'A' + 10), + b'a'..=b'f' => Ok(b - b'a' + 10), + // Wrong hex digit + _ => Err(b), } } @@ -1378,29 +1381,21 @@ mod tests { #[test] fn hex_color() { - assert_eq!(Color::hex("FFF").unwrap(), Color::rgb(1.0, 1.0, 1.0)); - assert_eq!(Color::hex("000").unwrap(), Color::rgb(0.0, 0.0, 0.0)); - assert!(Color::hex("---").is_err()); - - assert_eq!(Color::hex("FFFF").unwrap(), Color::rgba(1.0, 1.0, 1.0, 1.0)); - assert_eq!(Color::hex("0000").unwrap(), Color::rgba(0.0, 0.0, 0.0, 0.0)); - assert!(Color::hex("----").is_err()); - - assert_eq!(Color::hex("FFFFFF").unwrap(), Color::rgb(1.0, 1.0, 1.0)); - assert_eq!(Color::hex("000000").unwrap(), Color::rgb(0.0, 0.0, 0.0)); - assert!(Color::hex("------").is_err()); - - assert_eq!( - Color::hex("FFFFFFFF").unwrap(), - Color::rgba(1.0, 1.0, 1.0, 1.0) - ); - assert_eq!( - Color::hex("00000000").unwrap(), - Color::rgba(0.0, 0.0, 0.0, 0.0) - ); - assert!(Color::hex("--------").is_err()); - - assert!(Color::hex("1234567890").is_err()); + assert_eq!(Color::hex("FFF"), Ok(Color::WHITE)); + assert_eq!(Color::hex("FFFF"), Ok(Color::WHITE)); + assert_eq!(Color::hex("FFFFFF"), Ok(Color::WHITE)); + assert_eq!(Color::hex("FFFFFFFF"), Ok(Color::WHITE)); + assert_eq!(Color::hex("000"), Ok(Color::BLACK)); + assert_eq!(Color::hex("000F"), Ok(Color::BLACK)); + assert_eq!(Color::hex("000000"), Ok(Color::BLACK)); + assert_eq!(Color::hex("000000FF"), Ok(Color::BLACK)); + assert_eq!(Color::hex("03a9f4"), Ok(Color::rgb_u8(3, 169, 244))); + assert_eq!(Color::hex("yy"), Err(HexColorError::Length)); + assert_eq!(Color::hex("yyy"), Err(HexColorError::Char('y'))); + assert_eq!(Color::hex("#f2a"), Ok(Color::rgb_u8(255, 34, 170))); + assert_eq!(Color::hex("#e23030"), Ok(Color::rgb_u8(226, 48, 48))); + assert_eq!(Color::hex("#ff"), Err(HexColorError::Length)); + assert_eq!(Color::hex("##fff"), Err(HexColorError::Char('#'))); } #[test] From 45dfa71e032fef2827f154f60928da84e11cc92d Mon Sep 17 00:00:00 2001 From: robtfm <50659922+robtfm@users.noreply.github.com> Date: Tue, 17 Jan 2023 17:39:28 +0000 Subject: [PATCH 12/24] fix bloom viewport (#6802) # Objective fix bloom when used on a camera with a viewport specified ## Solution - pass viewport into the prefilter shader, and use it to read from the correct section of the original rendered screen - don't apply viewport for the intermediate bloom passes, only for the final blend output --- .../bevy_core_pipeline/src/bloom/bloom.wgsl | 6 ++-- crates/bevy_core_pipeline/src/bloom/mod.rs | 29 ++++++++++--------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/crates/bevy_core_pipeline/src/bloom/bloom.wgsl b/crates/bevy_core_pipeline/src/bloom/bloom.wgsl index 0e6addeef3ee3..a6bab621db520 100644 --- a/crates/bevy_core_pipeline/src/bloom/bloom.wgsl +++ b/crates/bevy_core_pipeline/src/bloom/bloom.wgsl @@ -5,6 +5,7 @@ struct BloomUniforms { knee: f32, scale: f32, intensity: f32, + viewport: vec4, }; @group(0) @binding(0) @@ -87,7 +88,8 @@ fn sample_original_3x3_tent(uv: vec2, scale: vec2) -> vec4 { } @fragment -fn downsample_prefilter(@location(0) uv: vec2) -> @location(0) vec4 { +fn downsample_prefilter(@location(0) output_uv: vec2) -> @location(0) vec4 { + let sample_uv = uniforms.viewport.xy + output_uv * uniforms.viewport.zw; let texel_size = 1.0 / vec2(textureDimensions(original)); let scale = texel_size; @@ -98,7 +100,7 @@ fn downsample_prefilter(@location(0) uv: vec2) -> @location(0) vec4 { 0.25 / uniforms.knee, ); - var o: vec4 = sample_13_tap(uv, scale); + var o: vec4 = sample_13_tap(sample_uv, scale); o = quadratic_threshold(o, uniforms.threshold, curve); o = max(o, vec4(0.00001)); diff --git a/crates/bevy_core_pipeline/src/bloom/mod.rs b/crates/bevy_core_pipeline/src/bloom/mod.rs index 3ceb6a2645581..ad438b2a25dcf 100644 --- a/crates/bevy_core_pipeline/src/bloom/mod.rs +++ b/crates/bevy_core_pipeline/src/bloom/mod.rs @@ -7,7 +7,7 @@ use bevy_ecs::{ system::{Commands, Query, Res, ResMut, Resource}, world::{FromWorld, World}, }; -use bevy_math::UVec2; +use bevy_math::{UVec2, UVec4, Vec4}; use bevy_reflect::{Reflect, TypeUuid}; use bevy_render::{ camera::ExtractedCamera, @@ -152,18 +152,27 @@ impl ExtractComponent for BloomSettings { return None; } - camera.physical_viewport_size().map(|size| { + if let (Some((origin, _)), Some(size), Some(target_size)) = ( + camera.physical_viewport_rect(), + camera.physical_viewport_size(), + camera.physical_target_size(), + ) { let min_view = size.x.min(size.y) / 2; let mip_count = calculate_mip_count(min_view); let scale = (min_view / 2u32.pow(mip_count)) as f32 / 8.0; - BloomUniform { + Some(BloomUniform { threshold: settings.threshold, knee: settings.knee, scale: settings.scale * scale, intensity: settings.intensity, - } - }) + viewport: UVec4::new(origin.x, origin.y, size.x, size.y).as_vec4() + / UVec4::new(target_size.x, target_size.y, target_size.x, target_size.y) + .as_vec4(), + }) + } else { + None + } } } @@ -246,9 +255,6 @@ impl Node for BloomNode { &bind_groups.prefilter_bind_group, &[uniform_index.index()], ); - if let Some(viewport) = camera.viewport.as_ref() { - prefilter_pass.set_camera_viewport(viewport); - } prefilter_pass.draw(0..3, 0..1); } @@ -270,9 +276,6 @@ impl Node for BloomNode { &bind_groups.downsampling_bind_groups[mip as usize - 1], &[uniform_index.index()], ); - if let Some(viewport) = camera.viewport.as_ref() { - downsampling_pass.set_camera_viewport(viewport); - } downsampling_pass.draw(0..3, 0..1); } @@ -294,9 +297,6 @@ impl Node for BloomNode { &bind_groups.upsampling_bind_groups[mip as usize - 1], &[uniform_index.index()], ); - if let Some(viewport) = camera.viewport.as_ref() { - upsampling_pass.set_camera_viewport(viewport); - } upsampling_pass.draw(0..3, 0..1); } @@ -612,6 +612,7 @@ pub struct BloomUniform { knee: f32, scale: f32, intensity: f32, + viewport: Vec4, } #[derive(Component)] From 63a291c6a800a12e8beb3dbad8f64927380493ca Mon Sep 17 00:00:00 2001 From: Mike Date: Tue, 17 Jan 2023 17:54:53 +0000 Subject: [PATCH 13/24] add tests for change detection and conditions for stageless (#7249) # Objective - add some tests for how change detection and run criteria interact in stageless --- crates/bevy_ecs/src/schedule_v3/mod.rs | 151 +++++++++++++++++++++++++ 1 file changed, 151 insertions(+) diff --git a/crates/bevy_ecs/src/schedule_v3/mod.rs b/crates/bevy_ecs/src/schedule_v3/mod.rs index 6a95e28e4a8e0..11828cbbe5fc0 100644 --- a/crates/bevy_ecs/src/schedule_v3/mod.rs +++ b/crates/bevy_ecs/src/schedule_v3/mod.rs @@ -197,6 +197,8 @@ mod tests { } mod conditions { + use crate::change_detection::DetectChanges; + use super::*; #[test] @@ -294,6 +296,155 @@ mod tests { schedule.run(&mut world); assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); } + + #[test] + fn system_conditions_and_change_detection() { + #[derive(Resource, Default)] + struct Bool2(pub bool); + + let mut world = World::default(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let mut schedule = Schedule::default(); + + schedule.add_system( + counting_system + .run_if(|res1: Res| res1.is_changed()) + .run_if(|res2: Res| res2.is_changed()), + ); + + // both resource were just added. + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // nothing has changed + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // RunConditionBool has changed, but counting_system did not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // internal state for the bool2 run criteria was updated in the + // previous run, so system still does not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // internal state for bool2 was updated, so system still does not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // now check that it works correctly changing Bool2 first and then RunConditionBool + world.get_resource_mut::().unwrap().0 = false; + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 2); + } + + #[test] + fn system_set_conditions_and_change_detection() { + #[derive(Resource, Default)] + struct Bool2(pub bool); + + let mut world = World::default(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let mut schedule = Schedule::default(); + + schedule.configure_set( + TestSet::A + .run_if(|res1: Res| res1.is_changed()) + .run_if(|res2: Res| res2.is_changed()), + ); + + schedule.add_system(counting_system.in_set(TestSet::A)); + + // both resource were just added. + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // nothing has changed + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // RunConditionBool has changed, but counting_system did not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // internal state for the bool2 run criteria was updated in the + // previous run, so system still does not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // internal state for bool2 was updated, so system still does not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // the system only runs when both are changed on the same run + world.get_resource_mut::().unwrap().0 = false; + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 2); + } + + #[test] + fn mixed_conditions_and_change_detection() { + #[derive(Resource, Default)] + struct Bool2(pub bool); + + let mut world = World::default(); + world.init_resource::(); + world.init_resource::(); + world.init_resource::(); + let mut schedule = Schedule::default(); + + schedule + .configure_set(TestSet::A.run_if(|res1: Res| res1.is_changed())); + + schedule.add_system( + counting_system + .run_if(|res2: Res| res2.is_changed()) + .in_set(TestSet::A), + ); + + // both resource were just added. + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // nothing has changed + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // RunConditionBool has changed, but counting_system did not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // we now only change bool2 and the system also should not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // internal state for the bool2 run criteria was updated in the + // previous run, so system still does not run + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 1); + + // the system only runs when both are changed on the same run + world.get_resource_mut::().unwrap().0 = false; + world.get_resource_mut::().unwrap().0 = false; + schedule.run(&mut world); + assert_eq!(world.resource::().0.load(Ordering::Relaxed), 2); + } } mod schedule_build_errors { From 0ca9c618e1dedecfe737d9a1a23748192e348441 Mon Sep 17 00:00:00 2001 From: Boxy Date: Tue, 17 Jan 2023 21:11:26 +0000 Subject: [PATCH 14/24] Update "Classifying PRs" section to talk about `D-Complex` (#7216) The current section does not talk about `D-Complex` and lists things like "adds unsafe code" as a reason to mark a PR `S-Controversial`. This is not how `D-Complex` and `S-Controversial` are being used at the moment. This PR lists what classifies a PR as `D-Complex` and what classifies a PR as `S-Controversial`. It also links to some PRs with each combination of labels to help give an idea for what this means in practice. cc #7211 which is doing a similar thing --- CONTRIBUTING.md | 71 ++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cb36131fd1cdc..d7d94ca9825d6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,37 +76,66 @@ Check out our dedicated [Bevy Organization document](/docs/the_bevy_organization ### Classifying PRs -Our merge strategy relies on the classification of PRs into three categories: **trivial**, **uncontroversial** and **controversial**. -When making PRs, try to split out more controversial changes from less controversial ones, in order to make your work easier to review and merge. -PRs that are deemed controversial will receive the [`S-Controversial`](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Controversial) label, and will have to go through the more thorough review process. +Our merge strategy relies on the classification of PRs on two axes: -PRs are trivial if there is no reasonable argument against them. This might include: +* How controversial are the design decisions +* How complex is the implementation -* Fixing dead links. -* Removing dead code or dependencies. -* Typo and grammar fixes. +PRs with non-trivial design decisions are given the [`S-Controversial`] label. This indicates that +the PR needs more thorough design review or an [RFC](https://github.com/bevyengine/rfcs), if complex enough. -PRs are controversial if there is serious design discussion required, or a large impact to contributors or users. Factors that increase controversy include: +PRs that are non-trivial to review are given the [`D-Complex`] label. This indicates that the PR +should be reviewed more thoroughly and by people with experience in the area that the PR touches. + +When making PRs, try to split out more controversial changes from less controversial ones, in order to make your work easier to review and merge. +It is also a good idea to try and split out simple changes from more complex changes if it is not helpful for then to be reviewed together. -1. Changes to project-wide workflow or style. +Some things that are reason to apply the [`S-Controversial`] label to a PR: + +1. Changes to a project-wide workflow or style. 2. New architecture for a large feature. -3. PRs where a serious tradeoff must be made. +3. Serious tradeoffs were made. 4. Heavy user impact. 5. New ways for users to make mistakes (footguns). -6. Introductions of `unsafe` code. -7. Large-scale code reorganization. -8. High levels of technical complexity. -9. Adding a dependency. -10. Touching licensing information (due to the level of precision required). -11. Adding root-level files (due to the high level of visibility). +6. Adding a dependency +7. Touching licensing information (due to level of precision required). +8. Adding root-level files (due to the high level of visibility) + +Some things that are reason to apply the [`D-Complex`] label to a PR: + +1. Introduction or modification of soundness relevent code (for example `unsafe` code) +2. High levels of technical complexity. +3. Large-scale code reorganization + +Examples of PRs that are not [`S-Controversial`] or [`D-Complex`]: + +* Fixing dead links. +* Removing dead code or unused dependencies. +* Typo and grammar fixes. +* [Add `Mut::reborrow`](https://github.com/bevyengine/bevy/pull/7114) +* [Add `Res::clone`](https://github.com/bevyengine/bevy/pull/4109) + +Examples of PRs that are [`S-Controversial`] but not [`D-Complex`]: + +* [Implement and require `#[derive(Component)]` on all component structs](https://github.com/bevyengine/bevy/pull/2254) +* [Use default serde impls for Entity](https://github.com/bevyengine/bevy/pull/6194) + +Examples of PRs that are not [`S-Controversial`] but are [`D-Complex`]: + +* [Ensure `Ptr`/`PtrMut`/`OwningPtr` are aligned in debug builds](https://github.com/bevyengine/bevy/pull/7117) +* [Replace `BlobVec`'s `swap_scratch` with a `swap_nonoverlapping`](https://github.com/bevyengine/bevy/pull/4853) + +Examples of PRs that are both [`S-Controversial`] and [`D-Complex`]: + +* [bevy_reflect: Binary formats](https://github.com/bevyengine/bevy/pull/6140) -Finally, changes are "relatively uncontroversial" if they are neither trivial or controversial. -Most PRs should fall into this category. +Some useful pull request queries: -Here are some useful pull request queries: +* [PRs which need reviews and are not `D-Complex`](https://github.com/bevyengine/bevy/pulls?q=is%3Apr+-label%3AD-Complex+-label%3AS-Ready-For-Final-Review+-label%3AS-Blocked++) +* [`D-Complex` PRs which need reviews](https://github.com/bevyengine/bevy/pulls?q=is%3Apr+label%3AD-Complex+-label%3AS-Ready-For-Final-Review+-label%3AS-Blocked) -* [Uncontroversial pull requests which have been reviewed and are ready for maintainers to merge](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Ready-For-Final-Review+-label%3AS-Controversial+-label%3AS-Blocked+-label%3AS-Adopt-Me+) -* [Controversial pull requests which have been reviewed and are ready for final input from a Project Lead or SME](https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Ready-For-Final-Review+label%3AS-Controversial+) +[`S-Controversial`]: https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AS-Controversial +[`D-Complex`]: https://github.com/bevyengine/bevy/pulls?q=is%3Aopen+is%3Apr+label%3AD-Complex ### Preparing Releases From 7d0edbc4d65c483d3e73c574b93a4a36560c0d07 Mon Sep 17 00:00:00 2001 From: James Liu Date: Tue, 17 Jan 2023 22:26:51 +0000 Subject: [PATCH 15/24] Improve change detection behavior for transform propagation (#6870) # Objective Fix #4647. If any child is changed, or even reordered, `Changed` is true, which causes transform propagation to propagate changes to all siblings of a changed child, even if they don't need to be. ## Solution As `Parent` and `Children` are updated in tandem in hierarchy commands after #4800. `Changed` is true on the child when `Changed` is true on the parent. However, unlike checking children, checking `Changed` is only localized to the current entity and will not force propagation to the siblings. Also took the opportunity to change propagation to use `Query::iter_many` instead of repeated `Query::get` calls. Should cut a bit of the overhead out of propagation. This means we won't panic when there isn't a `Parent` on the child, just skip over it. The tests from #4608 still pass, so the change detection here still works just fine under this approach. --- crates/bevy_transform/src/systems.rs | 90 +++++++++++++--------------- 1 file changed, 43 insertions(+), 47 deletions(-) diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs index 21eb7cbc51121..a5ca2f76128fd 100644 --- a/crates/bevy_transform/src/systems.rs +++ b/crates/bevy_transform/src/systems.rs @@ -27,12 +27,11 @@ pub fn sync_simple_transforms( /// to propagate transforms correctly. pub fn propagate_transforms( mut root_query: Query< - (Entity, Ref, Ref, &mut GlobalTransform), + (Entity, &Children, Ref, &mut GlobalTransform), Without, >, - transform_query: Query<(Ref, &mut GlobalTransform), With>, - parent_query: Query<&Parent>, - children_query: Query, (With, With)>, + transform_query: Query<(Ref, &mut GlobalTransform, Option<&Children>), With>, + parent_query: Query<(Entity, Ref)>, ) { root_query.par_for_each_mut( // The differing depths and sizes of hierarchy trees causes the work for each root to be @@ -40,18 +39,21 @@ pub fn propagate_transforms( // large trees are not clumped together. 1, |(entity, children, transform, mut global_transform)| { - let mut changed = transform.is_changed(); + let changed = transform.is_changed(); if changed { *global_transform = GlobalTransform::from(*transform); } - // If our `Children` has changed, we need to recalculate everything below us - changed |= children.is_changed(); - - for child in children.iter() { + for (child, actual_parent) in parent_query.iter_many(children) { + assert_eq!( + actual_parent.get(), entity, + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" + ); // SAFETY: - // - We may operate as if the hierarchy is consistent, since `propagate_recursive` will panic before continuing - // to propagate if it encounters an entity with inconsistent parentage. + // - `child` must have consistent parentage, or the above assertion would panic. + // Since `child` is parented to a root entity, the entire hierarchy leading to it is consistent. + // - We may operate as if all descendants are consistent, since `propagate_recursive` will panic before + // continuing to propagate if it encounters an entity with inconsistent parentage. // - Since each root entity is unique and the hierarchy is consistent and forest-like, // other root entities' `propagate_recursive` calls will not conflict with this one. // - Since this is the only place where `transform_query` gets used, there will be no conflicting fetches elsewhere. @@ -60,10 +62,8 @@ pub fn propagate_transforms( &global_transform, &transform_query, &parent_query, - &children_query, - entity, - *child, - changed, + child, + changed || actual_parent.is_changed(), ); } } @@ -75,35 +75,30 @@ pub fn propagate_transforms( /// /// # Panics /// -/// If `entity` or any of its descendants have a malformed hierarchy. -/// The panic will occur before propagating the transforms of any malformed entities and their descendants. +/// If `entity`'s descendants have a malformed hierarchy, this function will panic occur before propagating +/// the transforms of any malformed entities and their descendants. /// /// # Safety /// -/// While this function is running, `unsafe_transform_query` must not have any fetches for `entity`, +/// - While this function is running, `transform_query` must not have any fetches for `entity`, /// nor any of its descendants. +/// - The caller must ensure that the hierarchy leading to `entity` +/// is well-formed and must remain as a tree or a forest. Each entity must have at most one parent. unsafe fn propagate_recursive( parent: &GlobalTransform, - unsafe_transform_query: &Query<(Ref, &mut GlobalTransform), With>, - parent_query: &Query<&Parent>, - children_query: &Query, (With, With)>, - expected_parent: Entity, + transform_query: &Query< + (Ref, &mut GlobalTransform, Option<&Children>), + With, + >, + parent_query: &Query<(Entity, Ref)>, entity: Entity, mut changed: bool, ) { - let Ok(actual_parent) = parent_query.get(entity) else { - panic!("Propagated child for {entity:?} has no Parent component!"); - }; - assert_eq!( - actual_parent.get(), expected_parent, - "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" - ); - - let global_matrix = { - let Ok((transform, mut global_transform)) = + let (global_matrix, children) = { + let Ok((transform, mut global_transform, children)) = // SAFETY: This call cannot create aliased mutable references. // - The top level iteration parallelizes on the roots of the hierarchy. - // - The above assertion ensures that each child has one and only one unique parent throughout the entire + // - The caller ensures that each child has one and only one unique parent throughout the entire // hierarchy. // // For example, consider the following malformed hierarchy: @@ -127,7 +122,7 @@ unsafe fn propagate_recursive( // // Even if these A and B start two separate tasks running in parallel, one of them will panic before attempting // to mutably access E. - (unsafe { unsafe_transform_query.get_unchecked(entity) }) else { + (unsafe { transform_query.get_unchecked(entity) }) else { return; }; @@ -135,26 +130,27 @@ unsafe fn propagate_recursive( if changed { *global_transform = parent.mul_transform(*transform); } - *global_transform + (*global_transform, children) }; - let Ok(children) = children_query.get(entity) else { - return - }; - // If our `Children` has changed, we need to recalculate everything below us - changed |= children.is_changed(); - for child in &children { - // SAFETY: The caller guarantees that `unsafe_transform_query` will not be fetched + let Some(children) = children else { return }; + for (child, actual_parent) in parent_query.iter_many(children) { + assert_eq!( + actual_parent.get(), entity, + "Malformed hierarchy. This probably means that your hierarchy has been improperly maintained, or contains a cycle" + ); + // SAFETY: The caller guarantees that `transform_query` will not be fetched // for any descendants of `entity`, so it is safe to call `propagate_recursive` for each child. + // + // The above assertion ensures that each child has one and only one unique parent throughout the + // entire hierarchy. unsafe { propagate_recursive( &global_matrix, - unsafe_transform_query, + transform_query, parent_query, - children_query, - entity, - *child, - changed, + child, + changed || actual_parent.is_changed(), ); } } From 0df67cdaae30becd35447c6767d5e30afeee17f1 Mon Sep 17 00:00:00 2001 From: dis-da-moe Date: Tue, 17 Jan 2023 22:42:00 +0000 Subject: [PATCH 16/24] Add `AddAudioSource` trait and improve `Decodable` docs (#6649) # Objective - Fixes #6361 - Fixes #6362 - Fixes #6364 ## Solution - Added an example for creating a custom `Decodable` type - Clarified the documentation on `Decodable` - Added an `AddAudioSource` trait and implemented it for `App` Co-authored-by: dis-da-moe <84386186+dis-da-moe@users.noreply.github.com> --- Cargo.toml | 10 +++ crates/bevy_audio/src/audio_source.rs | 36 ++++++++-- crates/bevy_audio/src/lib.rs | 15 +++- examples/README.md | 1 + examples/audio/decodable.rs | 100 ++++++++++++++++++++++++++ 5 files changed, 155 insertions(+), 7 deletions(-) create mode 100644 examples/audio/decodable.rs diff --git a/Cargo.toml b/Cargo.toml index 898cd1f2e33ef..3e29eff47f2d4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -763,6 +763,16 @@ description = "Shows how to load and play an audio file, and control how it's pl category = "Audio" wasm = true +[[example]] +name = "decodable" +path = "examples/audio/decodable.rs" + +[package.metadata.example.decodable] +name = "Decodable" +description = "Shows how to create and register a custom audio source by implementing the `Decodable` type." +category = "Audio" +wasm = true + # Diagnostics [[example]] name = "log_diagnostics" diff --git a/crates/bevy_audio/src/audio_source.rs b/crates/bevy_audio/src/audio_source.rs index 470eccd04f59e..dda555f13a4de 100644 --- a/crates/bevy_audio/src/audio_source.rs +++ b/crates/bevy_audio/src/audio_source.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use bevy_asset::{AssetLoader, LoadContext, LoadedAsset}; +use bevy_asset::{Asset, AssetLoader, LoadContext, LoadedAsset}; use bevy_reflect::TypeUuid; use bevy_utils::BoxedFuture; use std::{io::Cursor, sync::Arc}; @@ -63,14 +63,24 @@ impl AssetLoader for AudioLoader { } } -/// A type implementing this trait can be decoded as a rodio source +/// A type implementing this trait can be converted to a [`rodio::Source`] type. +/// It must be [`Send`] and [`Sync`], and usually implements [`Asset`] so needs to be [`TypeUuid`], +/// in order to be registered. +/// Types that implement this trait usually contain raw sound data that can be converted into an iterator of samples. +/// This trait is implemented for [`AudioSource`]. +/// Check the example `audio/decodable` for how to implement this trait on a custom type. pub trait Decodable: Send + Sync + 'static { - /// The decoder that can decode the implementing type - type Decoder: rodio::Source + Send + Iterator; - /// A single value given by the decoder + /// The type of the audio samples. + /// Usually a [`u16`], [`i16`] or [`f32`], as those implement [`rodio::Sample`]. + /// Other types can implement the [`rodio::Sample`] trait as well. type DecoderItem: rodio::Sample + Send + Sync; - /// Build and return a [`Self::Decoder`] for the implementing type + /// The type of the iterator of the audio samples, + /// which iterates over samples of type [`Self::DecoderItem`]. + /// Must be a [`rodio::Source`] so that it can provide information on the audio it is iterating over. + type Decoder: rodio::Source + Send + Iterator; + + /// Build and return a [`Self::Decoder`] of the implementing type fn decoder(&self) -> Self::Decoder; } @@ -82,3 +92,17 @@ impl Decodable for AudioSource { rodio::Decoder::new(Cursor::new(self.clone())).unwrap() } } + +/// A trait that allows adding a custom audio source to the object. +/// This is implemented for [`App`][bevy_app::App] to allow registering custom [`Decodable`] types. +pub trait AddAudioSource { + /// Registers an audio source. + /// The type must implement [`Decodable`], + /// so that it can be converted to a [`rodio::Source`] type, + /// and [`Asset`], so that it can be registered as an asset. + /// To use this method on [`App`][bevy_app::App], + /// the [audio][super::AudioPlugin] and [asset][bevy_asset::AssetPlugin] plugins must be added first. + fn add_audio_source(&mut self) -> &mut Self + where + T: Decodable + Asset; +} diff --git a/crates/bevy_audio/src/lib.rs b/crates/bevy_audio/src/lib.rs index 9fc4a5484b88c..f91c60a9b55ee 100644 --- a/crates/bevy_audio/src/lib.rs +++ b/crates/bevy_audio/src/lib.rs @@ -35,12 +35,13 @@ pub mod prelude { pub use audio::*; pub use audio_output::*; pub use audio_source::*; + pub use rodio::cpal::Sample as CpalSample; pub use rodio::source::Source; pub use rodio::Sample; use bevy_app::prelude::*; -use bevy_asset::AddAsset; +use bevy_asset::{AddAsset, Asset}; /// Adds support for audio playback to a Bevy Application /// @@ -63,3 +64,15 @@ impl Plugin for AudioPlugin { app.init_asset_loader::(); } } + +impl AddAudioSource for App { + fn add_audio_source(&mut self) -> &mut Self + where + T: Decodable + Asset, + { + self.add_asset::() + .init_resource::>() + .init_non_send_resource::>() + .add_system_to_stage(CoreStage::PostUpdate, play_queued_audio_system::) + } +} diff --git a/examples/README.md b/examples/README.md index 83f6c129044f1..f5ff72e6752ba 100644 --- a/examples/README.md +++ b/examples/README.md @@ -178,6 +178,7 @@ Example | Description --- | --- [Audio](../examples/audio/audio.rs) | Shows how to load and play an audio file [Audio Control](../examples/audio/audio_control.rs) | Shows how to load and play an audio file, and control how it's played +[Decodable](../examples/audio/decodable.rs) | Shows how to create and register a custom audio source by implementing the `Decodable` type. ## Diagnostics diff --git a/examples/audio/decodable.rs b/examples/audio/decodable.rs new file mode 100644 index 0000000000000..1ac2fdd06db86 --- /dev/null +++ b/examples/audio/decodable.rs @@ -0,0 +1,100 @@ +//! Shows how to create a custom `Decodable` type by implementing a Sine wave. +//! ***WARNING THIS EXAMPLE IS VERY LOUD.*** Turn your volume down. +use bevy::audio::AddAudioSource; +use bevy::audio::Source; +use bevy::prelude::*; +use bevy::reflect::TypeUuid; +use bevy::utils::Duration; + +// This struct usually contains the data for the audio being played. +// This is where data read from an audio file would be stored, for example. +// Implementing `TypeUuid` will automatically implement `Asset`. +// This allows the type to be registered as an asset. +#[derive(TypeUuid)] +#[uuid = "c2090c23-78fd-44f1-8508-c89b1f3cec29"] +struct SineAudio { + frequency: f32, +} +// This decoder is responsible for playing the audio, +// and so stores data about the audio being played. +struct SineDecoder { + // how far along one period the wave is (between 0 and 1) + current_progress: f32, + // how much we move along the period every frame + progress_per_frame: f32, + // how long a period is + period: f32, + sample_rate: u32, +} + +impl SineDecoder { + fn new(frequency: f32) -> Self { + // standard sample rate for most recordings + let sample_rate = 44_100; + SineDecoder { + current_progress: 0., + progress_per_frame: frequency / sample_rate as f32, + period: std::f32::consts::PI * 2., + sample_rate, + } + } +} + +// The decoder must implement iterator so that it can implement `Decodable`. +impl Iterator for SineDecoder { + type Item = f32; + + fn next(&mut self) -> Option { + self.current_progress += self.progress_per_frame; + // we loop back round to 0 to avoid floating point inaccuracies + self.current_progress %= 1.; + Some(f32::sin(self.period * self.current_progress)) + } +} +// `Source` is what allows the audio source to be played by bevy. +// This trait provides information on the audio. +impl Source for SineDecoder { + fn current_frame_len(&self) -> Option { + None + } + + fn channels(&self) -> u16 { + 1 + } + + fn sample_rate(&self) -> u32 { + self.sample_rate + } + + fn total_duration(&self) -> Option { + None + } +} + +// Finally `Decodable` can be implemented for our `SineAudio`. +impl Decodable for SineAudio { + type Decoder = SineDecoder; + + type DecoderItem = ::Item; + + fn decoder(&self) -> Self::Decoder { + SineDecoder::new(self.frequency) + } +} + +fn main() { + let mut app = App::new(); + // register the audio source so that it can be used + app.add_plugins(DefaultPlugins) + .add_audio_source::() + .add_startup_system(setup) + .run(); +} + +fn setup(mut assets: ResMut>, audio: Res>) { + // add a `SineAudio` to the asset server so that it can be played + let audio_handle = assets.add(SineAudio { + frequency: 440., //this is the frequency of A4 + }); + audio.play(audio_handle); +} From 4ff50f6b5062145592575c98d9cc85f04a23ec82 Mon Sep 17 00:00:00 2001 From: IceSentry Date: Wed, 18 Jan 2023 02:07:26 +0000 Subject: [PATCH 17/24] fix load_internal_binary_asset with debug_asset_server (#7246) # Objective - Enabling the `debug_asset_server` feature doesn't compile when using it with `load_internal_binary_asset!()`. The issue is because it assumes the loader takes an `&'static str` as a parameter, but binary assets loader expect `&'static [u8]`. ## Solution - Add a generic type for the loader and use a different type in `load_internal_asset` and `load_internal_binary_asset` --- crates/bevy_asset/src/assets.rs | 4 ++-- crates/bevy_asset/src/debug_asset_server.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_asset/src/assets.rs b/crates/bevy_asset/src/assets.rs index dd3d4727e4261..1a3a300553990 100644 --- a/crates/bevy_asset/src/assets.rs +++ b/crates/bevy_asset/src/assets.rs @@ -417,7 +417,7 @@ macro_rules! load_internal_asset { let mut debug_app = $app .world .non_send_resource_mut::<$crate::debug_asset_server::DebugAssetApp>(); - $crate::debug_asset_server::register_handle_with_loader( + $crate::debug_asset_server::register_handle_with_loader::<_, &'static str>( $loader, &mut debug_app, $handle, @@ -455,7 +455,7 @@ macro_rules! load_internal_binary_asset { let mut debug_app = $app .world .non_send_resource_mut::<$crate::debug_asset_server::DebugAssetApp>(); - $crate::debug_asset_server::register_handle_with_loader( + $crate::debug_asset_server::register_handle_with_loader::<_, &'static [u8]>( $loader, &mut debug_app, $handle, diff --git a/crates/bevy_asset/src/debug_asset_server.rs b/crates/bevy_asset/src/debug_asset_server.rs index 690dd6769dc7c..e8809d5269c58 100644 --- a/crates/bevy_asset/src/debug_asset_server.rs +++ b/crates/bevy_asset/src/debug_asset_server.rs @@ -116,8 +116,8 @@ pub(crate) fn sync_debug_assets( /// /// If this feels a bit odd ... that's because it is. This was built to improve the UX of the /// `load_internal_asset` macro. -pub fn register_handle_with_loader( - _loader: fn(&'static str) -> A, +pub fn register_handle_with_loader( + _loader: fn(T) -> A, app: &mut DebugAssetApp, handle: HandleUntyped, file_path: &str, From 9eefd7c022efe572ffd2840cfc7a8b9eee982428 Mon Sep 17 00:00:00 2001 From: ickshonpe Date: Wed, 18 Jan 2023 02:19:17 +0000 Subject: [PATCH 18/24] Remove VerticalAlign from TextAlignment (#6807) # Objective Remove the `VerticalAlign` enum. Text's alignment field should only affect the text's internal text alignment, not its position. The only way to control a `TextBundle`'s position and bounds should be through the manipulation of the constraints in the `Style` components of the nodes in the Bevy UI's layout tree. `Text2dBundle` should have a separate `Anchor` component that sets its position relative to its transform. Related issues: #676, #1490, #5502, #5513, #5834, #6717, #6724, #6741, #6748 ## Changelog * Changed `TextAlignment` into an enum with `Left`, `Center`, and `Right` variants. * Removed the `HorizontalAlign` and `VerticalAlign` types. * Added an `Anchor` component to `Text2dBundle` * Added `Component` derive to `Anchor` * Use `f32::INFINITY` instead of `f32::MAX` to represent unbounded text in Text2dBounds ## Migration Guide The `alignment` field of `Text` now only affects the text's internal alignment. ### Change `TextAlignment` to TextAlignment` which is now an enum. Replace: * `TextAlignment::TOP_LEFT`, `TextAlignment::CENTER_LEFT`, `TextAlignment::BOTTOM_LEFT` with `TextAlignment::Left` * `TextAlignment::TOP_CENTER`, `TextAlignment::CENTER_LEFT`, `TextAlignment::BOTTOM_CENTER` with `TextAlignment::Center` * `TextAlignment::TOP_RIGHT`, `TextAlignment::CENTER_RIGHT`, `TextAlignment::BOTTOM_RIGHT` with `TextAlignment::Right` ### Changes for `Text2dBundle` `Text2dBundle` has a new field 'text_anchor' that takes an `Anchor` component that controls its position relative to its transform. --- crates/bevy_sprite/src/sprite.rs | 2 +- crates/bevy_text/src/glyph_brush.rs | 3 +- crates/bevy_text/src/lib.rs | 8 +- crates/bevy_text/src/text.rs | 125 ++++-------------- crates/bevy_text/src/text2d.rs | 81 +++++------- examples/2d/text2d.rs | 48 ++++--- .../external_source_external_thread.rs | 2 +- examples/ios/src/lib.rs | 2 +- examples/tools/gamepad_viewer.rs | 9 +- examples/ui/text.rs | 2 +- examples/ui/text_debug.rs | 2 +- 11 files changed, 88 insertions(+), 196 deletions(-) diff --git a/crates/bevy_sprite/src/sprite.rs b/crates/bevy_sprite/src/sprite.rs index da2a6de1c2cf1..3a3e85a4caf8a 100644 --- a/crates/bevy_sprite/src/sprite.rs +++ b/crates/bevy_sprite/src/sprite.rs @@ -24,7 +24,7 @@ pub struct Sprite { /// How a sprite is positioned relative to its [`Transform`](bevy_transform::components::Transform). /// It defaults to `Anchor::Center`. -#[derive(Debug, Clone, Default, Reflect)] +#[derive(Component, Debug, Clone, Default, Reflect)] #[doc(alias = "pivot")] pub enum Anchor { #[default] diff --git a/crates/bevy_text/src/glyph_brush.rs b/crates/bevy_text/src/glyph_brush.rs index b3b8e3d79f018..8f88a33f56467 100644 --- a/crates/bevy_text/src/glyph_brush.rs +++ b/crates/bevy_text/src/glyph_brush.rs @@ -41,8 +41,7 @@ impl GlyphBrush { ..Default::default() }; let section_glyphs = Layout::default() - .h_align(text_alignment.horizontal.into()) - .v_align(text_alignment.vertical.into()) + .h_align(text_alignment.into()) .calculate_glyphs(&self.fonts, &geom, sections); Ok(section_glyphs) } diff --git a/crates/bevy_text/src/lib.rs b/crates/bevy_text/src/lib.rs index 79f6909289a5e..84844ec5024ef 100644 --- a/crates/bevy_text/src/lib.rs +++ b/crates/bevy_text/src/lib.rs @@ -20,10 +20,7 @@ pub use text2d::*; pub mod prelude { #[doc(hidden)] - pub use crate::{ - Font, HorizontalAlign, Text, Text2dBundle, TextAlignment, TextError, TextSection, - TextStyle, VerticalAlign, - }; + pub use crate::{Font, Text, Text2dBundle, TextAlignment, TextError, TextSection, TextStyle}; } use bevy_app::prelude::*; @@ -77,9 +74,8 @@ impl Plugin for TextPlugin { .register_type::() .register_type::>() .register_type::() + .register_type::() .register_type::() - .register_type::() - .register_type::() .init_asset_loader::() .init_resource::() .init_resource::() diff --git a/crates/bevy_text/src/text.rs b/crates/bevy_text/src/text.rs index 7a8dc176c6349..719b59106b700 100644 --- a/crates/bevy_text/src/text.rs +++ b/crates/bevy_text/src/text.rs @@ -2,24 +2,36 @@ use bevy_asset::Handle; use bevy_ecs::{prelude::Component, reflect::ReflectComponent}; use bevy_reflect::{prelude::*, FromReflect}; use bevy_render::color::Color; +use bevy_utils::default; use serde::{Deserialize, Serialize}; use crate::Font; -#[derive(Component, Debug, Default, Clone, Reflect)] +#[derive(Component, Debug, Clone, Reflect)] #[reflect(Component, Default)] pub struct Text { pub sections: Vec, + /// The text's internal alignment. + /// Should not affect its position within a container. pub alignment: TextAlignment, } +impl Default for Text { + fn default() -> Self { + Self { + sections: Default::default(), + alignment: TextAlignment::Left, + } + } +} + impl Text { /// Constructs a [`Text`] with a single section. /// /// ``` /// # use bevy_asset::Handle; /// # use bevy_render::color::Color; - /// # use bevy_text::{Font, Text, TextAlignment, TextStyle, HorizontalAlign, VerticalAlign}; + /// # use bevy_text::{Font, Text, TextStyle, TextAlignment}; /// # /// # let font_handle: Handle = Default::default(); /// # @@ -42,12 +54,12 @@ impl Text { /// color: Color::WHITE, /// }, /// ) // You can still add an alignment. - /// .with_alignment(TextAlignment::CENTER); + /// .with_alignment(TextAlignment::Center); /// ``` pub fn from_section(value: impl Into, style: TextStyle) -> Self { Self { sections: vec![TextSection::new(value, style)], - alignment: Default::default(), + ..default() } } @@ -82,7 +94,7 @@ impl Text { pub fn from_sections(sections: impl IntoIterator) -> Self { Self { sections: sections.into_iter().collect(), - alignment: Default::default(), + ..default() } } @@ -117,78 +129,10 @@ impl TextSection { } } -#[derive(Debug, Clone, Copy, Reflect)] -pub struct TextAlignment { - pub vertical: VerticalAlign, - pub horizontal: HorizontalAlign, -} - -impl TextAlignment { - /// A [`TextAlignment`] set to the top-left. - pub const TOP_LEFT: Self = TextAlignment { - vertical: VerticalAlign::Top, - horizontal: HorizontalAlign::Left, - }; - - /// A [`TextAlignment`] set to the top-center. - pub const TOP_CENTER: Self = TextAlignment { - vertical: VerticalAlign::Top, - horizontal: HorizontalAlign::Center, - }; - - /// A [`TextAlignment`] set to the top-right. - pub const TOP_RIGHT: Self = TextAlignment { - vertical: VerticalAlign::Top, - horizontal: HorizontalAlign::Right, - }; - - /// A [`TextAlignment`] set to center the center-left. - pub const CENTER_LEFT: Self = TextAlignment { - vertical: VerticalAlign::Center, - horizontal: HorizontalAlign::Left, - }; - - /// A [`TextAlignment`] set to center on both axes. - pub const CENTER: Self = TextAlignment { - vertical: VerticalAlign::Center, - horizontal: HorizontalAlign::Center, - }; - - /// A [`TextAlignment`] set to the center-right. - pub const CENTER_RIGHT: Self = TextAlignment { - vertical: VerticalAlign::Center, - horizontal: HorizontalAlign::Right, - }; - - /// A [`TextAlignment`] set to the bottom-left. - pub const BOTTOM_LEFT: Self = TextAlignment { - vertical: VerticalAlign::Bottom, - horizontal: HorizontalAlign::Left, - }; - - /// A [`TextAlignment`] set to the bottom-center. - pub const BOTTOM_CENTER: Self = TextAlignment { - vertical: VerticalAlign::Bottom, - horizontal: HorizontalAlign::Center, - }; - - /// A [`TextAlignment`] set to the bottom-right. - pub const BOTTOM_RIGHT: Self = TextAlignment { - vertical: VerticalAlign::Bottom, - horizontal: HorizontalAlign::Right, - }; -} - -impl Default for TextAlignment { - fn default() -> Self { - TextAlignment::TOP_LEFT - } -} - /// Describes horizontal alignment preference for positioning & bounds. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Reflect, Serialize, Deserialize)] #[reflect(Serialize, Deserialize)] -pub enum HorizontalAlign { +pub enum TextAlignment { /// Leftmost character is immediately to the right of the render position.
/// Bounds start from the render position and advance rightwards. Left, @@ -200,35 +144,12 @@ pub enum HorizontalAlign { Right, } -impl From for glyph_brush_layout::HorizontalAlign { - fn from(val: HorizontalAlign) -> Self { - match val { - HorizontalAlign::Left => glyph_brush_layout::HorizontalAlign::Left, - HorizontalAlign::Center => glyph_brush_layout::HorizontalAlign::Center, - HorizontalAlign::Right => glyph_brush_layout::HorizontalAlign::Right, - } - } -} - -/// Describes vertical alignment preference for positioning & bounds. Currently a placeholder -/// for future functionality. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Reflect, Serialize, Deserialize)] -#[reflect(Serialize, Deserialize)] -pub enum VerticalAlign { - /// Characters/bounds start underneath the render position and progress downwards. - Top, - /// Characters/bounds center at the render position and progress outward equally. - Center, - /// Characters/bounds start above the render position and progress upward. - Bottom, -} - -impl From for glyph_brush_layout::VerticalAlign { - fn from(val: VerticalAlign) -> Self { +impl From for glyph_brush_layout::HorizontalAlign { + fn from(val: TextAlignment) -> Self { match val { - VerticalAlign::Top => glyph_brush_layout::VerticalAlign::Top, - VerticalAlign::Center => glyph_brush_layout::VerticalAlign::Center, - VerticalAlign::Bottom => glyph_brush_layout::VerticalAlign::Bottom, + TextAlignment::Left => glyph_brush_layout::HorizontalAlign::Left, + TextAlignment::Center => glyph_brush_layout::HorizontalAlign::Center, + TextAlignment::Right => glyph_brush_layout::HorizontalAlign::Right, } } } diff --git a/crates/bevy_text/src/text2d.rs b/crates/bevy_text/src/text2d.rs index 956c52644fef5..15a4038ad87e5 100644 --- a/crates/bevy_text/src/text2d.rs +++ b/crates/bevy_text/src/text2d.rs @@ -22,17 +22,10 @@ use bevy_utils::HashSet; use bevy_window::{WindowId, WindowScaleFactorChanged, Windows}; use crate::{ - Font, FontAtlasSet, FontAtlasWarning, HorizontalAlign, Text, TextError, TextLayoutInfo, - TextPipeline, TextSettings, VerticalAlign, YAxisOrientation, + Font, FontAtlasSet, FontAtlasWarning, Text, TextError, TextLayoutInfo, TextPipeline, + TextSettings, YAxisOrientation, }; -/// The calculated size of text drawn in 2D scene. -#[derive(Component, Default, Copy, Clone, Debug, Reflect)] -#[reflect(Component)] -pub struct Text2dSize { - pub size: Vec2, -} - /// The maximum width and height of text. The text will wrap according to the specified size. /// Characters out of the bounds after wrapping will be truncated. Text is aligned according to the /// specified `TextAlignment`. @@ -47,21 +40,27 @@ pub struct Text2dBounds { } impl Default for Text2dBounds { + #[inline] fn default() -> Self { - Self { - size: Vec2::new(f32::MAX, f32::MAX), - } + Self::UNBOUNDED } } +impl Text2dBounds { + /// Unbounded text will not be truncated or wrapped. + pub const UNBOUNDED: Self = Self { + size: Vec2::splat(f32::INFINITY), + }; +} + /// The bundle of components needed to draw text in a 2D scene via a 2D `Camera2dBundle`. /// [Example usage.](https://github.com/bevyengine/bevy/blob/latest/examples/2d/text2d.rs) #[derive(Bundle, Clone, Debug, Default)] pub struct Text2dBundle { pub text: Text, + pub text_anchor: Anchor, pub transform: Transform, pub global_transform: GlobalTransform, - pub text_2d_size: Text2dSize, pub text_2d_bounds: Text2dBounds, pub visibility: Visibility, pub computed_visibility: ComputedVisibility, @@ -77,32 +76,23 @@ pub fn extract_text2d_sprite( &ComputedVisibility, &Text, &TextLayoutInfo, + &Anchor, &GlobalTransform, - &Text2dSize, )>, >, ) { let scale_factor = windows.scale_factor(WindowId::primary()) as f32; - for (entity, computed_visibility, text, text_layout_info, text_transform, calculated_size) in + for (entity, computed_visibility, text, text_layout_info, anchor, text_transform) in text2d_query.iter() { if !computed_visibility.is_visible() { continue; } - let (width, height) = (calculated_size.size.x, calculated_size.size.y); let text_glyphs = &text_layout_info.glyphs; - let alignment_offset = match text.alignment.vertical { - VerticalAlign::Top => Vec3::new(0.0, -height, 0.0), - VerticalAlign::Center => Vec3::new(0.0, -height * 0.5, 0.0), - VerticalAlign::Bottom => Vec3::ZERO, - } + match text.alignment.horizontal { - HorizontalAlign::Left => Vec3::ZERO, - HorizontalAlign::Center => Vec3::new(-width * 0.5, 0.0, 0.0), - HorizontalAlign::Right => Vec3::new(-width, 0.0, 0.0), - }; - + let text_anchor = anchor.as_vec() * Vec2::new(1., -1.) - 0.5; + let alignment_offset = text_layout_info.size * text_anchor; let mut color = Color::WHITE; let mut current_section = usize::MAX; for text_glyph in text_glyphs { @@ -120,10 +110,9 @@ pub fn extract_text2d_sprite( let index = text_glyph.atlas_info.glyph_index; let rect = Some(atlas.textures[index]); - let glyph_transform = Transform::from_translation( - alignment_offset * scale_factor + text_glyph.position.extend(0.), - ); - // NOTE: Should match `bevy_ui::render::extract_text_uinodes` + let glyph_transform = + Transform::from_translation((alignment_offset + text_glyph.position).extend(0.)); + let transform = *text_transform * GlobalTransform::from_scale(Vec3::splat(scale_factor.recip())) * glyph_transform; @@ -167,8 +156,7 @@ pub fn update_text2d_layout( mut text_query: Query<( Entity, Ref, - Option<&Text2dBounds>, - &mut Text2dSize, + &Text2dBounds, Option<&mut TextLayoutInfo>, )>, ) { @@ -176,15 +164,12 @@ pub fn update_text2d_layout( let factor_changed = scale_factor_changed.iter().last().is_some(); let scale_factor = windows.scale_factor(WindowId::primary()); - for (entity, text, maybe_bounds, mut calculated_size, text_layout_info) in &mut text_query { + for (entity, text, bounds, text_layout_info) in &mut text_query { if factor_changed || text.is_changed() || queue.remove(&entity) { - let text_bounds = match maybe_bounds { - Some(bounds) => Vec2::new( - scale_value(bounds.size.x, scale_factor), - scale_value(bounds.size.y, scale_factor), - ), - None => Vec2::new(f32::MAX, f32::MAX), - }; + let text_bounds = Vec2::new( + scale_value(bounds.size.x, scale_factor), + scale_value(bounds.size.y, scale_factor), + ); match text_pipeline.queue_text( &fonts, @@ -207,18 +192,12 @@ pub fn update_text2d_layout( Err(e @ TextError::FailedToAddGlyph(_)) => { panic!("Fatal error when processing text: {e}."); } - Ok(info) => { - calculated_size.size = Vec2::new( - scale_value(info.size.x, 1. / scale_factor), - scale_value(info.size.y, 1. / scale_factor), - ); - match text_layout_info { - Some(mut t) => *t = info, - None => { - commands.entity(entity).insert(info); - } + Ok(info) => match text_layout_info { + Some(mut t) => *t = info, + None => { + commands.entity(entity).insert(info); } - } + }, } } } diff --git a/examples/2d/text2d.rs b/examples/2d/text2d.rs index 176c89b91e0f7..2f4f53df183ec 100644 --- a/examples/2d/text2d.rs +++ b/examples/2d/text2d.rs @@ -33,7 +33,7 @@ fn setup(mut commands: Commands, asset_server: Res) { font_size: 60.0, color: Color::WHITE, }; - let text_alignment = TextAlignment::CENTER; + let text_alignment = TextAlignment::Center; // 2d camera commands.spawn(Camera2dBundle::default()); // Demonstrate changing translation @@ -64,31 +64,29 @@ fn setup(mut commands: Commands, asset_server: Res) { // Demonstrate text wrapping let box_size = Vec2::new(300.0, 200.0); let box_position = Vec2::new(0.0, -250.0); - commands.spawn(SpriteBundle { - sprite: Sprite { - color: Color::rgb(0.25, 0.25, 0.75), - custom_size: Some(Vec2::new(box_size.x, box_size.y)), + commands + .spawn(SpriteBundle { + sprite: Sprite { + color: Color::rgb(0.25, 0.25, 0.75), + custom_size: Some(Vec2::new(box_size.x, box_size.y)), + ..default() + }, + transform: Transform::from_translation(box_position.extend(0.0)), ..default() - }, - transform: Transform::from_translation(box_position.extend(0.0)), - ..default() - }); - commands.spawn(Text2dBundle { - text: Text::from_section("this text wraps in the box", text_style), - text_2d_bounds: Text2dBounds { - // Wrap text in the rectangle - size: box_size, - }, - // We align text to the top-left, so this transform is the top-left corner of our text. The - // box is centered at box_position, so it is necessary to move by half of the box size to - // keep the text in the box. - transform: Transform::from_xyz( - box_position.x - box_size.x / 2.0, - box_position.y + box_size.y / 2.0, - 1.0, - ), - ..default() - }); + }) + .with_children(|builder| { + builder.spawn(Text2dBundle { + text: Text::from_section("this text wraps in the box", text_style) + .with_alignment(TextAlignment::Left), + text_2d_bounds: Text2dBounds { + // Wrap text in the rectangle + size: box_size, + }, + // ensure the text is drawn on top of the box + transform: Transform::from_translation(Vec3::Z), + ..default() + }); + }); } fn animate_translation( diff --git a/examples/async_tasks/external_source_external_thread.rs b/examples/async_tasks/external_source_external_thread.rs index 7e370879a2d90..3a174713904fa 100644 --- a/examples/async_tasks/external_source_external_thread.rs +++ b/examples/async_tasks/external_source_external_thread.rs @@ -66,7 +66,7 @@ fn spawn_text( for (per_frame, event) in reader.iter().enumerate() { commands.spawn(Text2dBundle { text: Text::from_section(event.0.to_string(), text_style.clone()) - .with_alignment(TextAlignment::CENTER), + .with_alignment(TextAlignment::Center), transform: Transform::from_xyz( per_frame as f32 * 100.0 + rand::thread_rng().gen_range(-40.0..40.0), 300.0, diff --git a/examples/ios/src/lib.rs b/examples/ios/src/lib.rs index e77085cd6737b..98c493af8619e 100644 --- a/examples/ios/src/lib.rs +++ b/examples/ios/src/lib.rs @@ -121,7 +121,7 @@ fn setup_scene( color: Color::BLACK, }, ) - .with_text_alignment(TextAlignment::CENTER), + .with_text_alignment(TextAlignment::Center), ); }); } diff --git a/examples/tools/gamepad_viewer.rs b/examples/tools/gamepad_viewer.rs index 2f7a77a57c04b..ae912192cbfdd 100644 --- a/examples/tools/gamepad_viewer.rs +++ b/examples/tools/gamepad_viewer.rs @@ -7,7 +7,7 @@ use bevy::{ GamepadAxisChangedEvent, GamepadButton, GamepadButtonChangedEvent, GamepadSettings, }, prelude::*, - sprite::{MaterialMesh2dBundle, Mesh2dHandle}, + sprite::{Anchor, MaterialMesh2dBundle, Mesh2dHandle}, }; const BUTTON_RADIUS: f32 = 25.; @@ -342,8 +342,8 @@ fn setup_sticks( value: format!("{:.3}", 0.), style, }, - ]) - .with_alignment(TextAlignment::BOTTOM_CENTER), + ]), + text_anchor: Anchor::BottomCenter, ..default() }, TextWithAxes { x_axis, y_axis }, @@ -409,8 +409,7 @@ fn setup_triggers( font_size: 16., color: TEXT_COLOR, }, - ) - .with_alignment(TextAlignment::CENTER), + ), ..default() }, TextWithButtonValue(button_type), diff --git a/examples/ui/text.rs b/examples/ui/text.rs index 014929cf7a63e..a25da20132d02 100644 --- a/examples/ui/text.rs +++ b/examples/ui/text.rs @@ -41,7 +41,7 @@ fn setup(mut commands: Commands, asset_server: Res) { color: Color::WHITE, }, ) // Set the alignment of the Text - .with_text_alignment(TextAlignment::TOP_CENTER) + .with_text_alignment(TextAlignment::Center) // Set the style of the TextBundle itself. .with_style(Style { position_type: PositionType::Absolute, diff --git a/examples/ui/text_debug.rs b/examples/ui/text_debug.rs index 040f2344c0526..6436f64720eee 100644 --- a/examples/ui/text_debug.rs +++ b/examples/ui/text_debug.rs @@ -54,7 +54,7 @@ fn infotext_system(mut commands: Commands, asset_server: Res) { color: Color::rgb(0.8, 0.2, 0.7), }, ) - .with_text_alignment(TextAlignment::CENTER) + .with_text_alignment(TextAlignment::Center) .with_style(Style { position_type: PositionType::Absolute, position: UiRect { From 88b353c4b1665625d5aabe6149e184ec5ba5984c Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 18 Jan 2023 02:19:19 +0000 Subject: [PATCH 19/24] Reduce the use of atomics in the render phase (#7084) # Objective Speed up the render phase of rendering. An extension of #6885. `SystemState::get` increments the `World`'s change tick atomically every time it's called. This is notably more expensive than a unsynchronized increment, even without contention. It also updates the archetypes, even when there has been nothing to update when it's called repeatedly. ## Solution Piggyback off of #6885. Split `SystemState::validate_world_and_update_archetypes` into `SystemState::validate_world` and `SystemState::update_archetypes`, and make the later `pub`. Then create safe variants of `SystemState::get_unchecked_manual` that still validate the `World` but do not update archetypes and do not increment the change tick using `World::read_change_tick` and `World::change_tick`. Update `RenderCommandState` to call `SystemState::update_archetypes` in `Draw::prepare` and `SystemState::get_manual` in `Draw::draw`. ## Performance There's a slight perf benefit (~2%) for `main_opaque_pass_3d` on `many_foxes` (340.39 us -> 333.32 us) ![image](https://user-images.githubusercontent.com/3137680/210643746-25320b98-3e2b-4a95-8084-892c23bb8b4e.png) ## Alternatives We can change `SystemState::get` to not increment the `World`'s change tick. Though this would still put updating the archetypes and an atomic read on the hot-path. --- ## Changelog Added: `SystemState::get_manual` Added: `SystemState::get_manual_mut` Added: `SystemState::update_archetypes` --- crates/bevy_ecs/src/system/function_system.rs | 70 ++++++++++++++++++- crates/bevy_render/src/render_phase/draw.rs | 3 +- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index c46cab36cb215..7493f6d3a1584 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -170,7 +170,8 @@ impl SystemState { where Param: ReadOnlySystemParam, { - self.validate_world_and_update_archetypes(world); + self.validate_world(world); + self.update_archetypes(world); // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world) } } @@ -178,7 +179,8 @@ impl SystemState { /// Retrieve the mutable [`SystemParam`] values. #[inline] pub fn get_mut<'w, 's>(&'s mut self, world: &'w mut World) -> SystemParamItem<'w, 's, Param> { - self.validate_world_and_update_archetypes(world); + self.validate_world(world); + self.update_archetypes(world); // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. unsafe { self.get_unchecked_manual(world) } } @@ -196,8 +198,20 @@ impl SystemState { self.world_id == world.id() } - fn validate_world_and_update_archetypes(&mut self, world: &World) { + /// Asserts that the [`SystemState`] matches the provided [`World`]. + #[inline] + fn validate_world(&self, world: &World) { assert!(self.matches_world(world), "Encountered a mismatched World. A SystemState cannot be used with Worlds other than the one it was created with."); + } + + /// Updates the state's internal view of the `world`'s archetypes. If this is not called before fetching the parameters, + /// the results may not accurately reflect what is in the `world`. + /// + /// This is only required if [`SystemState::get_manual`] or [`SystemState::get_manual_mut`] is being called, and it only needs to + /// be called if the `world` has been structurally mutated (i.e. added/removed a component or resource). Users using + /// [`SystemState::get`] or [`SystemState::get_mut`] do not need to call this as it will be automatically called for them. + #[inline] + pub fn update_archetypes(&mut self, world: &World) { let archetypes = world.archetypes(); let new_generation = archetypes.generation(); let old_generation = std::mem::replace(&mut self.archetype_generation, new_generation); @@ -212,6 +226,43 @@ impl SystemState { } } + /// Retrieve the [`SystemParam`] values. This can only be called when all parameters are read-only. + /// This will not update the state's view of the world's archetypes automatically nor increment the + /// world's change tick. + /// + /// For this to return accurate results, ensure [`SystemState::update_archetypes`] is called before this + /// function. + /// + /// Users should strongly prefer to use [`SystemState::get`] over this function. + #[inline] + pub fn get_manual<'w, 's>(&'s mut self, world: &'w World) -> SystemParamItem<'w, 's, Param> + where + Param: ReadOnlySystemParam, + { + self.validate_world(world); + let change_tick = world.read_change_tick(); + // SAFETY: Param is read-only and doesn't allow mutable access to World. It also matches the World this SystemState was created with. + unsafe { self.fetch(world, change_tick) } + } + + /// Retrieve the mutable [`SystemParam`] values. This will not update the state's view of the world's archetypes + /// automatically nor increment the world's change tick. + /// + /// For this to return accurate results, ensure [`SystemState::update_archetypes`] is called before this + /// function. + /// + /// Users should strongly prefer to use [`SystemState::get_mut`] over this function. + #[inline] + pub fn get_manual_mut<'w, 's>( + &'s mut self, + world: &'w mut World, + ) -> SystemParamItem<'w, 's, Param> { + self.validate_world(world); + let change_tick = world.change_tick(); + // SAFETY: World is uniquely borrowed and matches the World this SystemState was created with. + unsafe { self.fetch(world, change_tick) } + } + /// Retrieve the [`SystemParam`] values. This will not update archetypes automatically. /// /// # Safety @@ -224,6 +275,19 @@ impl SystemState { world: &'w World, ) -> SystemParamItem<'w, 's, Param> { let change_tick = world.increment_change_tick(); + self.fetch(world, change_tick) + } + + /// # Safety + /// This call might access any of the input parameters in a way that violates Rust's mutability rules. Make sure the data + /// access is safe in the context of global [`World`] access. The passed-in [`World`] _must_ be the [`World`] the [`SystemState`] was + /// created with. + #[inline] + unsafe fn fetch<'w, 's>( + &'s mut self, + world: &'w World, + change_tick: u32, + ) -> SystemParamItem<'w, 's, Param> { let param = Param::get_param(&mut self.param_state, &self.meta, world, change_tick); self.meta.last_change_tick = change_tick; param diff --git a/crates/bevy_render/src/render_phase/draw.rs b/crates/bevy_render/src/render_phase/draw.rs index d94cf3be7cd5a..f0421b1b64b47 100644 --- a/crates/bevy_render/src/render_phase/draw.rs +++ b/crates/bevy_render/src/render_phase/draw.rs @@ -244,6 +244,7 @@ where /// Prepares the render command to be used. This is called once and only once before the phase /// begins. There may be zero or more `draw` calls following a call to this function. fn prepare(&mut self, world: &'_ World) { + self.state.update_archetypes(world); self.view.update_archetypes(world); self.entity.update_archetypes(world); } @@ -256,7 +257,7 @@ where view: Entity, item: &P, ) { - let param = self.state.get(world); + let param = self.state.get_manual(world); let view = self.view.get_manual(world, view).unwrap(); let entity = self.entity.get_manual(world, item.entity()).unwrap(); // TODO: handle/log `RenderCommand` failure From d6bfd44f8f48af92c727a42ca7fe43f32a2ab747 Mon Sep 17 00:00:00 2001 From: Charles Bournhonesque Date: Wed, 18 Jan 2023 14:26:07 +0000 Subject: [PATCH 20/24] update doc comment for new_archetype in query-state (#7241) # Objective I was reading through the bevy_ecs code, trying to understand how everything works. I was getting a bit confused when reading the doc comment for the `new_archetype` function; it looks like it doesn't create a new archetype but instead updates some internal state in the SystemParam to facility QueryIteration. (I still couldn't find where a new archetype was actually created) ## Solution - Adding a doc comment with a more correct explanation. If it's deemed correct, I can also update the doc-comment for the other `new_archetype` calls --- crates/bevy_ecs/src/query/state.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/query/state.rs b/crates/bevy_ecs/src/query/state.rs index c96dbca3938f8..ca44e44d982ce 100644 --- a/crates/bevy_ecs/src/query/state.rs +++ b/crates/bevy_ecs/src/query/state.rs @@ -140,7 +140,7 @@ impl QueryState { } /// Takes a query for the given [`World`], checks if the given world is the same as the query, and - /// generates new archetypes for the given world. + /// updates the [`QueryState`]'s view of the [`World`] with any newly-added archetypes. /// /// # Panics /// @@ -166,7 +166,8 @@ impl QueryState { ); } - /// Creates a new [`Archetype`]. + /// Update the current [`QueryState`] with information from the provided [`Archetype`] + /// (if applicable, i.e. if the archetype has any intersecting [`ComponentId`] with the current [`QueryState`]). pub fn new_archetype(&mut self, archetype: &Archetype) { if Q::matches_component_set(&self.fetch_state, &|id| archetype.contains(id)) && F::matches_component_set(&self.filter_state, &|id| archetype.contains(id)) From 46293ce1e4c61421e353dc0b0431da67af6b7568 Mon Sep 17 00:00:00 2001 From: Rob Parrett Date: Wed, 18 Jan 2023 17:06:08 +0000 Subject: [PATCH 21/24] Fix init_non_send_resource overwriting previous values (#7261) # Objective Repeated calls to `init_non_send_resource` currently overwrite the old value because the wrong storage is being checked. ## Solution Use the correct storage. Add some tests. ## Notes Without the fix, the new test fails with ``` thread 'world::tests::init_non_send_resource_does_not_overwrite' panicked at 'assertion failed: `(left == right)` left: `1`, right: `0`', crates/bevy_ecs/src/world/mod.rs:2267:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace test world::tests::init_non_send_resource_does_not_overwrite ... FAILED ``` This was introduced by #7174 and it seems like a fairly straightforward oopsie. --- crates/bevy_ecs/src/world/mod.rs | 39 ++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 066268526e953..7fe6aecd2bd8d 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -827,7 +827,7 @@ impl World { let component_id = self.components.init_non_send::(); if self .storages - .resources + .non_send_resources .get(component_id) .map_or(true, |data| !data.is_present()) { @@ -2008,7 +2008,7 @@ impl FromWorld for T { #[cfg(test)] mod tests { - use super::World; + use super::{FromWorld, World}; use crate::{ change_detection::DetectChangesMut, component::{ComponentDescriptor, ComponentInfo, StorageType}, @@ -2230,6 +2230,41 @@ mod tests { assert_eq!(DROP_COUNT.load(std::sync::atomic::Ordering::SeqCst), 1); } + #[derive(Resource)] + struct TestFromWorld(u32); + impl FromWorld for TestFromWorld { + fn from_world(world: &mut World) -> Self { + let b = world.resource::(); + Self(b.0) + } + } + + #[test] + fn init_resource_does_not_overwrite() { + let mut world = World::new(); + world.insert_resource(TestResource(0)); + world.init_resource::(); + world.insert_resource(TestResource(1)); + world.init_resource::(); + + let resource = world.resource::(); + + assert_eq!(resource.0, 0); + } + + #[test] + fn init_non_send_resource_does_not_overwrite() { + let mut world = World::new(); + world.insert_resource(TestResource(0)); + world.init_non_send_resource::(); + world.insert_resource(TestResource(1)); + world.init_non_send_resource::(); + + let resource = world.non_send_resource::(); + + assert_eq!(resource.0, 0); + } + #[derive(Component)] struct Foo; From e0b921fbd99dffb612860ac9684f800b472a66ec Mon Sep 17 00:00:00 2001 From: harudagondi Date: Wed, 18 Jan 2023 17:20:26 +0000 Subject: [PATCH 22/24] AudioOutput is actually a normal resource now, not a non-send resource (#7262) # Objective - Fixes #7260 ## Solution - #6649 used `init_non_send_resource` for `AudioOutput`, but this is before #6436 was merged. - Use `init_resource` instead. --- crates/bevy_audio/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_audio/src/lib.rs b/crates/bevy_audio/src/lib.rs index f91c60a9b55ee..eb7e3b3222901 100644 --- a/crates/bevy_audio/src/lib.rs +++ b/crates/bevy_audio/src/lib.rs @@ -72,7 +72,7 @@ impl AddAudioSource for App { { self.add_asset::() .init_resource::>() - .init_non_send_resource::>() + .init_resource::>() .add_system_to_stage(CoreStage::PostUpdate, play_queued_audio_system::) } } From f8feec6ef1a47a6c8a562399b883d92198f02222 Mon Sep 17 00:00:00 2001 From: targrub Date: Wed, 18 Jan 2023 17:20:27 +0000 Subject: [PATCH 23/24] Fix tiny clippy issue for upcoming Rust version (#7266) Co-authored-by: targrub <62773321+targrub@users.noreply.github.com> --- crates/bevy_ecs/src/schedule_v3/schedule.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule_v3/schedule.rs b/crates/bevy_ecs/src/schedule_v3/schedule.rs index 2bd7b00eca72f..8aeb01d995336 100644 --- a/crates/bevy_ecs/src/schedule_v3/schedule.rs +++ b/crates/bevy_ecs/src/schedule_v3/schedule.rs @@ -77,7 +77,7 @@ impl Schedules { #[allow(unused_variables)] for (label, schedule) in self.inner.iter_mut() { #[cfg(feature = "trace")] - let name = format!("{:?}", label); + let name = format!("{label:?}"); #[cfg(feature = "trace")] let _one_span = info_span!("check schedule ticks", name = &name).entered(); schedule.check_change_ticks(change_tick); From f0c504947ce653068a424979faf226c1e990818d Mon Sep 17 00:00:00 2001 From: Stephen Martindale Date: Wed, 18 Jan 2023 23:02:38 +0000 Subject: [PATCH 24/24] Docs: App::run() might never return; effect of WinitSettings::return_from_run. (#7228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective See: - https://github.com/bevyengine/bevy/issues/7067#issuecomment-1381982285 - (This does not fully close that issue in my opinion.) - https://discord.com/channels/691052431525675048/1063454009769340989 ## Solution This merge request adds documentation: 1. Alert users to the fact that `App::run()` might never return and code placed after it might never be executed. 2. Makes `winit::WinitSettings::return_from_run` discoverable. 3. Better explains why `winit::WinitSettings::return_from_run` is discouraged and better links to up-stream docs. on that topic. 4. Adds notes to the `app/return_after_run.rs` example which otherwise promotes a feature that carries caveats. Furthermore, w.r.t `winit::WinitSettings::return_from_run`: - Broken links to `winit` docs are fixed. - Links now point to BOTH `EventLoop::run()` and `EventLoopExtRunReturn::run_return()` which are the salient up-stream pages and make more sense, taken together. - Collateral damage: "Supported platforms" heading; disambiguation of "run" → `App::run()`; links. ## Future Work I deliberately structured the "`run()` might not return" section under `App::run()` to allow for alternative patterns (e.g. `AppExit` event, `WindowClosed` event) to be listed or mentioned, beneath it, in the future. --- crates/bevy_app/src/app.rs | 18 +++++++++++++++++ crates/bevy_winit/src/winit_config.rs | 29 +++++++++++++++++++-------- examples/app/return_after_run.rs | 11 ++++++++++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 2a47aabb964c0..a8db6bfc042f1 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -178,6 +178,24 @@ impl App { /// Finalizes the [`App`] configuration. For general usage, see the example on the item /// level documentation. /// + /// # `run()` might not return + /// + /// Calls to [`App::run()`] might never return. + /// + /// In simple and *headless* applications, one can expect that execution will + /// proceed, normally, after calling [`run()`](App::run()) but this is not the case for + /// windowed applications. + /// + /// Windowed apps are typically driven by an *event loop* or *message loop* and + /// some window-manager APIs expect programs to terminate when their primary + /// window is closed and that event loop terminates – behaviour of processes that + /// do not is often platform dependent or undocumented. + /// + /// By default, *Bevy* uses the `winit` crate for window creation. See + /// [`WinitSettings::return_from_run`](https://docs.rs/bevy/latest/bevy/winit/struct.WinitSettings.html#structfield.return_from_run) + /// for further discussion of this topic and for a mechanism to require that [`App::run()`] + /// *does* return – albeit one that carries its own caveats and disclaimers. + /// /// # Panics /// /// Panics if called from `Plugin::build()`, because it would prevent other plugins to properly build. diff --git a/crates/bevy_winit/src/winit_config.rs b/crates/bevy_winit/src/winit_config.rs index c1db2b2fd27e6..40bfd1f87323b 100644 --- a/crates/bevy_winit/src/winit_config.rs +++ b/crates/bevy_winit/src/winit_config.rs @@ -4,15 +4,28 @@ use bevy_utils::Duration; /// A resource for configuring usage of the `rust_winit` library. #[derive(Debug, Resource)] pub struct WinitSettings { - /// Configures the winit library to return control to the main thread after the - /// [run](bevy_app::App::run) loop is exited. Winit strongly recommends avoiding this when - /// possible. Before using this please read and understand the - /// [caveats](winit::platform::run_return::EventLoopExtRunReturn::run_return) in the winit - /// documentation. + /// Configures `winit` to return control to the caller after exiting the + /// event loop, enabling [`App::run()`](bevy_app::App::run()) to return. /// - /// This feature is only available on desktop `target_os` configurations. Namely `windows`, - /// `macos`, `linux`, `dragonfly`, `freebsd`, `netbsd`, and `openbsd`. If set to true on an - /// unsupported platform [run](bevy_app::App::run) will panic. + /// By default, [`return_from_run`](Self::return_from_run) is `false` and *Bevy* + /// will use `winit`'s + /// [`EventLoop::run()`](https://docs.rs/winit/latest/winit/event_loop/struct.EventLoop.html#method.run) + /// to initiate the event loop. + /// [`EventLoop::run()`](https://docs.rs/winit/latest/winit/event_loop/struct.EventLoop.html#method.run) + /// will never return but will terminate the process after the event loop exits. + /// + /// Setting [`return_from_run`](Self::return_from_run) to `true` will cause *Bevy* + /// to use `winit`'s + /// [`EventLoopExtRunReturn::run_return()`](https://docs.rs/winit/latest/winit/platform/run_return/trait.EventLoopExtRunReturn.html#tymethod.run_return) + /// instead which is strongly discouraged by the `winit` authors. + /// + /// # Supported platforms + /// + /// This feature is only available on the following desktop `target_os` configurations: + /// `windows`, `macos`, `linux`, `dragonfly`, `freebsd`, `netbsd`, and `openbsd`. + /// + /// Setting [`return_from_run`](Self::return_from_run) to `true` on + /// unsupported platforms will cause [`App::run()`](bevy_app::App::run()) to panic! pub return_from_run: bool, /// Configures how the winit event loop updates while the window is focused. pub focused_mode: UpdateMode, diff --git a/examples/app/return_after_run.rs b/examples/app/return_after_run.rs index 966659688655e..f1fc07b50c5c3 100644 --- a/examples/app/return_after_run.rs +++ b/examples/app/return_after_run.rs @@ -1,4 +1,15 @@ //! Shows how to return to the calling function after a windowed Bevy app has exited. +//! +//! In windowed *Bevy* applications, executing code below a call to `App::run()` is +//! not recommended because `App::run()` might never return. +//! +//! This example demonstrates the use of `WinitSettings::return_from_run` to +//! require that `App::run()` *does* return but this is not recommended. Be sure +//! to read the documentation on both `App::run()` and `WinitSettings::return_from_run` +//! for caveats and further details: +//! +//! - +//! - use bevy::{prelude::*, winit::WinitSettings};