From 0b302db360fb7ea0c35159bb776dc76e0f441b78 Mon Sep 17 00:00:00 2001 From: Grey Date: Tue, 7 Jan 2025 11:37:19 +0800 Subject: [PATCH 1/2] Update readme so bindgen installation matches what we need (#262) Signed-off-by: Michael X. Grey --- README.md | 3 ++- rmf_site_editor_web/Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 34cb49f3..2d3c30e2 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,8 @@ Make sure you install rust from the main rust website. Cargo should take care of These are only needed if you're going to build a WebAssembly binary: ```bash $ sudo apt install binaryen -$ cargo install wasm-bindgen-cli basic-http-server +$ cargo install -f wasm-bindgen-cli --version 0.2.93 +$ cargo install basic-http-server $ rustup target add wasm32-unknown-unknown ``` diff --git a/rmf_site_editor_web/Cargo.toml b/rmf_site_editor_web/Cargo.toml index 98a0cd33..8c9db7eb 100644 --- a/rmf_site_editor_web/Cargo.toml +++ b/rmf_site_editor_web/Cargo.toml @@ -8,6 +8,6 @@ crate-type = ["cdylib", "rlib"] name = "librmf_site_editor_web" [dependencies] -wasm-bindgen = "=0.2.93" +wasm-bindgen = "=0.2.93" # Remember to update the README if we change this version number rmf_site_editor = { path = "../rmf_site_editor" } console_error_panic_hook = "0.1.7" From a83a94b1dc5aa1bbc27d3762ef33a3989d56eba2 Mon Sep 17 00:00:00 2001 From: Xiyu Date: Thu, 9 Jan 2025 10:08:17 +0800 Subject: [PATCH 2/2] Do not query SiteParent (#260) * Do not query SiteParent Signed-off-by: Xiyu Oh * Add diagnostic if unable to add to current level Signed-off-by: Xiyu Oh * Fix merge conflicts Signed-off-by: Michael X. Grey * Remove misalignment and fix deviation in commit history Signed-off-by: Xiyu Oh * Use Parented Signed-off-by: Xiyu Oh * Review comments Signed-off-by: Xiyu Oh --------- Signed-off-by: Xiyu Oh Signed-off-by: Michael X. Grey Co-authored-by: Michael X. Grey --- rmf_site_editor/src/interaction/cursor.rs | 12 ++--- rmf_site_editor/src/site/load.rs | 16 +++--- rmf_site_editor/src/site/mod.rs | 2 +- rmf_site_editor/src/site/model.rs | 60 ++++++++++++++++------ rmf_site_editor/src/site/save.rs | 43 +++++++--------- rmf_site_editor/src/site/scenario.rs | 15 ++---- rmf_site_format/src/legacy/building_map.rs | 6 +-- rmf_site_format/src/legacy/model.rs | 13 +++-- rmf_site_format/src/misc.rs | 37 ++----------- rmf_site_format/src/model.rs | 3 -- rmf_site_format/src/sdf.rs | 40 ++++++--------- rmf_site_format/src/site.rs | 2 +- 12 files changed, 113 insertions(+), 136 deletions(-) diff --git a/rmf_site_editor/src/interaction/cursor.rs b/rmf_site_editor/src/interaction/cursor.rs index 48606ff2..d173f380 100644 --- a/rmf_site_editor/src/interaction/cursor.rs +++ b/rmf_site_editor/src/interaction/cursor.rs @@ -127,13 +127,11 @@ impl Cursor { model_instance: Option>, ) { self.remove_preview(commands); - self.preview_model = model_instance.and_then(|model_instance| { - Some( - model_loader - .spawn_model_instance(self.frame, model_instance) - .insert(Pending) - .id(), - ) + self.preview_model = model_instance.map(|instance| { + model_loader + .spawn_model_instance(self.frame, instance) + .insert(Pending) + .id() }); } diff --git a/rmf_site_editor/src/site/load.rs b/rmf_site_editor/src/site/load.rs index aa32f34e..be23e2a3 100644 --- a/rmf_site_editor/src/site/load.rs +++ b/rmf_site_editor/src/site/load.rs @@ -362,16 +362,20 @@ fn generate_site_entities( } } - for (model_instance_id, model_instance_data) in &site_data.model_instances { - let model_instance = model_instance_data + for (model_instance_id, parented_model_instance) in &site_data.model_instances { + let model_instance = parented_model_instance + .bundle .convert(&id_to_entity) .for_site(site_id)?; + // The parent id is invalid, we do not spawn this model instance and generate + // an error instead + let parent = id_to_entity + .get(&parented_model_instance.parent) + .ok_or_else(|| LoadSiteError::new(site_id, parented_model_instance.parent))?; + let model_instance_entity = model_loader - .spawn_model_instance( - model_instance.parent.0.unwrap_or(site_id), - model_instance.clone(), - ) + .spawn_model_instance(*parent, model_instance.clone()) .insert((Category::Model, SiteID(*model_instance_id))) .id(); id_to_entity.insert(*model_instance_id, model_instance_entity); diff --git a/rmf_site_editor/src/site/mod.rs b/rmf_site_editor/src/site/mod.rs index 2bbc59d2..10f3be71 100644 --- a/rmf_site_editor/src/site/mod.rs +++ b/rmf_site_editor/src/site/mod.rs @@ -281,6 +281,7 @@ impl Plugin for SitePlugin { check_for_duplicated_dock_names, check_for_fiducials_without_affiliation, check_for_close_unconnected_anchors, + check_for_orphan_model_instances, fetch_image_for_texture, detect_last_selected_texture::, apply_last_selected_texture:: @@ -305,7 +306,6 @@ impl Plugin for SitePlugin { assign_orphan_levels_to_site, assign_orphan_nav_elements_to_site, assign_orphan_fiducials_to_parent, - assign_orphan_model_instances_to_level, assign_orphan_elements_to_level::, assign_orphan_elements_to_level::, assign_orphan_elements_to_level::, diff --git a/rmf_site_editor/src/site/model.rs b/rmf_site_editor/src/site/model.rs index af8999d4..3c28ad2e 100644 --- a/rmf_site_editor/src/site/model.rs +++ b/rmf_site_editor/src/site/model.rs @@ -17,8 +17,9 @@ use crate::{ interaction::{DragPlaneBundle, Preview, MODEL_PREVIEW_LAYER}, - site::{CurrentLevel, SiteAssets, SiteParent}, + site::SiteAssets, site_asset_io::MODEL_ENVIRONMENT_VARIABLE, + Issue, ValidateWorkspace, }; use bevy::{ ecs::system::{EntityCommands, SystemParam}, @@ -26,11 +27,13 @@ use bevy::{ prelude::*, render::view::RenderLayers, scene::SceneInstance, + utils::Uuid, }; use bevy_impulse::*; use bevy_mod_outline::OutlineMeshExt; use rmf_site_format::{ - Affiliation, AssetSource, Group, ModelInstance, ModelMarker, ModelProperty, Pending, Scale, + Affiliation, AssetSource, Group, IssueKey, ModelInstance, ModelMarker, ModelProperty, + NameInSite, Pending, Scale, }; use smallvec::SmallVec; use std::{any::TypeId, collections::HashSet, fmt, future::Future}; @@ -783,25 +786,48 @@ pub fn update_model_instances( } } -pub fn assign_orphan_model_instances_to_level( +/// Unique UUID to identify issue of orphan model instance +pub const ORPHAN_MODEL_INSTANCE_ISSUE_UUID: Uuid = + Uuid::from_u128(0x4e98ce0bc28e4fe528cb0a028f4d5c08u128); + +pub fn check_for_orphan_model_instances( mut commands: Commands, + mut validate_events: EventReader, mut orphan_instances: Query< - (Entity, Option<&Parent>, &mut SiteParent), - (With, Without), + (Entity, &NameInSite, &Affiliation), + (With, Without, Without), >, - current_level: Res, + model_descriptions: Query<&NameInSite, (With, With)>, ) { - let current_level = match current_level.0 { - Some(c) => c, - None => return, - }; - - for (instance_entity, parent, mut site_parent) in orphan_instances.iter_mut() { - if parent.is_none() { - commands.entity(current_level).add_child(instance_entity); - } - if site_parent.0.is_none() { - site_parent.0 = Some(current_level); + for root in validate_events.read() { + for (instance_entity, instance_name, affiliation) in orphan_instances.iter_mut() { + let brief = match affiliation + .0 + .map(|e| model_descriptions.get(e).ok()) + .flatten() + { + Some(description_name) => format!( + "Parent level entity not found for model instance {:?} with \ + affiliated model description {:?}", + instance_name, description_name + ), + None => format!( + "Parent level entity not found for model instance {:?} when saving", + instance_name, + ), + }; + let issue = Issue { + key: IssueKey { + entities: [instance_entity].into(), + kind: ORPHAN_MODEL_INSTANCE_ISSUE_UUID, + }, + brief, + hint: "Model instances need to be assigned to a parent level entity. \ + Respawn the orphan model instance" + .to_string(), + }; + let issue_id = commands.spawn(issue).id(); + commands.entity(**root).add_child(issue_id); } } } diff --git a/rmf_site_editor/src/site/save.rs b/rmf_site_editor/src/site/save.rs index a05e863d..13e1f6eb 100644 --- a/rmf_site_editor/src/site/save.rs +++ b/rmf_site_editor/src/site/save.rs @@ -1250,18 +1250,11 @@ fn generate_model_descriptions( fn generate_model_instances( site: Entity, world: &mut World, -) -> Result>, SiteGenerationError> { +) -> Result>>, SiteGenerationError> { let mut state: SystemState<( Query<&SiteID, (With, With, Without)>, Query< - ( - Entity, - &SiteID, - &NameInSite, - &Pose, - &SiteParent, - &Affiliation, - ), + (Entity, &SiteID, &NameInSite, &Pose, &Affiliation), (With, Without, Without), >, Query<(Entity, &SiteID), With>, @@ -1279,20 +1272,15 @@ fn generate_model_instances( site_levels_ids.insert(level_entity, site_id.0); } } - let mut res = BTreeMap::>::new(); - for ( - _instance_entity, - instance_id, - instance_name, - instance_pose, - instance_parent, - instance_affiliation, - ) in model_instances.iter() + let mut res = BTreeMap::>>::new(); + for (instance_entity, instance_id, instance_name, instance_pose, instance_affiliation) in + model_instances.iter() { - let Ok(parent) = instance_parent - .0 - .map(|p| site_levels_ids.get(&p).copied().ok_or(())) - .transpose() + let Some(level_id) = parents + .get(instance_entity) + .ok() + .map(|p| site_levels_ids.get(&p.get()).copied()) + .flatten() else { error!("Unable to find parent for instance [{}]", instance_name.0); continue; @@ -1300,7 +1288,6 @@ fn generate_model_instances( let mut model_instance = ModelInstance:: { name: instance_name.clone(), pose: instance_pose.clone(), - parent: SiteParent(parent), description: Affiliation( instance_affiliation .0 @@ -1309,7 +1296,7 @@ fn generate_model_instances( ), ..Default::default() }; - if let Ok(robot_tasks) = tasks.get(_instance_entity) { + if let Ok(robot_tasks) = tasks.get(instance_entity) { let tasks: Vec> = robot_tasks .0 .clone() @@ -1333,7 +1320,13 @@ fn generate_model_instances( .0 .push(OptionalModelProperty::Tasks(Tasks(tasks.clone()))); } - res.insert(instance_id.0, model_instance); + res.insert( + instance_id.0, + Parented { + parent: level_id, + bundle: model_instance, + }, + ); } Ok(res) } diff --git a/rmf_site_editor/src/site/scenario.rs b/rmf_site_editor/src/site/scenario.rs index f68240ca..8856d796 100644 --- a/rmf_site_editor/src/site/scenario.rs +++ b/rmf_site_editor/src/site/scenario.rs @@ -19,7 +19,7 @@ use crate::{ interaction::{Select, Selection}, site::{ Affiliation, CurrentScenario, Delete, DeletionBox, DeletionFilters, Dependents, - InstanceMarker, Pending, Pose, Scenario, ScenarioBundle, ScenarioMarker, SiteParent, + InstanceMarker, Pending, Pose, Scenario, ScenarioBundle, ScenarioMarker, }, CurrentWorkspace, }; @@ -37,10 +37,7 @@ pub fn update_current_scenario( mut current_scenario: ResMut, current_workspace: Res, scenarios: Query<&Scenario>, - mut instances: Query< - (Entity, &mut Pose, &SiteParent, &mut Visibility), - With, - >, + mut instances: Query<(Entity, &mut Pose, &mut Visibility), With>, ) { if let Some(ChangeCurrentScenario(scenario_entity)) = change_current_scenario.read().last() { // Used to build a scenario from root @@ -79,14 +76,8 @@ pub fn update_current_scenario( }; // If active, assign parent to level, otherwise assign parent to site - for (entity, mut pose, parent, mut visibility) in instances.iter_mut() { + for (entity, mut pose, mut visibility) in instances.iter_mut() { if let Some(new_pose) = active_instances.get(&entity) { - if let Some(parent_entity) = parent.0 { - commands.entity(entity).set_parent(parent_entity); - } else { - commands.entity(entity).set_parent(current_site_entity); - warn!("Model instance {:?} has no valid site parent", entity); - } *pose = new_pose.clone(); *visibility = Visibility::Inherited; } else { diff --git a/rmf_site_format/src/legacy/building_map.rs b/rmf_site_format/src/legacy/building_map.rs index 3c47f65b..d5570963 100644 --- a/rmf_site_format/src/legacy/building_map.rs +++ b/rmf_site_format/src/legacy/building_map.rs @@ -7,8 +7,8 @@ use crate::{ Drawing as SiteDrawing, DrawingProperties, Fiducial as SiteFiducial, FiducialGroup, FiducialMarker, Guided, Lane as SiteLane, LaneMarker, Level as SiteLevel, LevelElevation, LevelProperties as SiteLevelProperties, ModelDescriptionBundle, ModelInstance, Motion, - NameInSite, NameOfSite, NavGraph, Navigation, OrientationConstraint, PixelsPerMeter, Pose, - PreferredSemiTransparency, RankingsInLevel, ReverseLane, Rotation, ScenarioBundle, Site, + NameInSite, NameOfSite, NavGraph, Navigation, OrientationConstraint, Parented, PixelsPerMeter, + Pose, PreferredSemiTransparency, RankingsInLevel, ReverseLane, Rotation, ScenarioBundle, Site, SiteProperties, Texture as SiteTexture, TextureGroup, UserCameraPose, DEFAULT_NAV_GRAPH_COLORS, }; use glam::{DAffine2, DMat3, DQuat, DVec2, DVec3, EulerRot}; @@ -203,7 +203,7 @@ impl BuildingMap { let mut cartesian_fiducials: HashMap> = HashMap::new(); let mut model_descriptions: BTreeMap> = BTreeMap::new(); - let mut model_instances: BTreeMap> = BTreeMap::new(); + let mut model_instances: BTreeMap>> = BTreeMap::new(); let mut model_description_name_map = HashMap::::new(); let mut scenarios: BTreeMap> = BTreeMap::new(); let default_scenario_id = site_id.next().unwrap(); diff --git a/rmf_site_format/src/legacy/model.rs b/rmf_site_format/src/legacy/model.rs index 2ede13c0..47ec3e66 100644 --- a/rmf_site_format/src/legacy/model.rs +++ b/rmf_site_format/src/legacy/model.rs @@ -1,6 +1,6 @@ use crate::{ Affiliation, Angle, AssetSource, InstanceMarker, IsStatic, ModelDescriptionBundle, - ModelInstance, ModelMarker, ModelProperty, NameInSite, Pose, Rotation, Scale, SiteParent, + ModelInstance, ModelMarker, ModelProperty, NameInSite, Parented, Pose, Rotation, Scale, }; use glam::DVec2; use serde::{Deserialize, Serialize}; @@ -33,7 +33,7 @@ impl Model { &self, model_description_name_map: &mut HashMap, model_descriptions: &mut BTreeMap>, - model_instances: &mut BTreeMap>, + model_instances: &mut BTreeMap>>, site_id: &mut RangeFrom, level_id: u32, ) -> (u32, Pose) { @@ -64,13 +64,18 @@ impl Model { let model_instance = ModelInstance { name: NameInSite(self.instance_name.clone()), pose: pose.clone(), - parent: SiteParent(Some(level_id)), description: Affiliation(Some(model_description_id)), marker: ModelMarker, instance_marker: InstanceMarker, ..Default::default() }; - model_instances.insert(model_instance_id, model_instance); + model_instances.insert( + model_instance_id, + Parented { + parent: level_id, + bundle: model_instance, + }, + ); (model_instance_id, pose) } } diff --git a/rmf_site_format/src/misc.rs b/rmf_site_format/src/misc.rs index 6637b4b3..2a2c25fb 100644 --- a/rmf_site_format/src/misc.rs +++ b/rmf_site_format/src/misc.rs @@ -472,38 +472,11 @@ pub struct PreviewableMarker; #[cfg_attr(feature = "bevy", derive(Component, Deref, DerefMut))] pub struct SiteID(pub u32); -/// This component is applied to an entity as a reference to its parent entity. -#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] -#[serde(transparent)] -#[cfg_attr(feature = "bevy", derive(Component, Reflect))] -pub struct SiteParent(pub Option); - -impl From for SiteParent { - fn from(value: T) -> Self { - SiteParent(Some(value)) - } -} - -impl From> for SiteParent { - fn from(value: Option) -> Self { - SiteParent(value) - } -} - -impl Default for SiteParent { - fn default() -> Self { - SiteParent(None) - } -} - -impl SiteParent { - pub fn convert(&self, id_map: &HashMap) -> Result, T> { - if let Some(x) = self.0 { - Ok(SiteParent(Some(id_map.get(&x).ok_or(x)?.clone()))) - } else { - Ok(SiteParent(None)) - } - } +/// Helper structure to serialize / deserialize entities with parents +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct Parented { + pub parent: P, + pub bundle: T, } /// The Pending component indicates that an element is not yet ready to be diff --git a/rmf_site_format/src/model.rs b/rmf_site_format/src/model.rs index bde4f531..739a3d20 100644 --- a/rmf_site_format/src/model.rs +++ b/rmf_site_format/src/model.rs @@ -158,7 +158,6 @@ impl Default for ModelDescriptionBundle { pub struct ModelInstance { pub name: NameInSite, pub pose: Pose, - pub parent: SiteParent, pub description: Affiliation, #[serde(skip)] pub marker: ModelMarker, @@ -172,7 +171,6 @@ impl Default for ModelInstance { Self { name: NameInSite("".to_string()), pose: Pose::default(), - parent: SiteParent::default(), description: Affiliation::default(), marker: ModelMarker, instance_marker: InstanceMarker, @@ -186,7 +184,6 @@ impl ModelInstance { Ok(ModelInstance { name: self.name.clone(), pose: self.pose.clone(), - parent: self.parent.convert(id_map)?, description: self.description.convert(id_map)?, optional_properties: self.optional_properties.convert(id_map)?, ..Default::default() diff --git a/rmf_site_format/src/sdf.rs b/rmf_site_format/src/sdf.rs index 4397d138..c0970d7a 100644 --- a/rmf_site_format/src/sdf.rs +++ b/rmf_site_format/src/sdf.rs @@ -497,36 +497,26 @@ impl Site { // TODO(luca) We need this because there is no concept of ingestor or dispenser in // rmf_site yet. Remove when there is for (model_instance_id, _) in &default_scenario.scenario.added_instances { - let model_instance = self.model_instances.get(model_instance_id).ok_or( + let parented_model_instance = self.model_instances.get(model_instance_id).ok_or( SdfConversionError::BrokenModelInstanceReference(*model_instance_id), )?; - let (model_description_id, model_description_bundle) = - if let Some(model_description_id) = model_instance.description.0 { - ( - model_description_id, - self.model_descriptions.get(&model_description_id).ok_or( - SdfConversionError::BrokenModelDescriptionReference( - *model_instance_id, - ), - )?, - ) - } else { - continue; - }; - - if model_instance.parent.0.is_none() - || model_instance.parent.0.is_some_and(|x| x != *level_id) - { + let Some(model_description_id) = parented_model_instance.bundle.description.0 + else { continue; - } + }; + let model_description_bundle = + self.model_descriptions.get(&model_description_id).ok_or( + SdfConversionError::BrokenModelDescriptionReference(*model_instance_id), + )?; + let mut added = false; if model_description_bundle.source.0 == AssetSource::Search("OpenRobotics/TeleportIngestor".to_string()) { world.include.push(SdfWorldInclude { uri: "model://TeleportIngestor".to_string(), - name: Some(model_instance.name.0.clone()), - pose: Some(model_instance.pose.to_sdf()), + name: Some(parented_model_instance.bundle.name.0.clone()), + pose: Some(parented_model_instance.bundle.pose.to_sdf()), ..Default::default() }); added = true; @@ -535,8 +525,8 @@ impl Site { { world.include.push(SdfWorldInclude { uri: "model://TeleportDispenser".to_string(), - name: Some(model_instance.name.0.clone()), - pose: Some(model_instance.pose.to_sdf()), + name: Some(parented_model_instance.bundle.name.0.clone()), + pose: Some(parented_model_instance.bundle.pose.to_sdf()), ..Default::default() }); added = true; @@ -546,9 +536,9 @@ impl Site { // NameInSite instead of AssetSource for the URI, fix else if !model_description_bundle.is_static.0 .0 { world.model.push(SdfModel { - name: model_instance.name.0.clone(), + name: parented_model_instance.bundle.name.0.clone(), r#static: Some(model_description_bundle.is_static.0 .0), - pose: Some(model_instance.pose.to_sdf()), + pose: Some(parented_model_instance.bundle.pose.to_sdf()), link: vec![SdfLink { name: "link".into(), collision: vec![SdfCollision { diff --git a/rmf_site_format/src/site.rs b/rmf_site_format/src/site.rs index bb7301b4..9ded86e1 100644 --- a/rmf_site_format/src/site.rs +++ b/rmf_site_format/src/site.rs @@ -147,7 +147,7 @@ pub struct Site { pub model_descriptions: BTreeMap>, /// Model instances that exist in the site #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub model_instances: BTreeMap>, + pub model_instances: BTreeMap>>, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]