diff --git a/src/resolution.rs b/src/resolution.rs index 1e743efd..fe657934 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -8,7 +8,7 @@ use rbx_dom_weak::types::{ use rbx_reflection::{DataType, PropertyDescriptor}; use serde::{Deserialize, Serialize}; -use crate::REF_POINTER_ATTRIBUTE_PREFIX; +use crate::REF_PATH_ATTRIBUTE_PREFIX; /// A user-friendly version of `Variant` that supports specifying ambiguous /// values. Ambiguous values need a reflection database to be resolved to a @@ -245,8 +245,8 @@ impl AmbiguousValue { } (VariantType::Ref, AmbiguousValue::String(_)) => Err(format_err!( - "Cannot resolve Ref properties as a String.\ - Use an attribute named `{REF_POINTER_ATTRIBUTE_PREFIX}{prop_name}" + "Cannot resolve Ref properties as a String. \ + Use an attribute named `{REF_PATH_ATTRIBUTE_PREFIX}{prop_name}` with the path to the target instance" )), (_, unresolved) => Err(format_err!( "Wrong type of value for property {}.{}. Expected {:?}, got {}", diff --git a/src/rojo_ref.rs b/src/rojo_ref.rs index ade06f78..e9a42a3c 100644 --- a/src/rojo_ref.rs +++ b/src/rojo_ref.rs @@ -2,11 +2,14 @@ use std::{fmt, sync::Arc}; use serde::{Deserialize, Serialize}; +/// Legacy: ID attribute on target instances (kept for backwards compatibility) pub const REF_ID_ATTRIBUTE_NAME: &str = "Rojo_Id"; +/// Legacy: Pointer attribute prefix using IDs (kept for backwards compatibility) pub const REF_POINTER_ATTRIBUTE_PREFIX: &str = "Rojo_Target_"; -/// Prefix for attributes storing paths to external references (instances outside -/// the sync tree, like SoundGroups in SoundService). -pub const REF_EXTERNAL_ATTRIBUTE_PREFIX: &str = "Rojo_External_"; +/// Prefix for path-based reference attributes. This is the preferred system +/// that stores the path to the target instance (e.g., "SoundService/Effects"). +/// No modification to the target instance is needed. +pub const REF_PATH_ATTRIBUTE_PREFIX: &str = "Rojo_Ref_"; // TODO add an internment strategy for RojoRefs // Something like what rbx-dom does for SharedStrings probably works diff --git a/src/snapshot/patch_apply.rs b/src/snapshot/patch_apply.rs index a8d8a40e..cf077011 100644 --- a/src/snapshot/patch_apply.rs +++ b/src/snapshot/patch_apply.rs @@ -15,7 +15,7 @@ use super::{ InstanceSnapshot, RojoTree, }; use crate::{ - multimap::MultiMap, RojoRef, REF_EXTERNAL_ATTRIBUTE_PREFIX, REF_ID_ATTRIBUTE_NAME, + multimap::MultiMap, RojoRef, REF_ID_ATTRIBUTE_NAME, REF_PATH_ATTRIBUTE_PREFIX, REF_POINTER_ATTRIBUTE_PREFIX, }; @@ -79,14 +79,18 @@ struct PatchApplyContext { /// to be rewritten. has_refs_to_rewrite: HashSet, - /// Tracks all ref properties that were specified using attributes. This has - /// to be handled after everything else is done just like normal referent - /// properties. + /// Legacy: Tracks ref properties using the old ID-based system (Rojo_Target_). + /// Kept for backwards compatibility with existing projects. attribute_refs_to_rewrite: MultiMap, - /// Tracks external ref properties (references to instances outside the sync - /// tree, like SoundGroups in SoundService). These are stored as paths. - external_refs_to_rewrite: MultiMap, + /// Tracks path-based ref properties (Rojo_Ref_). This is the preferred system + /// that stores paths to target instances. + path_refs_to_rewrite: MultiMap, + + /// Tracks instances that have Rojo ref attributes that need to be cleaned up + /// after resolution. The attributes are only used as transport in the file + /// format and should not persist in the live game. + instances_needing_attr_cleanup: HashSet, /// The current applied patch result, describing changes made to the tree. applied_patch_set: AppliedPatchSet, @@ -136,33 +140,64 @@ fn finalize_patch_application(context: PatchApplyContext, tree: &mut RojoTree) - instance.properties_mut().extend(real_rewrites.drain(..)); } - // Handle external path-based references (e.g., SoundGroup pointing to SoundService) - let mut external_rewrites = Vec::new(); - for (id, map) in context.external_refs_to_rewrite { + // Handle path-based references (Rojo_Ref_) + let mut path_rewrites = Vec::new(); + for (id, map) in context.path_refs_to_rewrite { for (prop_name, path) in map { if let Some(target) = tree.get_instance_by_path(&path) { log::debug!( - "Resolved external reference {} -> {} (path: {})", + "Resolved path reference {} -> {} (path: {})", prop_name, target, path ); - external_rewrites.push((prop_name, Variant::Ref(target))); + path_rewrites.push((prop_name, Variant::Ref(target))); } else { log::debug!( - "Could not resolve external reference {} at path '{}' - instance not found in tree", + "Could not resolve path reference {} at path '{}' - instance not found in tree", prop_name, path ); } } - if !external_rewrites.is_empty() { + if !path_rewrites.is_empty() { let mut instance = tree .get_instance_mut(id) - .expect("Invalid instance ID in deferred external ref map"); - instance - .properties_mut() - .extend(external_rewrites.drain(..)); + .expect("Invalid instance ID in deferred path ref map"); + instance.properties_mut().extend(path_rewrites.drain(..)); + } + } + + // Clean up Rojo ref attributes - they're only used for transport in the file + // format and should not persist in the live game + for id in context.instances_needing_attr_cleanup { + let mut instance = match tree.get_instance_mut(id) { + Some(inst) => inst, + None => continue, + }; + + let attributes = match instance.properties_mut().get_mut(&ustr("Attributes")) { + Some(Variant::Attributes(attrs)) => attrs, + _ => continue, + }; + + // Remove all Rojo ref-related attributes + let keys_to_remove: Vec = attributes + .iter() + .filter_map(|(name, _)| { + if name == REF_ID_ATTRIBUTE_NAME + || name.starts_with(REF_POINTER_ATTRIBUTE_PREFIX) + || name.starts_with(REF_PATH_ATTRIBUTE_PREFIX) + { + Some(name.to_string()) + } else { + None + } + }) + .collect(); + + for key in keys_to_remove { + attributes.remove(key); } } @@ -312,6 +347,7 @@ fn defer_ref_properties(tree: &mut RojoTree, id: Ref, context: &mut PatchApplyCo } } if let Some(prop_name) = attr_name.strip_prefix(REF_POINTER_ATTRIBUTE_PREFIX) { + context.instances_needing_attr_cleanup.insert(id); if let Variant::String(prop_value) = attr_value { context .attribute_refs_to_rewrite @@ -332,20 +368,21 @@ fn defer_ref_properties(tree: &mut RojoTree, id: Ref, context: &mut PatchApplyCo ) } } - // Handle external path-based references (for instances outside the sync tree) - if let Some(prop_name) = attr_name.strip_prefix(REF_EXTERNAL_ATTRIBUTE_PREFIX) { + // Handle path-based references (Rojo_Ref_) + if let Some(prop_name) = attr_name.strip_prefix(REF_PATH_ATTRIBUTE_PREFIX) { + context.instances_needing_attr_cleanup.insert(id); if let Variant::String(path) = attr_value { context - .external_refs_to_rewrite + .path_refs_to_rewrite .insert(id, (ustr(prop_name), path.clone())); } else if let Variant::BinaryString(path) = attr_value { if let Ok(str) = std::str::from_utf8(path.as_ref()) { context - .external_refs_to_rewrite + .path_refs_to_rewrite .insert(id, (ustr(prop_name), str.to_string())); } else { log::error!( - "External paths specified by referent property attributes must be valid UTF-8 strings." + "Paths specified by referent property attributes must be valid UTF-8 strings." ) } } else { @@ -358,6 +395,8 @@ fn defer_ref_properties(tree: &mut RojoTree, id: Ref, context: &mut PatchApplyCo } } if let Some(specified_id) = attr_id { + // Legacy Rojo_Id also needs cleanup + context.instances_needing_attr_cleanup.insert(id); tree.set_specified_id(id, specified_id); } } diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index cc3fbbbb..025ea9dc 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -8,7 +8,7 @@ use rbx_dom_weak::{ ustr, HashMapExt as _, UstrMap, UstrSet, }; -use crate::{RojoRef, REF_POINTER_ATTRIBUTE_PREFIX}; +use crate::{RojoRef, REF_PATH_ATTRIBUTE_PREFIX, REF_POINTER_ATTRIBUTE_PREFIX}; use super::{ patch::{PatchAdd, PatchSet, PatchUpdate}, @@ -286,6 +286,38 @@ fn compute_ref_properties( }; for (attr_name, attr_value) in attributes.iter() { + // Handle new path-based refs (Rojo_Ref_) + if let Some(prop_name) = attr_name.strip_prefix(REF_PATH_ATTRIBUTE_PREFIX) { + let path = match attr_value { + Variant::String(str) => str.as_str(), + Variant::BinaryString(bytes) => { + if let Ok(str) = std::str::from_utf8(bytes.as_ref()) { + str + } else { + log::warn!( + "Paths specified by referent property attributes must be valid UTF-8 strings" + ); + continue; + } + } + _ => { + log::warn!( + "Attribute {attr_name} is of type {:?} when it was expected to be a String", + attr_value.ty() + ); + continue; + } + }; + if let Some(target_id) = tree.get_instance_by_path(path) { + map.insert(ustr(prop_name), Some(Variant::Ref(target_id))); + } else { + // Path not found yet - will be resolved during patch apply + map.insert(ustr(prop_name), None); + } + continue; + } + + // Handle legacy ID-based refs (Rojo_Target_) let prop_name = match attr_name.strip_prefix(REF_POINTER_ATTRIBUTE_PREFIX) { Some(str) => str, None => continue, diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index f1184c93..9d251585 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -1,5 +1,6 @@ //! Implements iterating through an entire WeakDom and linking all Ref -//! properties using attributes. +//! properties using path-based attributes (preferred) or ID-based attributes +//! (fallback for non-unique paths). use std::collections::{HashMap, HashSet, VecDeque}; @@ -9,36 +10,31 @@ use rbx_dom_weak::{ }; use crate::{ - multimap::MultiMap, syncback::snapshot::inst_path, REF_EXTERNAL_ATTRIBUTE_PREFIX, - REF_ID_ATTRIBUTE_NAME, REF_POINTER_ATTRIBUTE_PREFIX, + syncback::snapshot::inst_path, REF_ID_ATTRIBUTE_NAME, REF_PATH_ATTRIBUTE_PREFIX, + REF_POINTER_ATTRIBUTE_PREFIX, }; pub struct RefLinks { - /// A map of referents to each of their Ref properties. - prop_links: MultiMap, - /// A map of referents to external ref properties (ones pointing outside the tree). - external_links: MultiMap, - /// A set of referents that need their ID rewritten. This includes - /// Instances that have no existing ID. - need_rewrite: HashSet, + /// Refs that use path-based linking (path is unique). + path_links: HashMap>, + /// Refs that use ID-based linking (path is NOT unique - fallback). + id_links: HashMap>, + /// Target instances that need a Rojo_Id written (for ID-based system). + targets_needing_id: HashSet, } #[derive(PartialEq, Eq)] -struct RefLink { - /// The name of a property +struct PathRefLink { name: Ustr, - /// The value of the property. - value: Ref, -} - -#[derive(PartialEq, Eq)] -struct ExternalRefLink { - /// The name of a property - name: Ustr, - /// The path to the external instance. path: String, } +#[derive(PartialEq, Eq)] +struct IdRefLink { + name: Ustr, + target: Ref, +} + /// Collects all instance paths in a WeakDom before any pruning occurs. /// Returns a map of Ref -> path for all instances. pub fn collect_all_paths(dom: &WeakDom) -> HashMap { @@ -55,140 +51,165 @@ pub fn collect_all_paths(dom: &WeakDom) -> HashMap { paths } +/// Checks if a path is unique in the DOM by verifying no duplicates exist +/// at ANY level of the path (target, parent, grandparent, etc.). +fn is_path_unique(dom: &WeakDom, target_ref: Ref) -> bool { + let mut current_ref = target_ref; + + // Walk up the tree checking each level for duplicate names among siblings + loop { + let current = match dom.get_by_ref(current_ref) { + Some(inst) => inst, + None => return false, + }; + + let parent_ref = current.parent(); + if parent_ref.is_none() { + // Reached root - path is unique at all levels + return true; + } + + let parent = match dom.get_by_ref(parent_ref) { + Some(inst) => inst, + None => return true, + }; + + // Check if any sibling has the same name as current + let current_name = ¤t.name; + let mut count = 0; + for child_ref in parent.children() { + if let Some(child) = dom.get_by_ref(*child_ref) { + if &child.name == current_name { + count += 1; + if count > 1 { + // Duplicate found at this level - path is not unique + return false; + } + } + } + } + + // Move up to parent and check the next level + current_ref = parent_ref; + } +} + /// Iterates through a WeakDom and collects referent properties. -/// -/// They can be linked to a dom later using `link_referents`. +/// Uses paths when unique, falls back to IDs when paths are ambiguous. /// /// The `pre_prune_paths` parameter should contain paths for instances that may -/// have been pruned from the DOM. This allows external references (references -/// to instances outside the sync tree) to be preserved as path-based attributes. +/// have been pruned from the DOM. This allows references to instances outside +/// the sync tree to be preserved as path-based attributes. pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) -> RefLinks { - let mut ids = HashMap::new(); - let mut need_rewrite = HashSet::new(); - let mut links = MultiMap::new(); - let mut external_links = MultiMap::new(); + let mut path_links: HashMap> = HashMap::new(); + let mut id_links: HashMap> = HashMap::new(); + let mut targets_needing_id: HashSet = HashSet::new(); - // Note that this is back-in, front-out. This is important because - // VecDeque::extend is the equivalent to using push_back. let mut queue = VecDeque::new(); queue.push_back(dom.root_ref()); + while let Some(inst_ref) = queue.pop_front() { let pointer = dom.get_by_ref(inst_ref).unwrap(); queue.extend(pointer.children().iter().copied()); for (prop_name, prop_value) in &pointer.properties { - let Variant::Ref(prop_value) = prop_value else { + let Variant::Ref(target_ref) = prop_value else { continue; }; - if prop_value.is_none() { + if target_ref.is_none() { continue; } - let target = match dom.get_by_ref(*prop_value) { - Some(inst) => inst, - None => { - // The target doesn't exist in the current DOM. Check if we - // have its path from before pruning. - if let Some(external_path) = pre_prune_paths.get(prop_value) { - log::debug!( - "Property {}.{} points to external instance at '{}', storing as path reference", - inst_path(dom, inst_ref), - prop_name, - external_path - ); - external_links.insert( - inst_ref, - ExternalRefLink { - name: *prop_name, - path: external_path.clone(), - }, - ); - } else { - // Properties that are Some but point to nothing may as - // well be `nil`. This is a truly dangling reference. - log::warn!( - "Property {}.{} will be `nil` on the disk because the referenced instance does not exist", - inst_path(dom, inst_ref), - prop_name - ); - } - continue; + // Check if target exists in current DOM + if let Some(_target) = dom.get_by_ref(*target_ref) { + // Target exists - check if path is unique + if is_path_unique(dom, *target_ref) { + // Path is unique - use path-based system + let target_path = inst_path(dom, *target_ref); + path_links.entry(inst_ref).or_default().push(PathRefLink { + name: *prop_name, + path: target_path, + }); + } else { + // Path is NOT unique - fall back to ID-based system + log::debug!( + "Property {}.{} uses ID-based reference because target has duplicate-named siblings", + inst_path(dom, inst_ref), + prop_name + ); + id_links.entry(inst_ref).or_default().push(IdRefLink { + name: *prop_name, + target: *target_ref, + }); + targets_needing_id.insert(*target_ref); } - }; - - links.insert( - inst_ref, - RefLink { + } else if let Some(external_path) = pre_prune_paths.get(target_ref) { + // Target was pruned - use path (we can't check uniqueness, assume it's fine) + log::debug!( + "Property {}.{} points to pruned instance at '{}', storing as path reference", + inst_path(dom, inst_ref), + prop_name, + external_path + ); + path_links.entry(inst_ref).or_default().push(PathRefLink { name: *prop_name, - value: *prop_value, - }, - ); - - // 1. Check if target has an ID - if let Some(id) = get_existing_id(target) { - // If it does, we need to check whether that ID is a duplicate - if let Some(id_ref) = ids.get(id) { - // If the same ID points to a new Instance, rewrite it. - if id_ref != prop_value { - if log::log_enabled!(log::Level::Trace) { - log::trace!( - "{} needs an id rewritten because it has the same id as {}", - target.name, - dom.get_by_ref(*id_ref).unwrap().name - ); - } - need_rewrite.insert(*prop_value); - } - } - ids.insert(id, *prop_value); + path: external_path.clone(), + }); } else { - log::trace!("{} needs an id rewritten because it has no id but is referred to by {}.{prop_name}", target.name, pointer.name); - // If it does not, it needs one. - need_rewrite.insert(*prop_value); + // Truly dangling reference + log::warn!( + "Property {}.{} will be `nil` on disk because the referenced instance does not exist", + inst_path(dom, inst_ref), + prop_name + ); } } } RefLinks { - need_rewrite, - prop_links: links, - external_links, + path_links, + id_links, + targets_needing_id, } } +/// Writes reference attributes to instances in the DOM. +/// Uses paths for unique references, IDs for non-unique (duplicate siblings). pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> { - write_id_attributes(&links, dom)?; - - // Convert MultiMaps to HashMaps for easier access - let prop_links: HashMap> = links.prop_links.into_iter().collect(); - let external_links: HashMap> = - links.external_links.into_iter().collect(); + // First, write Rojo_Id attributes to targets that need them (for ID-based refs) + write_id_attributes(&links.targets_needing_id, dom)?; // Collect all instance IDs that need attributes updated - let mut all_inst_ids: HashSet = prop_links.keys().copied().collect(); - all_inst_ids.extend(external_links.keys().copied()); - - let mut prop_list = Vec::new(); - let mut external_prop_list = Vec::new(); + let mut all_inst_ids: HashSet = links.path_links.keys().copied().collect(); + all_inst_ids.extend(links.id_links.keys().copied()); for inst_id in all_inst_ids { - // Collect internal ref links - if let Some(properties) = prop_links.get(&inst_id) { - for ref_link in properties { - let prop_inst = match dom.get_by_ref(ref_link.value) { - Some(inst) => inst, - None => continue, - }; - let id = get_existing_id(prop_inst) - .expect("all Instances that are pointed to should have an ID"); - prop_list.push((ref_link.name, Variant::String(id.to_owned()))); + let mut path_attrs: Vec<(String, Variant)> = Vec::new(); + let mut id_attrs: Vec<(String, Variant)> = Vec::new(); + + // Collect path-based refs + if let Some(path_refs) = links.path_links.get(&inst_id) { + for link in path_refs { + path_attrs.push(( + format!("{REF_PATH_ATTRIBUTE_PREFIX}{}", link.name), + Variant::String(link.path.clone()), + )); } } - // Collect external ref links - if let Some(external_properties) = external_links.get(&inst_id) { - for ext_link in external_properties { - external_prop_list.push((ext_link.name, Variant::String(ext_link.path.clone()))); + // Collect ID-based refs + if let Some(id_refs) = links.id_links.get(&inst_id) { + for link in id_refs { + let target = match dom.get_by_ref(link.target) { + Some(inst) => inst, + None => continue, + }; + let id = + get_existing_id(target).expect("all ID-based targets should have an ID by now"); + id_attrs.push(( + format!("{REF_POINTER_ATTRIBUTE_PREFIX}{}", link.name), + Variant::String(id.to_owned()), + )); } } @@ -209,25 +230,19 @@ pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> } .into_iter() .filter(|(name, _)| { - !name.starts_with(REF_POINTER_ATTRIBUTE_PREFIX) - && !name.starts_with(REF_EXTERNAL_ATTRIBUTE_PREFIX) + !name.starts_with(REF_PATH_ATTRIBUTE_PREFIX) + && !name.starts_with(REF_POINTER_ATTRIBUTE_PREFIX) }) .collect(); - // Add internal ref pointers - for (prop_name, prop_value) in prop_list.drain(..) { - attributes.insert( - format!("{REF_POINTER_ATTRIBUTE_PREFIX}{prop_name}"), - prop_value, - ); + // Add path-based refs + for (attr_name, attr_value) in path_attrs { + attributes.insert(attr_name, attr_value); } - // Add external ref pointers (path-based) - for (prop_name, prop_value) in external_prop_list.drain(..) { - attributes.insert( - format!("{REF_EXTERNAL_ATTRIBUTE_PREFIX}{prop_name}"), - prop_value, - ); + // Add ID-based refs + for (attr_name, attr_value) in id_attrs { + attributes.insert(attr_name, attr_value); } inst.properties @@ -237,12 +252,18 @@ pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> Ok(()) } -fn write_id_attributes(links: &RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> { - for referent in &links.need_rewrite { +fn write_id_attributes(targets: &HashSet, dom: &mut WeakDom) -> anyhow::Result<()> { + for referent in targets { let inst = match dom.get_by_ref_mut(*referent) { Some(inst) => inst, None => continue, }; + + // Skip if already has an ID + if get_existing_id_from_props(&inst.properties).is_some() { + continue; + } + let unique_id = match inst.properties.get(&ustr("UniqueId")) { Some(Variant::UniqueId(id)) => Some(*id), _ => None, @@ -286,3 +307,18 @@ fn get_existing_id(inst: &Instance) -> Option<&str> { None } } + +fn get_existing_id_from_props(props: &rbx_dom_weak::UstrMap) -> Option { + if let Variant::Attributes(attrs) = props.get(&ustr("Attributes"))? { + let id = attrs.get(REF_ID_ATTRIBUTE_NAME)?; + match id { + Variant::String(str) => Some(str.clone()), + Variant::BinaryString(bstr) => std::str::from_utf8(bstr.as_ref()) + .ok() + .map(|s| s.to_string()), + _ => None, + } + } else { + None + } +}