From a1e270949d63100e5449684d517fe4e6f13f9622 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 20 Jan 2026 21:05:28 -0600 Subject: [PATCH] Add support for external references in RojoTree and syncback process - Introduced REF_EXTERNAL_ATTRIBUTE_PREFIX for attributes pointing to external instances. - Enhanced PatchApplyContext to track external references and apply them during patch application. - Implemented get_instance_by_path method in RojoTree for resolving paths to external instances. - Updated collect_referents function to handle external references and preserve their paths during syncback. - Refactored link_referents to include external reference handling in the linking process. --- src/rojo_ref.rs | 3 + src/snapshot/patch_apply.rs | 61 +++++++++++++++- src/snapshot/tree.rs | 42 +++++++++++ src/syncback/mod.rs | 11 ++- src/syncback/ref_properties.rs | 127 +++++++++++++++++++++++++++------ 5 files changed, 217 insertions(+), 27 deletions(-) diff --git a/src/rojo_ref.rs b/src/rojo_ref.rs index fbbf997e..ade06f78 100644 --- a/src/rojo_ref.rs +++ b/src/rojo_ref.rs @@ -4,6 +4,9 @@ use serde::{Deserialize, Serialize}; pub const REF_ID_ATTRIBUTE_NAME: &str = "Rojo_Id"; 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_"; // 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 8427c1a1..29fbca66 100644 --- a/src/snapshot/patch_apply.rs +++ b/src/snapshot/patch_apply.rs @@ -14,7 +14,10 @@ use super::{ patch::{AppliedPatchSet, AppliedPatchUpdate, PatchSet, PatchUpdate}, InstanceSnapshot, RojoTree, }; -use crate::{multimap::MultiMap, RojoRef, REF_ID_ATTRIBUTE_NAME, REF_POINTER_ATTRIBUTE_PREFIX}; +use crate::{ + multimap::MultiMap, RojoRef, REF_EXTERNAL_ATTRIBUTE_PREFIX, REF_ID_ATTRIBUTE_NAME, + REF_POINTER_ATTRIBUTE_PREFIX, +}; /// Consumes the input `PatchSet`, applying all of its prescribed changes to the /// tree and returns an `AppliedPatchSet`, which can be used to keep another @@ -81,6 +84,10 @@ struct PatchApplyContext { /// properties. 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, + /// The current applied patch result, describing changes made to the tree. applied_patch_set: AppliedPatchSet, } @@ -129,6 +136,34 @@ 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 { + for (prop_name, path) in map { + if let Some(target) = tree.get_instance_by_path(&path) { + log::debug!( + "Resolved external reference {} -> {} (path: {})", + prop_name, + target, + path + ); + external_rewrites.push((prop_name, Variant::Ref(target))); + } else { + log::debug!( + "Could not resolve external reference {} at path '{}' - instance not found in tree", + prop_name, + path + ); + } + } + if !external_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(..)); + } + } + context.applied_patch_set } @@ -295,6 +330,30 @@ 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) { + if let Variant::String(path) = attr_value { + context + .external_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 + .insert(id, (ustr(prop_name), str.to_string())); + } else { + log::error!( + "External paths specified by referent property attributes must be valid UTF-8 strings." + ) + } + } else { + log::warn!( + "Attribute {attr_name} is of type {:?} when it was \ + expected to be a String", + attr_value.ty() + ) + } + } } if let Some(specified_id) = attr_id { tree.set_specified_id(id, specified_id); diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 9e891f2c..9abe6917 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -231,6 +231,48 @@ impl RojoTree { self.specified_id_to_refs.insert(specified, id); } + /// Looks up an instance by its path in the tree (e.g., "SoundService/MySoundGroup"). + /// The path is relative to the root of the tree, with segments separated by "/". + /// Returns the Ref if found, None otherwise. + pub fn get_instance_by_path(&self, path: &str) -> Option { + if path.is_empty() { + return Some(self.get_root_id()); + } + + let segments: Vec<&str> = path.split('/').collect(); + let mut current_ref = self.get_root_id(); + + // The root of the tree doesn't appear in the path, so we start with + // the root's children. + for (i, segment) in segments.iter().enumerate() { + let current = self.inner.get_by_ref(current_ref)?; + + // For the first segment, check direct children of root + // For subsequent segments, check children of current + let children = if i == 0 { + current.children() + } else { + current.children() + }; + + let mut found = false; + for &child_ref in children { + let child = self.inner.get_by_ref(child_ref)?; + if child.name == *segment { + current_ref = child_ref; + found = true; + break; + } + } + + if !found { + return None; + } + } + + Some(current_ref) + } + fn insert_metadata(&mut self, id: Ref, metadata: InstanceMetadata) { for path in &metadata.relevant_paths { self.path_to_ids.insert(path.clone(), id); diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index e5262f06..4cb1d68b 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -14,7 +14,7 @@ use rbx_dom_weak::{ }; use serde::{Deserialize, Serialize}; use std::{ - collections::{HashMap, HashSet, VecDeque}, + collections::{HashMap, VecDeque}, env, path::Path, sync::OnceLock, @@ -24,7 +24,7 @@ use crate::{ glob::Glob, snapshot::{InstanceWithMeta, RojoTree}, snapshot_middleware::Middleware, - syncback::ref_properties::{collect_referents, link_referents}, + syncback::ref_properties::{collect_all_paths, collect_referents, link_referents}, Project, }; @@ -65,6 +65,11 @@ pub fn syncback_loop( .map(|rules| rules.compile_tree_globs()) .transpose()?; + // Collect all instance paths BEFORE pruning so we can track external references + // (references to instances that will be pruned, like SoundGroups in SoundService). + log::debug!("Collecting instance paths before pruning..."); + let pre_prune_paths = collect_all_paths(&new_tree); + // TODO: Add a better way to tell if the root of a project is a directory let skip_pruning = if let Some(path) = &project.tree.path { let middleware = @@ -87,7 +92,7 @@ pub fn syncback_loop( } log::debug!("Collecting referents for new DOM..."); - let deferred_referents = collect_referents(&new_tree); + let deferred_referents = collect_referents(&new_tree, &pre_prune_paths); // Remove any properties that are manually blocked from syncback via the // project file. diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 054f61b8..f1184c93 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -9,13 +9,15 @@ use rbx_dom_weak::{ }; use crate::{ - multimap::MultiMap, syncback::snapshot::inst_path, REF_ID_ATTRIBUTE_NAME, - REF_POINTER_ATTRIBUTE_PREFIX, + multimap::MultiMap, syncback::snapshot::inst_path, REF_EXTERNAL_ATTRIBUTE_PREFIX, + REF_ID_ATTRIBUTE_NAME, 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, @@ -29,13 +31,42 @@ struct RefLink { value: Ref, } +#[derive(PartialEq, Eq)] +struct ExternalRefLink { + /// The name of a property + name: Ustr, + /// The path to the external instance. + path: String, +} + +/// 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 { + let mut paths = HashMap::new(); + let mut queue = VecDeque::new(); + queue.push_back(dom.root_ref()); + + while let Some(inst_ref) = queue.pop_front() { + let inst = dom.get_by_ref(inst_ref).unwrap(); + queue.extend(inst.children().iter().copied()); + paths.insert(inst_ref, inst_path(dom, inst_ref)); + } + + paths +} + /// Iterates through a WeakDom and collects referent properties. /// /// They can be linked to a dom later using `link_referents`. -pub fn collect_referents(dom: &WeakDom) -> RefLinks { +/// +/// 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. +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(); // Note that this is back-in, front-out. This is important because // VecDeque::extend is the equivalent to using push_back. @@ -56,15 +87,31 @@ pub fn collect_referents(dom: &WeakDom) -> RefLinks { let target = match dom.get_by_ref(*prop_value) { Some(inst) => inst, None => { - // Properties that are Some but point to nothing may as - // well be `nil`. Roblox and us never produce these values - // but syncback prunes trees without adjusting ref - // properties for performance reasons. - log::warn!( - "Property {}.{} will be `nil` on the disk because the actual value is not being included in syncback", - inst_path(dom, inst_ref), - prop_name - ); + // 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; } }; @@ -105,24 +152,46 @@ pub fn collect_referents(dom: &WeakDom) -> RefLinks { RefLinks { need_rewrite, prop_links: links, + external_links, } } pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> { write_id_attributes(&links, dom)?; - let mut prop_list = Vec::new(); + // 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(); - for (inst_id, properties) in links.prop_links { - 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()))); + // 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(); + + 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()))); + } } + + // 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()))); + } + } + let inst = match dom.get_by_ref_mut(inst_id) { Some(inst) => inst, None => continue, @@ -139,9 +208,13 @@ pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> } } .into_iter() - .filter(|(name, _)| !name.starts_with(REF_POINTER_ATTRIBUTE_PREFIX)) + .filter(|(name, _)| { + !name.starts_with(REF_POINTER_ATTRIBUTE_PREFIX) + && !name.starts_with(REF_EXTERNAL_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}"), @@ -149,6 +222,14 @@ pub fn link_referents(links: RefLinks, dom: &mut WeakDom) -> anyhow::Result<()> ); } + // 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, + ); + } + inst.properties .insert("Attributes".into(), attributes.into()); }