From ebdda09fc3e3aed2306d32f90f93e595b2979660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois?= Date: Mon, 26 Feb 2024 16:55:26 +0100 Subject: [PATCH] make gizmo groups ordering stable when rendering (#12034) # Objective - During rendering of different gizmo groups, the ordering is random between executions ## Solution - Make the ordering stable - It's using a `TypeIdMap`, its iteration order depends on the insertion order. so insert when adding a group instead of when adding a gizmo - Also changed `extract_gizmo_data` to not be group dependent. there will be only one of those systems, no matter the number of gizmo groups added --------- Co-authored-by: pablo-lua <126117294+pablo-lua@users.noreply.github.com> --- crates/bevy_gizmos/src/lib.rs | 116 ++++++++++++++++++---------------- 1 file changed, 63 insertions(+), 53 deletions(-) diff --git a/crates/bevy_gizmos/src/lib.rs b/crates/bevy_gizmos/src/lib.rs index 249db71d31..caeae3a5d1 100644 --- a/crates/bevy_gizmos/src/lib.rs +++ b/crates/bevy_gizmos/src/lib.rs @@ -116,6 +116,8 @@ impl Plugin for GizmoPlugin { prepare_line_gizmo_bind_group.in_set(RenderSet::PrepareBindGroups), ); + render_app.add_systems(ExtractSchedule, extract_gizmo_data); + #[cfg(feature = "bevy_sprite")] app.add_plugins(pipeline_2d::LineGizmo2dPlugin); #[cfg(feature = "bevy_pbr")] @@ -163,6 +165,12 @@ impl AppGizmoBuilder for App { return self; } + let mut handles = self + .world + .get_resource_or_insert_with::(Default::default); + handles.list.insert(TypeId::of::(), None); + handles.strip.insert(TypeId::of::(), None); + self.init_resource::>() .add_systems(Last, update_gizmo_meshes::); @@ -170,12 +178,6 @@ impl AppGizmoBuilder for App { .get_resource_or_insert_with::(Default::default) .register::(); - let Ok(render_app) = self.get_sub_app_mut(RenderApp) else { - return self; - }; - - render_app.add_systems(ExtractSchedule, extract_gizmo_data::); - self } @@ -192,23 +194,28 @@ impl AppGizmoBuilder for App { return self; } + let mut handles = self + .world + .get_resource_or_insert_with::(Default::default); + handles.list.insert(TypeId::of::(), None); + handles.strip.insert(TypeId::of::(), None); + self.init_resource::>() .add_systems(Last, update_gizmo_meshes::); - let Ok(render_app) = self.get_sub_app_mut(RenderApp) else { - return self; - }; - - render_app.add_systems(ExtractSchedule, extract_gizmo_data::); - self } } +/// Holds handles to the line gizmos for each gizmo configuration group +// As `TypeIdMap` iteration order depends on the order of insertions and deletions, this uses +// `Option` to be able to reserve the slot when creating the gizmo configuration group. +// That way iteration order is stable accross executions and depends on the order of configuration +// group creation. #[derive(Resource, Default)] struct LineGizmoHandles { - list: TypeIdMap>, - strip: TypeIdMap>, + list: TypeIdMap>>, + strip: TypeIdMap>>, } fn update_gizmo_meshes( @@ -217,63 +224,66 @@ fn update_gizmo_meshes( mut storage: ResMut>, ) { if storage.list_positions.is_empty() { - handles.list.remove(&TypeId::of::()); - } else if let Some(handle) = handles.list.get(&TypeId::of::()) { - let list = line_gizmos.get_mut(handle).unwrap(); + handles.list.insert(TypeId::of::(), None); + } else if let Some(handle) = handles.list.get_mut(&TypeId::of::()) { + if let Some(handle) = handle { + let list = line_gizmos.get_mut(handle.id()).unwrap(); - list.positions = mem::take(&mut storage.list_positions); - list.colors = mem::take(&mut storage.list_colors); - } else { - let mut list = LineGizmo { - strip: false, - ..Default::default() - }; + list.positions = mem::take(&mut storage.list_positions); + list.colors = mem::take(&mut storage.list_colors); + } else { + let mut list = LineGizmo { + strip: false, + ..Default::default() + }; - list.positions = mem::take(&mut storage.list_positions); - list.colors = mem::take(&mut storage.list_colors); + list.positions = mem::take(&mut storage.list_positions); + list.colors = mem::take(&mut storage.list_colors); - handles - .list - .insert(TypeId::of::(), line_gizmos.add(list)); + *handle = Some(line_gizmos.add(list)); + } } if storage.strip_positions.is_empty() { - handles.strip.remove(&TypeId::of::()); - } else if let Some(handle) = handles.strip.get(&TypeId::of::()) { - let strip = line_gizmos.get_mut(handle).unwrap(); + handles.strip.insert(TypeId::of::(), None); + } else if let Some(handle) = handles.strip.get_mut(&TypeId::of::()) { + if let Some(handle) = handle { + let strip = line_gizmos.get_mut(handle.id()).unwrap(); - strip.positions = mem::take(&mut storage.strip_positions); - strip.colors = mem::take(&mut storage.strip_colors); - } else { - let mut strip = LineGizmo { - strip: true, - ..Default::default() - }; + strip.positions = mem::take(&mut storage.strip_positions); + strip.colors = mem::take(&mut storage.strip_colors); + } else { + let mut strip = LineGizmo { + strip: true, + ..Default::default() + }; - strip.positions = mem::take(&mut storage.strip_positions); - strip.colors = mem::take(&mut storage.strip_colors); + strip.positions = mem::take(&mut storage.strip_positions); + strip.colors = mem::take(&mut storage.strip_colors); - handles - .strip - .insert(TypeId::of::(), line_gizmos.add(strip)); + *handle = Some(line_gizmos.add(strip)); + } } } -fn extract_gizmo_data( +fn extract_gizmo_data( mut commands: Commands, handles: Extract>, config: Extract>, ) { - let (config, _) = config.config::(); - - if !config.enabled { - return; - } - - for map in [&handles.list, &handles.strip].into_iter() { - let Some(handle) = map.get(&TypeId::of::()) else { + for (group_type_id, handle) in handles.list.iter().chain(handles.strip.iter()) { + let Some((config, _)) = config.get_config_dyn(group_type_id) else { continue; }; + + if !config.enabled { + continue; + } + + let Some(handle) = handle else { + continue; + }; + commands.spawn(( LineGizmoUniform { line_width: config.line_width,