From 1cbd67f96a4e294f5aa46971265e5adb3dfd9bd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Tue, 8 Jul 2025 19:02:02 +0200 Subject: [PATCH 1/7] formatting fix in bevy_remote cargo.toml (#20033) # Objective - bevy_log is not "others". it's part of Bevy ## Solution - Move it - Also use the same format as other Bevy dependency (path first) as I use it in some regexes to run tests on the repo --- crates/bevy_remote/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_remote/Cargo.toml b/crates/bevy_remote/Cargo.toml index e7a40c65ba..899ac8b846 100644 --- a/crates/bevy_remote/Cargo.toml +++ b/crates/bevy_remote/Cargo.toml @@ -30,6 +30,7 @@ bevy_platform = { path = "../bevy_platform", version = "0.17.0-dev", default-fea "serialize", ] } bevy_asset = { path = "../bevy_asset", version = "0.17.0-dev", optional = true } +bevy_log = { path = "../bevy_log", version = "0.17.0-dev" } # other anyhow = "1" @@ -38,7 +39,6 @@ serde = { version = "1", features = ["derive"] } serde_json = "1.0.140" http-body-util = "0.1" async-channel = "2" -bevy_log = { version = "0.17.0-dev", path = "../bevy_log" } # dependencies that will not compile on wasm [target.'cfg(not(target_family = "wasm"))'.dependencies] From 9b114a49366512f3bcfc7accca78568da3e20708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Tue, 8 Jul 2025 19:08:20 +0200 Subject: [PATCH 2/7] enable syn/full in bevy_render_macros (#20034) # Objective - `bevy_render_macros` fails to build on its own: ``` error[E0432]: unresolved import `syn::Pat` --> crates/bevy_render/macros/src/specializer.rs:13:69 | 13 | DeriveInput, Expr, Field, Ident, Index, Member, Meta, MetaList, Pat, Path, Token, Type, | ^^^ | | | no `Pat` in the root | help: a similar name exists in the module: `Path` | note: found an item that was configured out --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/syn-2.0.104/src/lib.rs:485:15 | 485 | FieldPat, Pat, PatConst, PatIdent, PatLit, PatMacro, PatOr, PatParen, PatPath, PatRange, | ^^^ note: the item is gated behind the `full` feature --> /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/syn-2.0.104/src/lib.rs:482:7 | 482 | #[cfg(feature = "full")] | ^^^^^^^^^^^^^^^^ ``` ## Solution - Enable the `full` feature of `syn` --- crates/bevy_render/macros/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_render/macros/Cargo.toml b/crates/bevy_render/macros/Cargo.toml index 016fe88765..99d1dca5f9 100644 --- a/crates/bevy_render/macros/Cargo.toml +++ b/crates/bevy_render/macros/Cargo.toml @@ -14,7 +14,7 @@ proc-macro = true [dependencies] bevy_macro_utils = { path = "../../bevy_macro_utils", version = "0.17.0-dev" } -syn = "2.0" +syn = { version = "2.0", features = ["full"] } proc-macro2 = "1.0" quote = "1.0" From 9bb41a8309e345061f05645850964ff4b0633fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Mockers?= Date: Tue, 8 Jul 2025 19:46:01 +0200 Subject: [PATCH 3/7] bevy_math: don't allow dead code (#20039) # Objective - `bevy_math` allows the `dead_code` lint on some private structs when `alloc` is not enabled - allowing lints is not allowed, we should use expect ## Solution - Don't even compile the code if its expected to be dead instead of allowing or expecting the lint --- crates/bevy_math/src/primitives/polygon.rs | 39 +++++++++------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/crates/bevy_math/src/primitives/polygon.rs b/crates/bevy_math/src/primitives/polygon.rs index 9aa261b297..096a19ecfb 100644 --- a/crates/bevy_math/src/primitives/polygon.rs +++ b/crates/bevy_math/src/primitives/polygon.rs @@ -2,17 +2,13 @@ use { super::{Measured2d, Triangle2d}, alloc::{collections::BTreeMap, vec::Vec}, + core::cmp::Ordering, }; -use core::cmp::Ordering; - use crate::Vec2; -#[cfg_attr( - not(feature = "alloc"), - expect(dead_code, reason = "this type is only used with the alloc feature") -)] #[derive(Debug, Clone, Copy)] +#[cfg(feature = "alloc")] enum Endpoint { Left, Right, @@ -24,22 +20,16 @@ enum Endpoint { /// If `e1.position().x == e2.position().x` the events are ordered from bottom to top. /// /// This is the order expected by the [`SweepLine`]. +#[cfg(feature = "alloc")] #[derive(Debug, Clone, Copy)] -#[cfg_attr( - not(feature = "alloc"), - allow(dead_code, reason = "this type is only used with the alloc feature") -)] struct SweepLineEvent { segment: Segment, /// Type of the vertex (left or right) endpoint: Endpoint, } +#[cfg(feature = "alloc")] impl SweepLineEvent { - #[cfg_attr( - not(feature = "alloc"), - allow(dead_code, reason = "this type is only used with the alloc feature") - )] fn position(&self) -> Vec2 { match self.endpoint { Endpoint::Left => self.segment.left, @@ -48,20 +38,24 @@ impl SweepLineEvent { } } +#[cfg(feature = "alloc")] impl PartialEq for SweepLineEvent { fn eq(&self, other: &Self) -> bool { self.position() == other.position() } } +#[cfg(feature = "alloc")] impl Eq for SweepLineEvent {} +#[cfg(feature = "alloc")] impl PartialOrd for SweepLineEvent { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } +#[cfg(feature = "alloc")] impl Ord for SweepLineEvent { fn cmp(&self, other: &Self) -> Ordering { xy_order(self.position(), other.position()) @@ -69,10 +63,7 @@ impl Ord for SweepLineEvent { } /// Orders 2D points according to the order expected by the sweep line and event queue from -X to +X and then -Y to Y. -#[cfg_attr( - not(feature = "alloc"), - allow(dead_code, reason = "this type is only used with the alloc feature") -)] +#[cfg(feature = "alloc")] fn xy_order(a: Vec2, b: Vec2) -> Ordering { a.x.total_cmp(&b.x).then_with(|| a.y.total_cmp(&b.y)) } @@ -129,26 +120,31 @@ impl EventQueue { /// Segments are ordered from bottom to top based on their left vertices if possible. /// If their y values are identical, the segments are ordered based on the y values of their right vertices. #[derive(Debug, Clone, Copy)] +#[cfg(feature = "alloc")] struct Segment { edge_index: usize, left: Vec2, right: Vec2, } +#[cfg(feature = "alloc")] impl PartialEq for Segment { fn eq(&self, other: &Self) -> bool { self.edge_index == other.edge_index } } +#[cfg(feature = "alloc")] impl Eq for Segment {} +#[cfg(feature = "alloc")] impl PartialOrd for Segment { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } +#[cfg(feature = "alloc")] impl Ord for Segment { fn cmp(&self, other: &Self) -> Ordering { self.left @@ -159,10 +155,7 @@ impl Ord for Segment { } /// Holds information about which segment is above and which is below a given [`Segment`] -#[cfg_attr( - not(feature = "alloc"), - expect(dead_code, reason = "this type is only used with the alloc feature") -)] +#[cfg(feature = "alloc")] #[derive(Debug, Clone, Copy)] struct SegmentOrder { above: Option, @@ -173,8 +166,8 @@ struct SegmentOrder { /// /// It can be thought of as a vertical line sweeping from -X to +X across the polygon that keeps track of the order of the segments /// the sweep line is intersecting at any given moment. -#[cfg(feature = "alloc")] #[derive(Debug, Clone)] +#[cfg(feature = "alloc")] struct SweepLine<'a> { vertices: &'a [Vec2], tree: BTreeMap, From 0ee937784e8ebf990c4293fb1e824f1f67cbcbf8 Mon Sep 17 00:00:00 2001 From: Chris Russell <8494645+chescock@users.noreply.github.com> Date: Tue, 8 Jul 2025 14:09:45 -0400 Subject: [PATCH 4/7] Simplify self-edge checking in schedule building (#20015) # Objective Make the schedule graph code more understandable, and replace some panics with `Result`s. I found the `check_edges` and `check_hierarchy` functions [a little confusing](https://github.com/bevyengine/bevy/pull/19352#discussion_r2181486099), as they combined two concerns: Initializing graph nodes for system sets, and checking for self-edges on system sets. It was hard to understand the self-edge checks because it wasn't clear what `NodeId` was being checked against! So, let's separate those concerns, and move them to more appropriate locations. Fix a bug where `schedule.configure_sets((SameSet, SameSet).chain());` would panic with an unhelpful message: `assertion failed: index_a < index_b`. ## Solution Remove the `check_edges` and `check_hierarchy` functions, separating the initialization behavior and the checking behavior and moving them where they are easier to understand. For initializing graph nodes, do this on-demand using the `entry` API by replacing later `self.system_set_ids[&set]` calls with a `self.system_sets.get_or_add_set(set)` method. This should avoid the need for an extra pass over the graph and an extra lookup. Unfortunately, implementing that method directly on `ScheduleGraph` leads to borrowing errors as it borrows the entire `struct`. So, split out the collections managing system sets into a separate `struct`. For checking self-edges, move this check later so that it can be reported by returning a `Result` from `Schedule::initialize` instead of having to panic in `configure_set_inner`. The issue was that `iter_sccs` does not report self-edges as cycles, since the resulting components only have one node, but that later code assumes all edges point forward. So, check for self-edges directly, immediately before calling `iter_sccs`. This also ensures we catch *every* way that self-edges can be added. The previous code missed an edge case where `chain()`ing a set to itself would create a self-edge and would trigger a `debug_assert`. --- crates/bevy_ecs/src/schedule/mod.rs | 17 +- crates/bevy_ecs/src/schedule/schedule.rs | 202 ++++++++--------------- 2 files changed, 88 insertions(+), 131 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/mod.rs b/crates/bevy_ecs/src/schedule/mod.rs index 80189d58c1..d368e6b5e7 100644 --- a/crates/bevy_ecs/src/schedule/mod.rs +++ b/crates/bevy_ecs/src/schedule/mod.rs @@ -563,10 +563,21 @@ mod tests { use super::*; #[test] - #[should_panic] fn dependency_loop() { let mut schedule = Schedule::default(); schedule.configure_sets(TestSystems::X.after(TestSystems::X)); + let mut world = World::new(); + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::DependencyLoop(_)))); + } + + #[test] + fn dependency_loop_from_chain() { + let mut schedule = Schedule::default(); + schedule.configure_sets((TestSystems::X, TestSystems::X).chain()); + let mut world = World::new(); + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::DependencyLoop(_)))); } #[test] @@ -598,10 +609,12 @@ mod tests { } #[test] - #[should_panic] fn hierarchy_loop() { let mut schedule = Schedule::default(); schedule.configure_sets(TestSystems::X.in_set(TestSystems::X)); + let mut world = World::new(); + let result = schedule.initialize(&mut world); + assert!(matches!(result, Err(ScheduleBuildError::HierarchyLoop(_)))); } #[test] diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 0384377ab9..ed40f21fbd 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -390,14 +390,14 @@ impl Schedule { let a = a.into_system_set(); let b = b.into_system_set(); - let Some(&a_id) = self.graph.system_set_ids.get(&a.intern()) else { + let Some(&a_id) = self.graph.system_sets.ids.get(&a.intern()) else { panic!( "Could not mark system as ambiguous, `{:?}` was not found in the schedule. Did you try to call `ambiguous_with` before adding the system to the world?", a ); }; - let Some(&b_id) = self.graph.system_set_ids.get(&b.intern()) else { + let Some(&b_id) = self.graph.system_sets.ids.get(&b.intern()) else { panic!( "Could not mark system as ambiguous, `{:?}` was not found in the schedule. Did you try to call `ambiguous_with` before adding the system to the world?", @@ -760,6 +760,27 @@ enum UninitializedId { }, } +/// Metadata for system sets in a schedule. +#[derive(Default)] +struct SystemSets { + /// List of system sets in the schedule + sets: SlotMap, + /// List of conditions for each system set, in the same order as `system_sets` + conditions: SecondaryMap>, + /// Map from system set to node id + ids: HashMap, +} + +impl SystemSets { + fn get_or_add_set(&mut self, set: InternedSystemSet) -> SystemSetKey { + *self.ids.entry(set).or_insert_with(|| { + let key = self.sets.insert(SystemSetNode::new(set)); + self.conditions.insert(key, Vec::new()); + key + }) + } +} + /// Metadata for a [`Schedule`]. /// /// The order isn't optimized; calling `ScheduleGraph::build_schedule` will return a @@ -770,12 +791,8 @@ pub struct ScheduleGraph { pub systems: SlotMap, /// List of conditions for each system, in the same order as `systems` pub system_conditions: SecondaryMap>, - /// List of system sets in the schedule - system_sets: SlotMap, - /// List of conditions for each system set, in the same order as `system_sets` - system_set_conditions: SecondaryMap>, - /// Map from system set to node id - system_set_ids: HashMap, + /// Data about system sets in the schedule + system_sets: SystemSets, /// Systems that have not been initialized yet; for system sets, we store the index of the first uninitialized condition /// (all the conditions after that index still need to be initialized) uninit: Vec, @@ -800,9 +817,7 @@ impl ScheduleGraph { Self { systems: SlotMap::with_key(), system_conditions: SecondaryMap::new(), - system_sets: SlotMap::with_key(), - system_set_conditions: SecondaryMap::new(), - system_set_ids: HashMap::default(), + system_sets: SystemSets::default(), uninit: Vec::new(), hierarchy: Dag::new(), dependency: Dag::new(), @@ -826,7 +841,7 @@ impl ScheduleGraph { /// Returns `true` if the given system set is part of the graph. Otherwise, returns `false`. pub fn contains_set(&self, set: impl SystemSet) -> bool { - self.system_set_ids.contains_key(&set.intern()) + self.system_sets.ids.contains_key(&set.intern()) } /// Returns the system at the given [`NodeId`]. @@ -840,7 +855,7 @@ impl ScheduleGraph { /// Returns the set at the given [`NodeId`], if it exists. pub fn get_set_at(&self, key: SystemSetKey) -> Option<&dyn SystemSet> { - self.system_sets.get(key).map(|set| &*set.inner) + self.system_sets.sets.get(key).map(|set| &*set.inner) } /// Returns the set at the given [`NodeId`]. @@ -854,7 +869,7 @@ impl ScheduleGraph { /// Returns the conditions for the set at the given [`SystemSetKey`], if it exists. pub fn get_set_conditions_at(&self, key: SystemSetKey) -> Option<&[ConditionWithAccess]> { - self.system_set_conditions.get(key).map(Vec::as_slice) + self.system_sets.conditions.get(key).map(Vec::as_slice) } /// Returns the conditions for the set at the given [`SystemSetKey`]. @@ -882,9 +897,9 @@ impl ScheduleGraph { pub fn system_sets( &self, ) -> impl Iterator { - self.system_sets.iter().filter_map(|(key, set_node)| { + self.system_sets.sets.iter().filter_map(|(key, set_node)| { let set = &*set_node.inner; - let conditions = self.system_set_conditions.get(key)?.as_slice(); + let conditions = self.system_sets.conditions.get(key)?.as_slice(); Some((key, set, conditions)) }) } @@ -946,7 +961,7 @@ impl ScheduleGraph { } let mut set_config = InternedSystemSet::into_config(set.intern()); set_config.conditions.extend(collective_conditions); - self.configure_set_inner(set_config).unwrap(); + self.configure_set_inner(set_config); } } } @@ -1047,10 +1062,7 @@ impl ScheduleGraph { } /// Add a [`ScheduleConfig`] to the graph, including its dependencies and conditions. - fn add_system_inner( - &mut self, - config: ScheduleConfig, - ) -> Result { + fn add_system_inner(&mut self, config: ScheduleConfig) -> SystemKey { let key = self.systems.insert(SystemNode::new(config.node)); self.system_conditions.insert( key, @@ -1064,9 +1076,9 @@ impl ScheduleGraph { self.uninit.push(UninitializedId::System(key)); // graph updates are immediate - self.update_graphs(NodeId::System(key), config.metadata)?; + self.update_graphs(NodeId::System(key), config.metadata); - Ok(NodeId::System(key)) + key } #[track_caller] @@ -1075,39 +1087,26 @@ impl ScheduleGraph { } /// Add a single `ScheduleConfig` to the graph, including its dependencies and conditions. - fn configure_set_inner( - &mut self, - set: ScheduleConfig, - ) -> Result { + fn configure_set_inner(&mut self, set: ScheduleConfig) -> SystemSetKey { let ScheduleConfig { node: set, metadata, conditions, } = set; - let key = match self.system_set_ids.get(&set) { - Some(&id) => id, - None => self.add_set(set), - }; + let key = self.system_sets.get_or_add_set(set); // graph updates are immediate - self.update_graphs(NodeId::Set(key), metadata)?; + self.update_graphs(NodeId::Set(key), metadata); // system init has to be deferred (need `&mut World`) - let system_set_conditions = self.system_set_conditions.entry(key).unwrap().or_default(); + let system_set_conditions = self.system_sets.conditions.entry(key).unwrap().or_default(); self.uninit.push(UninitializedId::Set { key, first_uninit_condition: system_set_conditions.len(), }); system_set_conditions.extend(conditions.into_iter().map(ConditionWithAccess::new)); - Ok(NodeId::Set(key)) - } - - fn add_set(&mut self, set: InternedSystemSet) -> SystemSetKey { - let key = self.system_sets.insert(SystemSetNode::new(set)); - self.system_set_conditions.insert(key, Vec::new()); - self.system_set_ids.insert(set, key); key } @@ -1117,78 +1116,8 @@ impl ScheduleGraph { AnonymousSet::new(id) } - /// Check that no set is included in itself. - /// Add all the sets from the [`GraphInfo`]'s hierarchy to the graph. - fn check_hierarchy_sets( - &mut self, - id: NodeId, - graph_info: &GraphInfo, - ) -> Result<(), ScheduleBuildError> { - for &set in &graph_info.hierarchy { - if let Some(&set_id) = self.system_set_ids.get(&set) { - if let NodeId::Set(key) = id - && set_id == key - { - { - return Err(ScheduleBuildError::HierarchyLoop( - self.get_node_name(&NodeId::Set(key)), - )); - } - } - } else { - // If the set is not in the graph, we add it - self.add_set(set); - } - } - - Ok(()) - } - - /// Checks that no system set is dependent on itself. - /// Add all the sets from the [`GraphInfo`]'s dependencies to the graph. - fn check_edges( - &mut self, - id: NodeId, - graph_info: &GraphInfo, - ) -> Result<(), ScheduleBuildError> { - for Dependency { set, .. } in &graph_info.dependencies { - if let Some(&set_id) = self.system_set_ids.get(set) { - if let NodeId::Set(key) = id - && set_id == key - { - return Err(ScheduleBuildError::DependencyLoop( - self.get_node_name(&NodeId::Set(key)), - )); - } - } else { - // If the set is not in the graph, we add it - self.add_set(*set); - } - } - - Ok(()) - } - - /// Add all the sets from the [`GraphInfo`]'s ambiguity to the graph. - fn add_ambiguities(&mut self, graph_info: &GraphInfo) { - 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); - } - } - } - } - /// Update the internal graphs (hierarchy, dependency, ambiguity) by adding a single [`GraphInfo`] - fn update_graphs( - &mut self, - id: NodeId, - graph_info: GraphInfo, - ) -> Result<(), ScheduleBuildError> { - self.check_hierarchy_sets(id, &graph_info)?; - self.check_edges(id, &graph_info)?; - self.add_ambiguities(&graph_info); + fn update_graphs(&mut self, id: NodeId, graph_info: GraphInfo) { self.changed = true; let GraphInfo { @@ -1201,16 +1130,22 @@ impl ScheduleGraph { self.hierarchy.graph.add_node(id); self.dependency.graph.add_node(id); - for key in sets.into_iter().map(|set| self.system_set_ids[&set]) { + for key in sets + .into_iter() + .map(|set| self.system_sets.get_or_add_set(set)) + { self.hierarchy.graph.add_edge(NodeId::Set(key), id); // ensure set also appears in dependency graph self.dependency.graph.add_node(NodeId::Set(key)); } - for (kind, key, options) in dependencies - .into_iter() - .map(|Dependency { kind, set, options }| (kind, self.system_set_ids[&set], options)) + for (kind, key, options) in + dependencies + .into_iter() + .map(|Dependency { kind, set, options }| { + (kind, self.system_sets.get_or_add_set(set), options) + }) { let (lhs, rhs) = match kind { DependencyKind::Before => (id, NodeId::Set(key)), @@ -1230,7 +1165,7 @@ impl ScheduleGraph { Ambiguity::IgnoreWithSet(ambiguous_with) => { for key in ambiguous_with .into_iter() - .map(|set| self.system_set_ids[&set]) + .map(|set| self.system_sets.get_or_add_set(set)) { self.ambiguous_with.add_edge(id, NodeId::Set(key)); } @@ -1239,8 +1174,6 @@ impl ScheduleGraph { self.ambiguous_with_all.insert(id); } } - - Ok(()) } /// Initializes any newly-added systems and conditions by calling [`System::initialize`](crate::system::System) @@ -1258,7 +1191,7 @@ impl ScheduleGraph { key, first_uninit_condition, } => { - for condition in self.system_set_conditions[key] + for condition in self.system_sets.conditions[key] .iter_mut() .skip(first_uninit_condition) { @@ -1358,9 +1291,9 @@ impl ScheduleGraph { HashMap>, ) { let mut set_systems: HashMap> = - HashMap::with_capacity_and_hasher(self.system_sets.len(), Default::default()); + HashMap::with_capacity_and_hasher(self.system_sets.sets.len(), Default::default()); let mut set_system_sets: HashMap> = - HashMap::with_capacity_and_hasher(self.system_sets.len(), Default::default()); + HashMap::with_capacity_and_hasher(self.system_sets.sets.len(), Default::default()); for &id in hierarchy_topsort.iter().rev() { let NodeId::Set(set_key) = id else { continue; @@ -1559,7 +1492,7 @@ impl ScheduleGraph { // ignore system sets that have no conditions // ignore system type sets (already covered, they don't have conditions) let key = id.as_set()?; - (!self.system_set_conditions[key].is_empty()).then_some((i, key)) + (!self.system_sets.conditions[key].is_empty()).then_some((i, key)) }) .unzip(); @@ -1659,7 +1592,7 @@ impl ScheduleGraph { .drain(..) .zip(schedule.set_conditions.drain(..)) { - self.system_set_conditions[key] = conditions; + self.system_sets.conditions[key] = conditions; } *schedule = self.build_schedule(world, schedule_label, ignored_ambiguities)?; @@ -1673,7 +1606,7 @@ impl ScheduleGraph { } for &key in &schedule.set_ids { - let conditions = core::mem::take(&mut self.system_set_conditions[key]); + let conditions = core::mem::take(&mut self.system_sets.conditions[key]); schedule.set_conditions.push(conditions); } @@ -1700,13 +1633,13 @@ trait ProcessScheduleConfig: Schedulable + Sized { impl ProcessScheduleConfig for ScheduleSystem { fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig) -> NodeId { - schedule_graph.add_system_inner(config).unwrap() + NodeId::System(schedule_graph.add_system_inner(config)) } } impl ProcessScheduleConfig for InternedSystemSet { fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig) -> NodeId { - schedule_graph.configure_set_inner(config).unwrap() + NodeId::Set(schedule_graph.configure_set_inner(config)) } } @@ -1748,7 +1681,7 @@ impl ScheduleGraph { } } NodeId::Set(key) => { - let set = &self.system_sets[key]; + let set = &self.system_sets.sets[key]; if set.is_anonymous() { self.anonymous_set_name(id) } else { @@ -1833,6 +1766,17 @@ impl ScheduleGraph { graph: &DiGraph, report: ReportCycles, ) -> Result, ScheduleBuildError> { + // Check explicitly for self-edges. + // `iter_sccs` won't report them as cycles because they still form components of one node. + if let Some((node, _)) = graph.all_edges().find(|(left, right)| left == right) { + let name = self.get_node_name(&node); + let error = match report { + ReportCycles::Hierarchy => ScheduleBuildError::HierarchyLoop(name), + ReportCycles::Dependency => ScheduleBuildError::DependencyLoop(name), + }; + return Err(error); + } + // Tarjan's SCC algorithm returns elements in *reverse* topological order. let mut top_sorted_nodes = Vec::with_capacity(graph.node_count()); let mut sccs_with_cycles = Vec::new(); @@ -1963,7 +1907,7 @@ impl ScheduleGraph { set_systems: &HashMap>, ) -> Result<(), ScheduleBuildError> { for (&key, systems) in set_systems { - let set = &self.system_sets[key]; + let set = &self.system_sets.sets[key]; if set.is_system_type() { let instances = systems.len(); let ambiguous_with = self.ambiguous_with.edges(NodeId::Set(key)); @@ -2070,7 +2014,7 @@ impl ScheduleGraph { fn names_of_sets_containing_node(&self, id: &NodeId) -> Vec { let mut sets = >::default(); self.traverse_sets_containing_node(*id, &mut |key| { - !self.system_sets[key].is_system_type() && sets.insert(key) + !self.system_sets.sets[key].is_system_type() && sets.insert(key) }); let mut sets: Vec<_> = sets .into_iter() From aa87581cce082532a097a87646c6a2d6d36fd0da Mon Sep 17 00:00:00 2001 From: Talin Date: Tue, 8 Jul 2025 18:07:49 -0700 Subject: [PATCH 5/7] Change CoreWidgets plugin to plugin group. (#20036) What it says on the tin. :) --- crates/bevy_core_widgets/src/lib.rs | 23 +++++++++---------- examples/ui/core_widgets.rs | 4 ++-- examples/ui/core_widgets_observers.rs | 4 ++-- examples/ui/feathers.rs | 4 ++-- .../release-notes/headless-widgets.md | 2 +- 5 files changed, 18 insertions(+), 19 deletions(-) diff --git a/crates/bevy_core_widgets/src/lib.rs b/crates/bevy_core_widgets/src/lib.rs index 2a3fc1ac09..3fc13c5c0e 100644 --- a/crates/bevy_core_widgets/src/lib.rs +++ b/crates/bevy_core_widgets/src/lib.rs @@ -21,7 +21,7 @@ mod core_radio; mod core_scrollbar; mod core_slider; -use bevy_app::{App, Plugin}; +use bevy_app::{PluginGroup, PluginGroupBuilder}; pub use callback::{Callback, Notify}; pub use core_button::{CoreButton, CoreButtonPlugin}; @@ -36,18 +36,17 @@ pub use core_slider::{ SliderRange, SliderStep, SliderValue, TrackClick, }; -/// A plugin that registers the observers for all of the core widgets. If you don't want to +/// A plugin group that registers the observers for all of the core widgets. If you don't want to /// use all of the widgets, you can import the individual widget plugins instead. -pub struct CoreWidgetsPlugin; +pub struct CoreWidgetsPlugins; -impl Plugin for CoreWidgetsPlugin { - fn build(&self, app: &mut App) { - app.add_plugins(( - CoreButtonPlugin, - CoreCheckboxPlugin, - CoreRadioGroupPlugin, - CoreScrollbarPlugin, - CoreSliderPlugin, - )); +impl PluginGroup for CoreWidgetsPlugins { + fn build(self) -> PluginGroupBuilder { + PluginGroupBuilder::start::() + .add(CoreButtonPlugin) + .add(CoreCheckboxPlugin) + .add(CoreRadioGroupPlugin) + .add(CoreScrollbarPlugin) + .add(CoreSliderPlugin) } } diff --git a/examples/ui/core_widgets.rs b/examples/ui/core_widgets.rs index 86aaa820f8..7f99bdd848 100644 --- a/examples/ui/core_widgets.rs +++ b/examples/ui/core_widgets.rs @@ -4,7 +4,7 @@ use bevy::{ color::palettes::basic::*, core_widgets::{ Callback, CoreButton, CoreCheckbox, CoreRadio, CoreRadioGroup, CoreSlider, - CoreSliderDragState, CoreSliderThumb, CoreWidgetsPlugin, SliderRange, SliderValue, + CoreSliderDragState, CoreSliderThumb, CoreWidgetsPlugins, SliderRange, SliderValue, TrackClick, }, input_focus::{ @@ -21,7 +21,7 @@ fn main() { App::new() .add_plugins(( DefaultPlugins, - CoreWidgetsPlugin, + CoreWidgetsPlugins, InputDispatchPlugin, TabNavigationPlugin, )) diff --git a/examples/ui/core_widgets_observers.rs b/examples/ui/core_widgets_observers.rs index 1ab4cda3b0..c12edee08d 100644 --- a/examples/ui/core_widgets_observers.rs +++ b/examples/ui/core_widgets_observers.rs @@ -3,7 +3,7 @@ use bevy::{ color::palettes::basic::*, core_widgets::{ - Callback, CoreButton, CoreCheckbox, CoreSlider, CoreSliderThumb, CoreWidgetsPlugin, + Callback, CoreButton, CoreCheckbox, CoreSlider, CoreSliderThumb, CoreWidgetsPlugins, SliderRange, SliderValue, }, ecs::system::SystemId, @@ -21,7 +21,7 @@ fn main() { App::new() .add_plugins(( DefaultPlugins, - CoreWidgetsPlugin, + CoreWidgetsPlugins, InputDispatchPlugin, TabNavigationPlugin, )) diff --git a/examples/ui/feathers.rs b/examples/ui/feathers.rs index ae6ec31f4c..da8b1faf27 100644 --- a/examples/ui/feathers.rs +++ b/examples/ui/feathers.rs @@ -1,7 +1,7 @@ //! This example shows off the various Bevy Feathers widgets. use bevy::{ - core_widgets::{Callback, CoreRadio, CoreRadioGroup, CoreWidgetsPlugin, SliderStep}, + core_widgets::{Callback, CoreRadio, CoreRadioGroup, CoreWidgetsPlugins, SliderStep}, feathers::{ controls::{ button, checkbox, radio, slider, toggle_switch, ButtonProps, ButtonVariant, @@ -25,7 +25,7 @@ fn main() { App::new() .add_plugins(( DefaultPlugins, - CoreWidgetsPlugin, + CoreWidgetsPlugins, InputDispatchPlugin, TabNavigationPlugin, FeathersPlugin, diff --git a/release-content/release-notes/headless-widgets.md b/release-content/release-notes/headless-widgets.md index 5e2a91c556..5b3ff3dc17 100644 --- a/release-content/release-notes/headless-widgets.md +++ b/release-content/release-notes/headless-widgets.md @@ -1,7 +1,7 @@ --- title: Headless Widgets authors: ["@viridia", "@ickshonpe", "@alice-i-cecile"] -pull_requests: [19366, 19584, 19665, 19778, 19803] +pull_requests: [19366, 19584, 19665, 19778, 19803, 20036] --- Bevy's `Button` and `Interaction` components have been around for a long time. Unfortunately From 6fc4bc126d593b33859a172346be6838578266f5 Mon Sep 17 00:00:00 2001 From: atlv Date: Wed, 9 Jul 2025 02:05:45 -0400 Subject: [PATCH 6/7] Split out spot_light_world_from_view into a function in shadows.wgsl (#20050) # Objective - Improve readability ## Solution - Split a function out ## Testing - spotlight example works --- crates/bevy_pbr/src/render/shadows.wgsl | 28 +++++++++++++++---------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/crates/bevy_pbr/src/render/shadows.wgsl b/crates/bevy_pbr/src/render/shadows.wgsl index 0e539f0009..ef2f73e968 100644 --- a/crates/bevy_pbr/src/render/shadows.wgsl +++ b/crates/bevy_pbr/src/render/shadows.wgsl @@ -62,6 +62,22 @@ fn fetch_point_shadow(light_id: u32, frag_position: vec4, surface_normal: v return sample_shadow_cubemap(frag_ls * flip_z, distance_to_light, depth, light_id); } +// this method of constructing a basis from a vec3 is used by glam::Vec3::any_orthonormal_pair +// so we reproduce it here to avoid a mismatch if glam changes. we also switch the handedness +// the construction of the orthonormal basis up and right vectors needs to precisely mirror the code +// in bevy_light/spot_light.rs:spot_light_world_from_view +fn spot_light_world_from_view(fwd: vec3) -> mat3x3 { + var sign = -1.0; + if (fwd.z >= 0.0) { + sign = 1.0; + } + let a = -1.0 / (fwd.z + sign); + let b = fwd.x * fwd.y * a; + let up_dir = vec3(1.0 + sign * fwd.x * fwd.x * a, sign * b, -sign * fwd.x); + let right_dir = vec3(-b, -sign - fwd.y * fwd.y * a, fwd.y); + return mat3x3(right_dir, up_dir, fwd); +} + fn fetch_spot_shadow( light_id: u32, frag_position: vec4, @@ -88,17 +104,7 @@ fn fetch_spot_shadow( + ((*light).shadow_depth_bias * normalize(surface_to_light)) + (surface_normal.xyz * (*light).shadow_normal_bias) * distance_to_light; - // the construction of the up and right vectors needs to precisely mirror the code - // in render/light.rs:spot_light_view_matrix - var sign = -1.0; - if (fwd.z >= 0.0) { - sign = 1.0; - } - let a = -1.0 / (fwd.z + sign); - let b = fwd.x * fwd.y * a; - let up_dir = vec3(1.0 + sign * fwd.x * fwd.x * a, sign * b, -sign * fwd.x); - let right_dir = vec3(-b, -sign - fwd.y * fwd.y * a, fwd.y); - let light_inv_rot = mat3x3(right_dir, up_dir, fwd); + let light_inv_rot = spot_light_world_from_view(fwd); // because the matrix is a pure rotation matrix, the inverse is just the transpose, and to calculate // the product of the transpose with a vector we can just post-multiply instead of pre-multiplying. From d40c5b54ae3da86feea5070e9fc223b8f791345e Mon Sep 17 00:00:00 2001 From: Brian Reavis Date: Tue, 8 Jul 2025 23:23:44 -0700 Subject: [PATCH 7/7] Material, mesh, skin extraction optimization (#17976) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective The extraction systems for materials, meshes, and skins previously iterated over `RemovedComponents` in addition to more specific variants like `RemovedComponents>`. This caused each system to loop through and check many irrelevant despawned entities—sometimes multiple times. With many material types, this overhead added up and became noticeable in frames with many despawns. Screenshot 2025-02-21 at 10 28 01 AM ## Solution This PR removes superfluous `RemovedComponents` iteration for `ViewVisibility` and `GlobalTransform`, ensuring that we only iterate over the most specific `RemovedComponents` relevant to the system (e.g., material components, mesh components). This is guaranteed to match what the system originally collected. ### Before (red) / After (yellow): Screenshot 2025-02-21 at 10 46 17 AM Log plot to highlight the long tail that this PR is addressing. --- crates/bevy_pbr/src/material.rs | 4 ++-- crates/bevy_pbr/src/render/mesh.rs | 8 +------- crates/bevy_pbr/src/render/skin.rs | 6 +----- crates/bevy_sprite/src/mesh2d/material.rs | 6 +----- 4 files changed, 5 insertions(+), 19 deletions(-) diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index cc3a69a0ad..2e642dba92 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -746,11 +746,11 @@ fn early_sweep_material_instances( /// preparation for a new frame. pub(crate) fn late_sweep_material_instances( mut material_instances: ResMut, - mut removed_visibilities_query: Extract>, + mut removed_meshes_query: Extract>, ) { let last_change_tick = material_instances.current_change_tick; - for entity in removed_visibilities_query.read() { + for entity in removed_meshes_query.read() { if let Entry::Occupied(occupied_entry) = material_instances.instances.entry(entity.into()) { // Only sweep the entry if it wasn't updated this frame. It's // possible that a `ViewVisibility` component was removed and diff --git a/crates/bevy_pbr/src/render/mesh.rs b/crates/bevy_pbr/src/render/mesh.rs index 2e9562c5f8..53b3b4129a 100644 --- a/crates/bevy_pbr/src/render/mesh.rs +++ b/crates/bevy_pbr/src/render/mesh.rs @@ -1452,8 +1452,6 @@ pub fn extract_meshes_for_gpu_building( >, >, all_meshes_query: Extract>, - mut removed_visibilities_query: Extract>, - mut removed_global_transforms_query: Extract>, mut removed_meshes_query: Extract>, gpu_culling_query: Extract, Without)>>, meshes_to_reextract_next_frame: ResMut, @@ -1509,11 +1507,7 @@ pub fn extract_meshes_for_gpu_building( } // Also record info about each mesh that became invisible. - for entity in removed_visibilities_query - .read() - .chain(removed_global_transforms_query.read()) - .chain(removed_meshes_query.read()) - { + for entity in removed_meshes_query.read() { // Only queue a mesh for removal if we didn't pick it up above. // It's possible that a necessary component was removed and re-added in // the same frame. diff --git a/crates/bevy_pbr/src/render/skin.rs b/crates/bevy_pbr/src/render/skin.rs index f9ec672a66..ca035c980a 100644 --- a/crates/bevy_pbr/src/render/skin.rs +++ b/crates/bevy_pbr/src/render/skin.rs @@ -309,7 +309,6 @@ pub fn extract_skins( skinned_mesh_inverse_bindposes: Extract>>, changed_transforms: Extract>>, joints: Extract>, - mut removed_visibilities_query: Extract>, mut removed_skinned_meshes_query: Extract>, ) { let skin_uniforms = skin_uniforms.into_inner(); @@ -335,10 +334,7 @@ pub fn extract_skins( ); // Delete skins that became invisible. - for skinned_mesh_entity in removed_visibilities_query - .read() - .chain(removed_skinned_meshes_query.read()) - { + for skinned_mesh_entity in removed_skinned_meshes_query.read() { // Only remove a skin if we didn't pick it up in `add_or_delete_skins`. // It's possible that a necessary component was removed and re-added in // the same frame. diff --git a/crates/bevy_sprite/src/mesh2d/material.rs b/crates/bevy_sprite/src/mesh2d/material.rs index 06914690ca..4117e39823 100644 --- a/crates/bevy_sprite/src/mesh2d/material.rs +++ b/crates/bevy_sprite/src/mesh2d/material.rs @@ -331,7 +331,6 @@ pub fn extract_mesh_materials_2d( Or<(Changed, Changed>)>, >, >, - mut removed_visibilities_query: Extract>, mut removed_materials_query: Extract>>, ) { for (entity, view_visibility, material) in &changed_meshes_query { @@ -342,10 +341,7 @@ pub fn extract_mesh_materials_2d( } } - for entity in removed_visibilities_query - .read() - .chain(removed_materials_query.read()) - { + for entity in removed_materials_query.read() { // Only queue a mesh for removal if we didn't pick it up above. // It's possible that a necessary component was removed and re-added in // the same frame.