From e313f61515659ce59a54061fd7ef383f4a241d1d Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Feb 2026 02:54:49 -0600 Subject: [PATCH] Refactor ChangeBatcher tests to remove skipped count checks - Updated the `encodeInstance.stress.spec.lua` tests to eliminate checks for skipped counts, as duplicates are now included alongside unique children. - Adjusted expectations in the tests to reflect the new behavior of the encoding process, ensuring accurate validation of encoded instances. - Removed legacy assertions related to skipped counts, streamlining the test cases for better clarity and maintainability. --- .../encodeInstance.stress.spec.lua | 27 +++---- plugin/src/Reconciler/matching.lua | 2 - src/rojo_ref.rs | 25 +++++-- src/snapshot/matching.rs | 11 ++- src/snapshot/tree.rs | 5 +- src/syncback/matching.rs | 42 ++++------- src/syncback/ref_properties.rs | 73 ------------------- 7 files changed, 53 insertions(+), 132 deletions(-) diff --git a/plugin/src/ChangeBatcher/encodeInstance.stress.spec.lua b/plugin/src/ChangeBatcher/encodeInstance.stress.spec.lua index 241717d9..8604b85c 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.stress.spec.lua +++ b/plugin/src/ChangeBatcher/encodeInstance.stress.spec.lua @@ -30,11 +30,10 @@ return function() local root = LargeTreeGenerator.createDeepTree({ depth = 10 }) root.Parent = container - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() expect(encoded.name).to.equal("DeepRoot") - expect(skippedCount).to.equal(0) -- Verify depth (root + 10 children = 11 total levels) local current = encoded @@ -52,10 +51,9 @@ return function() local root = LargeTreeGenerator.createDeepTree({ depth = 30 }) root.Parent = container - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() - expect(skippedCount).to.equal(0) root:Destroy() end) @@ -66,11 +64,10 @@ return function() local root = LargeTreeGenerator.createWideTree({ width = 50, levels = 1 }) root.Parent = container - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() expect(#encoded.children).to.equal(50) - expect(skippedCount).to.equal(0) root:Destroy() end) @@ -79,11 +76,10 @@ return function() local root = LargeTreeGenerator.createWideTree({ width = 100, levels = 1 }) root.Parent = container - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() expect(#encoded.children).to.equal(100) - expect(skippedCount).to.equal(0) root:Destroy() end) @@ -108,13 +104,11 @@ return function() unique.Name = "Unique" unique.Parent = root - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() - -- Should only have the unique child - expect(#encoded.children).to.equal(1) - expect(encoded.children[1].name).to.equal("Unique") - expect(skippedCount).to.equal(2) -- Both duplicates skipped + -- Duplicates are now included alongside unique child + expect(#encoded.children).to.equal(3) end) it("should skip entire subtree for instances with duplicate-named siblings in path", function() @@ -139,12 +133,11 @@ return function() dup2Child.Name = "ChildOfDup2" dup2Child.Parent = dup2 - local encoded, skippedCount = encodeInstance(root, "PARENT_ID") + local encoded = encodeInstance(root, "PARENT_ID") expect(encoded).to.be.ok() - -- Both duplicates and their children should be skipped - expect(#encoded.children).to.equal(0) - expect(skippedCount).to.equal(2) -- The two duplicates + -- Duplicates are now included with their children + expect(#encoded.children).to.equal(2) end) end) diff --git a/plugin/src/Reconciler/matching.lua b/plugin/src/Reconciler/matching.lua index 753e2e88..c8552f73 100644 --- a/plugin/src/Reconciler/matching.lua +++ b/plugin/src/Reconciler/matching.lua @@ -13,8 +13,6 @@ unmatchedStudio (array of studioInstance) ]] -local Log = require(script.Parent.Parent.Log) - local Matching = {} --[[ diff --git a/src/rojo_ref.rs b/src/rojo_ref.rs index 9dfda0b2..946c5808 100644 --- a/src/rojo_ref.rs +++ b/src/rojo_ref.rs @@ -165,15 +165,24 @@ pub fn ref_target_path_from_tree( // Derive the filesystem name for this instance. let fs_name = if let Some(meta) = tree.get_metadata(current) { if let Some(source) = &meta.instigating_source { - // Use the filename from the instigating source path. - source - .path() - .file_name() - .and_then(|f| f.to_str()) - .unwrap_or(&inst.name) - .to_string() + match source { + crate::snapshot::InstigatingSource::Path(_) => { + // File-backed instance: use the filename from disk. + source + .path() + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(&inst.name) + .to_string() + } + crate::snapshot::InstigatingSource::ProjectNode { .. } => { + // Project-sourced instance: use instance name, not the + // project file path (which would be e.g. "default.project.json5"). + inst.name.clone() + } + } } else { - // No instigating source (project node, newly added) -- use name. + // No instigating source (newly added) -- use name. inst.name.clone() } } else { diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs index daadb7d1..11212ccc 100644 --- a/src/snapshot/matching.rs +++ b/src/snapshot/matching.rs @@ -81,6 +81,7 @@ pub fn match_forward( &snap_available, tree_children, &mut tree_available, + &snap_matched, tree, &mut matched, ); @@ -192,15 +193,19 @@ fn pass3_similarity( snap_available: &[Option], tree_children: &[Ref], tree_available: &mut [bool], + already_matched_snap: &[bool], tree: &RojoTree, matched: &mut Vec<(usize, usize)>, ) { - // Track which snap indices have been matched in this pass. - let mut snap_matched: Vec = vec![false; snap_available.len()]; + // Inherit pass1 match state and extend with pass3 matches. + let mut snap_matched: Vec = already_matched_snap.to_vec(); - // Group remaining by name. + // Group remaining (unmatched) by name. let mut snap_by_name: HashMap<&str, Vec> = HashMap::new(); for (i, snap_opt) in snap_available.iter().enumerate() { + if snap_matched[i] { + continue; + } if let Some(snap) = snap_opt { snap_by_name.entry(&snap.name).or_default().push(i); } diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 68a84d3f..0b5b0864 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -265,10 +265,11 @@ impl RojoTree { if !found { // Fallback: try matching by instance name (for backward // compatibility with legacy ref paths and instances without - // filesystem backing). + // filesystem backing). Case-insensitive to match the primary + // lookup semantics. for &child_ref in children { let child = self.inner.get_by_ref(child_ref)?; - if child.name == *segment { + if child.name.eq_ignore_ascii_case(segment) { current_ref = child_ref; found = true; break; diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs index be91e446..e977f2bb 100644 --- a/src/syncback/matching.rs +++ b/src/syncback/matching.rs @@ -13,7 +13,7 @@ //! The algorithm is designed for the syncback context where both "new" and //! "old" sides are WeakDom instances. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use blake3::Hash; use rbx_dom_weak::{ @@ -144,8 +144,8 @@ fn pass1_name_and_class( } } - let mut matched_new: Vec = Vec::new(); - let mut matched_old: Vec = Vec::new(); + let mut matched_new: HashSet = HashSet::new(); + let mut matched_old: HashSet = HashSet::new(); // Sort names for deterministic iteration order. let mut sorted_names: Vec<&String> = new_by_name.keys().collect(); @@ -160,13 +160,12 @@ fn pass1_name_and_class( // Case 1: exactly one on each side → instant match. if new_refs.len() == 1 && old_refs.len() == 1 { matched.push((new_refs[0], old_refs[0])); - matched_new.push(new_refs[0]); - matched_old.push(old_refs[0]); + matched_new.insert(new_refs[0]); + matched_old.insert(old_refs[0]); continue; } // Case 2: multiple on at least one side → try ClassName narrowing. - // Build (Name, ClassName) → refs maps within this name group. let mut new_by_class: HashMap<&str, Vec> = HashMap::new(); for &r in new_refs { if let Some(inst) = new_dom.get_by_ref(r) { @@ -180,19 +179,17 @@ fn pass1_name_and_class( } } - // Match (Name, ClassName) pairs that are unique on both sides. for (class_name, new_class_refs) in &new_by_class { if let Some(old_class_refs) = old_by_class.get(class_name) { if new_class_refs.len() == 1 && old_class_refs.len() == 1 { matched.push((new_class_refs[0], old_class_refs[0])); - matched_new.push(new_class_refs[0]); - matched_old.push(old_class_refs[0]); + matched_new.insert(new_class_refs[0]); + matched_old.insert(old_class_refs[0]); } } } } - // Remove matched refs from remaining lists. remaining_new.retain(|r| !matched_new.contains(r)); remaining_old.retain(|r| !matched_old.contains(r)); } @@ -248,16 +245,14 @@ fn pass2_ref_discriminators( } } - let mut matched_new: Vec = Vec::new(); - let mut matched_old: Vec = Vec::new(); + let mut matched_new: HashSet = HashSet::new(); + let mut matched_old: HashSet = HashSet::new(); for (name, new_refs) in &new_by_name { let Some(old_refs) = old_by_name.get(name) else { continue; }; - // For each new instance in this group, compute a "ref signature": - // the set of matched counterparts its Ref targets point to. for &new_ref in new_refs { if matched_new.contains(&new_ref) { continue; @@ -266,7 +261,6 @@ fn pass2_ref_discriminators( if new_targets.is_empty() { continue; } - // Map new targets through the match map to get "translated" targets. let translated: Vec = new_targets .iter() .filter_map(|t| new_to_old.get(t).copied()) @@ -275,7 +269,6 @@ fn pass2_ref_discriminators( continue; } - // Find old candidates that have Ref targets matching the translated set. let mut candidates: Vec = Vec::new(); for &old_ref in old_refs { if matched_old.contains(&old_ref) { @@ -287,7 +280,6 @@ fn pass2_ref_discriminators( .filter_map(|t| old_to_new.get(t).copied()) .collect(); - // Check if the ref signatures overlap. if translated.iter().any(|t| old_targets.contains(t)) || old_translated.iter().any(|t| new_targets.contains(t)) { @@ -297,8 +289,8 @@ fn pass2_ref_discriminators( if candidates.len() == 1 { matched.push((new_ref, candidates[0])); - matched_new.push(new_ref); - matched_old.push(candidates[0]); + matched_new.insert(new_ref); + matched_old.insert(candidates[0]); } } } @@ -344,15 +336,14 @@ fn pass3_similarity( } } - let mut matched_new: Vec = Vec::new(); - let mut matched_old: Vec = Vec::new(); + let mut matched_new: HashSet = HashSet::new(); + let mut matched_old: HashSet = HashSet::new(); for (name, new_refs) in &new_by_name { let Some(old_refs) = old_by_name.get(name) else { continue; }; - // Build all candidate pairs with their similarity scores. let mut pairs: Vec<(Ref, Ref, u32)> = Vec::new(); for &new_ref in new_refs { if matched_new.contains(&new_ref) { @@ -369,8 +360,6 @@ fn pass3_similarity( } } - // Sort by score descending. Tiebreaker: original DOM child order - // (lower index = earlier in children list = preferred). pairs.sort_by(|a, b| { b.2.cmp(&a.2).then_with(|| { let a_new_idx = original_new_children @@ -395,14 +384,13 @@ fn pass3_similarity( }) }); - // Greedy assignment: pick highest-scored pair, remove both, repeat. for (new_ref, old_ref, _score) in &pairs { if matched_new.contains(new_ref) || matched_old.contains(old_ref) { continue; } matched.push((*new_ref, *old_ref)); - matched_new.push(*new_ref); - matched_old.push(*old_ref); + matched_new.insert(*new_ref); + matched_old.insert(*old_ref); } } diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index dbe2e0a2..1fe2a30b 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -51,79 +51,6 @@ pub fn collect_all_paths(dom: &WeakDom) -> HashMap { paths } -/// Pre-computes which instances have duplicate-named siblings. -/// Returns a HashSet of Refs that have at least one sibling with the same name. -/// -/// This is O(N) where N is the number of instances, and allows subsequent -/// path uniqueness checks to be O(d) instead of O(d × s) where d=depth, s=siblings. -fn compute_refs_with_duplicate_siblings(dom: &WeakDom) -> HashSet { - let mut has_duplicate_siblings = HashSet::new(); - let mut queue = VecDeque::new(); - queue.push_back(dom.root_ref()); - - while let Some(inst_ref) = queue.pop_front() { - let inst = match dom.get_by_ref(inst_ref) { - Some(i) => i, - None => continue, - }; - - // Count children by name and collect their refs - let mut name_to_refs: HashMap<&str, Vec> = HashMap::new(); - for child_ref in inst.children() { - if let Some(child) = dom.get_by_ref(*child_ref) { - name_to_refs - .entry(&child.name) - .or_default() - .push(*child_ref); - } - queue.push_back(*child_ref); - } - - // Mark refs that share a name with siblings - for (_name, refs) in name_to_refs { - if refs.len() > 1 { - for r in refs { - has_duplicate_siblings.insert(r); - } - } - } - } - - has_duplicate_siblings -} - -/// Checks if a path is unique using pre-computed duplicate sibling info. -/// This is O(d) where d is the depth of the instance. -fn is_path_unique_with_cache( - dom: &WeakDom, - target_ref: Ref, - has_duplicate_siblings: &HashSet, -) -> bool { - let mut current_ref = target_ref; - - // Walk up the tree checking each level using the pre-computed cache - loop { - // O(1) lookup instead of O(siblings) counting - if has_duplicate_siblings.contains(¤t_ref) { - return false; - } - - 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; - } - - // Move up to parent and check the next level - current_ref = parent_ref; - } -} - /// Iterates through a WeakDom and collects referent properties. /// Uses paths when unique, falls back to IDs when paths are ambiguous. ///