From e207a398b1b6c167360e9adfeb6443ca7330d608 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Feb 2026 05:31:36 -0600 Subject: [PATCH] Refactor Reconciler equality checks and enhance path handling - Introduced a new `trueEquals` function in `trueEquals.lua` for shared value equality checks, improving consistency between `diff.lua` and `matching.lua`. - Removed redundant equality logic from `diff.lua` and `matching.lua`, streamlining the codebase. - Updated path handling in various test snapshots to ensure correct reference values, enhancing the accuracy of reconciliation tests. - Implemented a post-processing step in `syncback` to fix `Rojo_Ref_*` attribute paths with correct dedup suffixes, improving path resolution. These changes enhance the clarity and efficiency of the Reconciler's equality checks and path handling, paving the way for improved project management and user experience. --- plugin/src/Reconciler/diff.lua | 100 +----- plugin/src/Reconciler/matching.lua | 83 +---- plugin/src/Reconciler/trueEquals.lua | 107 ++++++ ...f_properties-src__pointer.model.json5.snap | 2 +- ...erties_blank-src__pointer.model.json5.snap | 2 +- ...s_conflict-src__Pointer_2.model.json5.snap | 2 +- src/change_processor.rs | 18 +- src/snapshot/matching.rs | 339 +++++++++++++++++- src/syncback/fs_snapshot.rs | 50 +++ src/syncback/matching.rs | 227 +++++++++++- src/syncback/mod.rs | 30 +- src/syncback/ref_properties.rs | 40 ++- src/syncback/snapshot.rs | 35 +- src/web/api.rs | 15 +- 14 files changed, 832 insertions(+), 218 deletions(-) create mode 100644 plugin/src/Reconciler/trueEquals.lua diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index 348a15da..8e5d149d 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -12,108 +12,12 @@ local getProperty = require(script.Parent.getProperty) local Error = require(script.Parent.Error) local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) + local function isEmpty(table) return next(table) == nil end -local function fuzzyEq(a: number, b: number, epsilon: number): boolean - local diff = math.abs(a - b) - -- Use both absolute and relative epsilon for better precision handling - -- Absolute: for small numbers near zero - -- Relative: for larger numbers where absolute epsilon is too tight - local maxVal = math.max(math.abs(a), math.abs(b), 1) - return diff < epsilon or diff < maxVal * epsilon -end - -local function trueEquals(a, b): boolean - -- Exit early for simple equality values - if a == b then - return true - end - - -- Treat nil and { Ref = "000...0" } as equal - if - (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") - or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") - then - return true - end - - local typeA, typeB = typeof(a), typeof(b) - - -- For tables, try recursive deep equality - if typeA == "table" and typeB == "table" then - local checkedKeys = {} - for key, value in a do - checkedKeys[key] = true - if not trueEquals(value, b[key]) then - return false - end - end - for key, value in b do - if checkedKeys[key] then - continue - end - if not trueEquals(value, a[key]) then - return false - end - end - return true - - -- For NaN, check if both values are not equal to themselves - elseif a ~= a and b ~= b then - return true - - -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "number" and typeB == "number" then - return fuzzyEq(a, b, 0.0001) - - -- For EnumItem->number, compare the EnumItem's value - elseif typeA == "number" and typeB == "EnumItem" then - return a == b.Value - elseif typeA == "EnumItem" and typeB == "number" then - return a.Value == b - - -- For Color3s, compare to RGB ints to avoid floating point inequality - elseif typeA == "Color3" and typeB == "Color3" then - local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) - local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) - return aR == bR and aG == bG and aB == bB - - -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "CFrame" and typeB == "CFrame" then - local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector3" and typeB == "Vector3" then - local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector2" and typeB == "Vector2" then - local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - end - - return false -end - local function shouldDeleteUnknownInstances(virtualInstance) if virtualInstance.Metadata ~= nil then return not virtualInstance.Metadata.ignoreUnknownInstances diff --git a/plugin/src/Reconciler/matching.lua b/plugin/src/Reconciler/matching.lua index 0e19fb95..9eb7c0dd 100644 --- a/plugin/src/Reconciler/matching.lua +++ b/plugin/src/Reconciler/matching.lua @@ -16,89 +16,10 @@ local Packages = script.Parent.Parent.Parent.Packages local RbxDom = require(Packages.RbxDom) +local trueEquals = require(script.Parent.trueEquals) + local UNMATCHED_PENALTY = 10000 --- Value equality check. This MUST match the trueEquals function in diff.lua --- exactly. If matching says "equal" but diff says "different", we get phantom --- changes from cross-paired instances. -local function fuzzyEq(a: number, b: number, epsilon: number): boolean - local diff = math.abs(a - b) - local maxVal = math.max(math.abs(a), math.abs(b), 1) - return diff < epsilon or diff < maxVal * epsilon -end - -local function trueEquals(a, b): boolean - if a == b then - return true - end - - if - (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") - or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") - then - return true - end - - local typeA, typeB = typeof(a), typeof(b) - - if typeA == "table" and typeB == "table" then - local checkedKeys = {} - for key, value in a do - checkedKeys[key] = true - if not trueEquals(value, b[key]) then - return false - end - end - for key, value in b do - if checkedKeys[key] then - continue - end - if not trueEquals(value, a[key]) then - return false - end - end - return true - elseif a ~= a and b ~= b then - return true - elseif typeA == "number" and typeB == "number" then - return fuzzyEq(a, b, 0.0001) - elseif typeA == "number" and typeB == "EnumItem" then - return a == b.Value - elseif typeA == "EnumItem" and typeB == "number" then - return a.Value == b - elseif typeA == "Color3" and typeB == "Color3" then - local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) - local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) - return aR == bR and aG == bG and aB == bB - elseif typeA == "CFrame" and typeB == "CFrame" then - local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - elseif typeA == "Vector3" and typeB == "Vector3" then - local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - elseif typeA == "Vector2" and typeB == "Vector2" then - local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - end - - return false -end - local Matching = {} --[[ diff --git a/plugin/src/Reconciler/trueEquals.lua b/plugin/src/Reconciler/trueEquals.lua new file mode 100644 index 00000000..43ccaed2 --- /dev/null +++ b/plugin/src/Reconciler/trueEquals.lua @@ -0,0 +1,107 @@ +--[[ + Shared value equality check used by both the diff and matching modules. + + If matching says "equal" but diff says "different" (or vice versa), + we get phantom changes from cross-paired instances. Both modules MUST + use this same function. +]] + +local function fuzzyEq(a: number, b: number, epsilon: number): boolean + local diff = math.abs(a - b) + -- Use both absolute and relative epsilon for better precision handling + -- Absolute: for small numbers near zero + -- Relative: for larger numbers where absolute epsilon is too tight + local maxVal = math.max(math.abs(a), math.abs(b), 1) + return diff < epsilon or diff < maxVal * epsilon +end + +local function trueEquals(a, b): boolean + -- Exit early for simple equality values + if a == b then + return true + end + + -- Treat nil and { Ref = "000...0" } as equal + if + (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") + or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") + then + return true + end + + local typeA, typeB = typeof(a), typeof(b) + + -- For tables, try recursive deep equality + if typeA == "table" and typeB == "table" then + local checkedKeys = {} + for key, value in a do + checkedKeys[key] = true + if not trueEquals(value, b[key]) then + return false + end + end + for key, value in b do + if checkedKeys[key] then + continue + end + if not trueEquals(value, a[key]) then + return false + end + end + return true + + -- For NaN, check if both values are not equal to themselves + elseif a ~= a and b ~= b then + return true + + -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "number" and typeB == "number" then + return fuzzyEq(a, b, 0.0001) + + -- For EnumItem->number, compare the EnumItem's value + elseif typeA == "number" and typeB == "EnumItem" then + return a == b.Value + elseif typeA == "EnumItem" and typeB == "number" then + return a.Value == b + + -- For Color3s, compare to RGB ints to avoid floating point inequality + elseif typeA == "Color3" and typeB == "Color3" then + local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) + local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) + return aR == bR and aG == bG and aB == bB + + -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "CFrame" and typeB == "CFrame" then + local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector3" and typeB == "Vector3" then + local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector2" and typeB == "Vector2" then + local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + end + + return false +end + +return trueEquals diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties-src__pointer.model.json5.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties-src__pointer.model.json5.snap index c2bd235f..274dadb4 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties-src__pointer.model.json5.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties-src__pointer.model.json5.snap @@ -4,7 +4,7 @@ expression: src/pointer.model.json5 --- { attributes: { - Rojo_Ref_Value: "ReplicatedStorage/target" + Rojo_Ref_Value: "target.model.json5" }, className: "ObjectValue" } diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_blank-src__pointer.model.json5.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_blank-src__pointer.model.json5.snap index 0fe7fc11..322623e6 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_blank-src__pointer.model.json5.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_blank-src__pointer.model.json5.snap @@ -4,7 +4,7 @@ expression: src/pointer.model.json5 --- { attributes: { - Rojo_Ref_Value: "ReplicatedStorage/target.txt" + Rojo_Ref_Value: "target.txt" }, className: "ObjectValue" } diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_conflict-src__Pointer_2.model.json5.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_conflict-src__Pointer_2.model.json5.snap index 54b72c1c..178652af 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_conflict-src__Pointer_2.model.json5.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__ref_properties_conflict-src__Pointer_2.model.json5.snap @@ -4,7 +4,7 @@ expression: src/Pointer_2.model.json5 --- { attributes: { - Rojo_Ref_Value: "ReplicatedStorage/Target_2" + Rojo_Ref_Value: "Target_2.model.json5" }, className: "ObjectValue" } diff --git a/src/change_processor.rs b/src/change_processor.rs index 8c5cfc5a..cee02548 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1104,7 +1104,13 @@ impl JobThreadContext { None => (removed_stem.to_string(), None), }; - // Find siblings that share the same dedup base stem + // Find siblings that share the same dedup base stem AND extension. + // Matching by stem alone would incorrectly group across different + // middleware types (e.g., Foo.server.luau and Foo.luau have the + // same stem "Foo" but different dedup keys). + let removed_extension = + removed_fs_name.find('.').map(|i| &removed_fs_name[i + 1..]); + let Some(parent_inst) = tree.get_instance(parent_ref) else { continue; }; @@ -1117,6 +1123,13 @@ impl JobThreadContext { } let sibling_fs = tree.filesystem_name_for(sibling_ref); let sibling_stem = sibling_fs.split('.').next().unwrap_or(&sibling_fs); + let sibling_ext = sibling_fs.find('.').map(|i| &sibling_fs[i + 1..]); + + // Only group siblings with the same extension (same dedup key space) + if sibling_ext != removed_extension { + continue; + } + let sibling_base = match parse_dedup_suffix(sibling_stem) { Some((b, _)) => b, None => sibling_stem, @@ -1130,8 +1143,7 @@ impl JobThreadContext { continue; } - // Determine extension from the removed instance's FS name - let extension = removed_fs_name.find('.').map(|i| &removed_fs_name[i + 1..]); + let extension = removed_extension; // Get the parent directory path let parent_dir = { diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs index 9f4ec3c8..0a701c40 100644 --- a/src/snapshot/matching.rs +++ b/src/snapshot/matching.rs @@ -20,6 +20,11 @@ use super::{InstanceSnapshot, InstanceWithMeta, RojoTree}; const UNMATCHED_PENALTY: u32 = 10_000; +/// Maximum recursion depth for `compute_change_count`. Beyond this depth, +/// only flat property comparison is used (no subtree recursion). Prevents +/// O(n^k) explosion on deeply nested trees with many same-named instances. +const MAX_SCORING_DEPTH: u32 = 3; + /// Result of the forward sync matching algorithm. pub struct ForwardMatchResult { /// Pairs of (snapshot_child, tree_ref) that were matched. @@ -128,7 +133,7 @@ pub fn match_forward( continue; } - // Score all (A, B) pairs in child order + // Score all (A, B) pairs using recursive change-count scoring let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); let mut best_so_far = u32::MAX; for &si in &avail_snap { @@ -137,12 +142,7 @@ pub fn match_forward( None => continue, }; for &ti in &avail_tree { - let inst = match tree.get_instance(tree_children[ti]) { - Some(i) => i, - None => continue, - }; - let cost = count_own_diffs(snap, &inst); - // Children count diff included in count_own_diffs + let cost = compute_change_count(snap, tree_children[ti], tree, best_so_far, 0); pairs.push((cost, si, ti)); if cost < best_so_far { best_so_far = cost; @@ -197,6 +197,191 @@ fn build_result( } } +/// Lightweight matching result used during recursive scoring. +/// Returns index pairs (snap_index, tree_child_index) without consuming +/// the snapshots. +struct ScoringMatchResult { + matched: Vec<(usize, usize)>, + unmatched_snap: usize, + unmatched_tree: usize, +} + +/// Match children by reference for scoring purposes (non-consuming). +/// Groups by (Name, ClassName), instant-matches 1:1 groups, and scores +/// ambiguous groups using `compute_change_count` (mutually recursive). +fn match_children_for_scoring( + snap_children: &[InstanceSnapshot], + tree_children: &[Ref], + tree: &RojoTree, + depth: u32, +) -> ScoringMatchResult { + let mut snap_matched = vec![false; snap_children.len()]; + let mut tree_matched = vec![false; tree_children.len()]; + let mut matched = Vec::new(); + + if snap_children.is_empty() && tree_children.is_empty() { + return ScoringMatchResult { + matched, + unmatched_snap: 0, + unmatched_tree: 0, + }; + } + + // Group by (Name, ClassName) + let mut snap_by_key: HashMap<(String, String), Vec> = HashMap::new(); + for (i, snap) in snap_children.iter().enumerate() { + snap_by_key + .entry((snap.name.to_string(), snap.class_name.to_string())) + .or_default() + .push(i); + } + + let mut tree_by_key: HashMap<(String, String), Vec> = HashMap::new(); + for (i, &child_ref) in tree_children.iter().enumerate() { + if let Some(inst) = tree.get_instance(child_ref) { + tree_by_key + .entry((inst.name().to_string(), inst.class_name().to_string())) + .or_default() + .push(i); + } + } + + // 1:1 groups: instant match + for (key, snap_indices) in &snap_by_key { + if let Some(tree_indices) = tree_by_key.get(key) { + if snap_indices.len() == 1 && tree_indices.len() == 1 { + let si = snap_indices[0]; + let ti = tree_indices[0]; + matched.push((si, ti)); + snap_matched[si] = true; + tree_matched[ti] = true; + } + } + } + + // Ambiguous groups: score + greedy assign + for (key, snap_indices) in &snap_by_key { + let Some(tree_indices) = tree_by_key.get(key) else { + continue; + }; + + let avail_snap: Vec = snap_indices + .iter() + .filter(|&&si| !snap_matched[si]) + .copied() + .collect(); + let avail_tree: Vec = tree_indices + .iter() + .filter(|&&ti| !tree_matched[ti]) + .copied() + .collect(); + + if avail_snap.is_empty() || avail_tree.is_empty() { + continue; + } + if avail_snap.len() == 1 && avail_tree.len() == 1 { + let si = avail_snap[0]; + let ti = avail_tree[0]; + matched.push((si, ti)); + snap_matched[si] = true; + tree_matched[ti] = true; + continue; + } + + let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); + let mut best_so_far = u32::MAX; + for &si in &avail_snap { + for &ti in &avail_tree { + let cost = compute_change_count( + &snap_children[si], + tree_children[ti], + tree, + best_so_far, + depth, + ); + pairs.push((cost, si, ti)); + if cost < best_so_far { + best_so_far = cost; + } + } + } + + pairs.sort_by_key(|&(cost, _, _)| cost); + + for &(_, si, ti) in &pairs { + if snap_matched[si] || tree_matched[ti] { + continue; + } + matched.push((si, ti)); + snap_matched[si] = true; + tree_matched[ti] = true; + } + } + + let unmatched_snap = snap_matched.iter().filter(|&&m| !m).count(); + let unmatched_tree = tree_matched.iter().filter(|&&m| !m).count(); + + ScoringMatchResult { + matched, + unmatched_snap, + unmatched_tree, + } +} + +/// Compute total change count between a snapshot and a tree instance, +/// including recursive subtree scoring. Returns the number of reconciler +/// operations needed to turn the tree instance into the snapshot. +/// +/// Mutually recursive with `match_children_for_scoring`: this function +/// scores a single pair, while `match_children_for_scoring` groups and +/// assigns children pairs (calling this function for ambiguous scoring). +fn compute_change_count( + snap: &InstanceSnapshot, + tree_ref: Ref, + tree: &RojoTree, + best_so_far: u32, + depth: u32, +) -> u32 { + let inst = match tree.get_instance(tree_ref) { + Some(i) => i, + None => return UNMATCHED_PENALTY, + }; + + let mut cost = count_own_diffs(snap, &inst); + if cost >= best_so_far || depth >= MAX_SCORING_DEPTH { + return cost; + } + + let snap_children = &snap.children; + let tree_children = inst.children(); + + if snap_children.is_empty() && tree_children.is_empty() { + return cost; + } + + let scoring = match_children_for_scoring(snap_children, tree_children, tree, depth + 1); + + for &(si, ti) in &scoring.matched { + let remaining = best_so_far.saturating_sub(cost); + cost += compute_change_count( + &snap_children[si], + tree_children[ti], + tree, + remaining, + depth + 1, + ); + if cost >= best_so_far { + return cost; + } + } + + cost = cost.saturating_add( + (scoring.unmatched_snap + scoring.unmatched_tree) as u32 * UNMATCHED_PENALTY, + ); + + cost +} + /// Count own property diffs between a snapshot and a tree instance. /// Each differing property = +1. Tags and Attributes counted granularly. /// Children count diff = +1. @@ -435,4 +620,144 @@ mod tests { assert!(result.unmatched_snapshot.is_empty()); assert!(result.unmatched_tree.is_empty()); } + + fn make_snapshot_with_children( + name: &str, + class: &str, + children: Vec, + ) -> InstanceSnapshot { + InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: InstanceMetadata::default(), + name: Cow::Owned(name.to_string()), + class_name: ustr(class), + properties: Default::default(), + children, + } + } + + #[test] + fn recursive_matching_distinguishes_by_children() { + // Two Folders both named "Data" with identical top-level properties + // but different children. The recursive scoring should pair each + // Folder with the tree Folder that has matching children. + use rbx_dom_weak::types::Variant; + + let snap_a = make_snapshot_with_children( + "Data", + "Folder", + vec![make_snapshot_with_props( + "Child", + "Part", + vec![("Transparency", Variant::Float32(0.0))], + )], + ); + let snap_b = make_snapshot_with_children( + "Data", + "Folder", + vec![make_snapshot_with_props( + "Child", + "Part", + vec![("Transparency", Variant::Float32(1.0))], + )], + ); + let snaps = vec![snap_a, snap_b]; + + // Build tree with the two Folders in REVERSED order + let tree_child_a = make_snapshot_with_children( + "Data", + "Folder", + vec![make_snapshot_with_props( + "Child", + "Part", + vec![("Transparency", Variant::Float32(1.0))], + )], + ); + let tree_child_b = make_snapshot_with_children( + "Data", + "Folder", + vec![make_snapshot_with_props( + "Child", + "Part", + vec![("Transparency", Variant::Float32(0.0))], + )], + ); + let root_snap = InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: InstanceMetadata::default(), + name: Cow::Borrowed("DataModel"), + class_name: ustr("DataModel"), + properties: Default::default(), + children: vec![tree_child_a, tree_child_b], + }; + let tree = RojoTree::new(root_snap); + let root_id = tree.get_root_id(); + let tree_children: Vec = tree.get_instance(root_id).unwrap().children().to_vec(); + + let result = match_forward(snaps, &tree_children, &tree); + + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_snapshot.is_empty()); + assert!(result.unmatched_tree.is_empty()); + + // Verify recursive matching paired by children content, not position. + // snap[0] has child Transparency=0.0, should match tree[1] (also 0.0). + // snap[1] has child Transparency=1.0, should match tree[0] (also 1.0). + for (snap, tree_ref) in &result.matched { + let tree_inst = tree.get_instance(*tree_ref).unwrap(); + let tree_child_ref = tree_inst.children()[0]; + let tree_child = tree.get_instance(tree_child_ref).unwrap(); + + let snap_child = &snap.children[0]; + let snap_val = snap_child.properties.get(&ustr("Transparency")); + let tree_val = tree_child.properties().get(&ustr("Transparency")); + assert_eq!( + snap_val, tree_val, + "Recursive matching failed: snap child Transparency={:?}, tree child Transparency={:?}", + snap_val, tree_val + ); + } + } + + #[test] + fn depth_limit_completes_quickly() { + // Deeply nested tree (depth=6) with same-named instances at each level. + // Should complete without hanging thanks to the depth limit. + fn make_deep_snap(depth: u32) -> InstanceSnapshot { + if depth == 0 { + return make_snapshot("Leaf", "Part"); + } + make_snapshot_with_children( + "Node", + "Folder", + vec![make_deep_snap(depth - 1), make_deep_snap(depth - 1)], + ) + } + + let snaps = vec![make_deep_snap(6), make_deep_snap(6)]; + + let tree_children_snaps = vec![make_deep_snap(6), make_deep_snap(6)]; + let root_snap = InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: InstanceMetadata::default(), + name: Cow::Borrowed("DataModel"), + class_name: ustr("DataModel"), + properties: Default::default(), + children: tree_children_snaps, + }; + let tree = RojoTree::new(root_snap); + let root_id = tree.get_root_id(); + let tree_children: Vec = tree.get_instance(root_id).unwrap().children().to_vec(); + + let start = std::time::Instant::now(); + let result = match_forward(snaps, &tree_children, &tree); + let elapsed = start.elapsed(); + + assert_eq!(result.matched.len(), 2); + assert!( + elapsed.as_secs() < 5, + "Depth-limited matching took too long: {:?}", + elapsed + ); + } } diff --git a/src/syncback/fs_snapshot.rs b/src/syncback/fs_snapshot.rs index 89e62a06..286f4b8e 100644 --- a/src/syncback/fs_snapshot.rs +++ b/src/syncback/fs_snapshot.rs @@ -446,4 +446,54 @@ impl FsSnapshot { removed_dirs.sort_unstable(); removed_dirs } + + /// Post-process `Rojo_Ref_*` attribute paths in meta/model JSON files. + /// + /// For each entry in `substitutions` (old_path → new_path), finds all + /// `.meta.json5` and `.model.json5` files in the snapshot and replaces + /// occurrences of the old path with the new path. This fixes tentative + /// ref paths that didn't include dedup suffixes. + pub fn fix_ref_paths(&mut self, substitutions: &[(String, String)]) { + if substitutions.is_empty() { + return; + } + + let ref_paths: Vec = self + .added_files + .keys() + .filter(|p| { + let name = p.file_name().and_then(|f| f.to_str()).unwrap_or(""); + name.ends_with(".meta.json5") + || name.ends_with(".meta.json") + || name.ends_with(".model.json5") + || name.ends_with(".model.json") + }) + .cloned() + .collect(); + + for path in ref_paths { + let Some(contents) = self.added_files.get_mut(&path) else { + continue; + }; + let Ok(text) = std::str::from_utf8(contents) else { + continue; + }; + if !text.contains("Rojo_Ref_") { + continue; + } + + let mut modified = text.to_string(); + for (old_path, new_path) in substitutions { + if old_path == new_path { + continue; + } + // Replace exact path matches and prefix matches (path + "/") + modified = modified.replace(old_path.as_str(), new_path.as_str()); + } + + if modified != text { + *contents = modified.into_bytes(); + } + } + } } diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs index 5518879c..4aba519d 100644 --- a/src/syncback/matching.rs +++ b/src/syncback/matching.rs @@ -20,6 +20,10 @@ use crate::variant_eq::variant_eq; const UNMATCHED_PENALTY: u32 = 10_000; +/// Maximum recursion depth for `compute_change_count`. Beyond this depth, +/// only flat property comparison is used (no subtree recursion). +const MAX_SCORING_DEPTH: u32 = 3; + /// Result of the matching algorithm. #[derive(Debug)] pub struct MatchResult { @@ -140,14 +144,22 @@ pub fn match_children( continue; } - // Score all (A, B) pairs + // Score all (A, B) pairs using recursive change-count scoring let mut pairs: Vec<(u32, Ref, Ref)> = Vec::new(); let mut best_so_far = u32::MAX; for &new_ref in &avail_new { for &old_ref in &avail_old { - let cost = - count_own_diffs(new_ref, old_ref, new_dom, old_dom, new_hashes, old_hashes); + let cost = compute_change_count( + new_ref, + old_ref, + new_dom, + old_dom, + new_hashes, + old_hashes, + best_so_far, + 0, + ); pairs.push((cost, new_ref, old_ref)); if cost < best_so_far { best_so_far = cost; @@ -179,16 +191,156 @@ pub fn match_children( } } -/// Count own property diffs between two WeakDom instances. -/// Hash fast-path: if hashes match, return 0 (identical, no property check needed). -/// Each differing property = +1. Children count diff = +1. -fn count_own_diffs( +/// Lightweight matching result used during recursive scoring. +struct ScoringMatchResult { + matched: Vec<(usize, usize)>, + unmatched_new: usize, + unmatched_old: usize, +} + +/// Match children by reference for scoring purposes (non-consuming). +/// Groups by (Name, ClassName), instant-matches 1:1 groups, and scores +/// ambiguous groups using `compute_change_count` (mutually recursive). +fn match_children_for_scoring( + new_children: &[Ref], + old_children: &[Ref], + new_dom: &WeakDom, + old_dom: &WeakDom, + new_hashes: Option<&HashMap>, + old_hashes: Option<&HashMap>, + depth: u32, +) -> ScoringMatchResult { + let mut new_matched = vec![false; new_children.len()]; + let mut old_matched = vec![false; old_children.len()]; + let mut matched = Vec::new(); + + if new_children.is_empty() && old_children.is_empty() { + return ScoringMatchResult { + matched, + unmatched_new: 0, + unmatched_old: 0, + }; + } + + // Group by (Name, ClassName) + let mut new_by_key: HashMap<(String, String), Vec> = HashMap::new(); + for (i, &r) in new_children.iter().enumerate() { + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_key + .entry((inst.name.clone(), inst.class.to_string())) + .or_default() + .push(i); + } + } + + let mut old_by_key: HashMap<(String, String), Vec> = HashMap::new(); + for (i, &r) in old_children.iter().enumerate() { + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_key + .entry((inst.name.clone(), inst.class.to_string())) + .or_default() + .push(i); + } + } + + // 1:1 groups: instant match + for (key, new_indices) in &new_by_key { + if let Some(old_indices) = old_by_key.get(key) { + if new_indices.len() == 1 && old_indices.len() == 1 { + let ni = new_indices[0]; + let oi = old_indices[0]; + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; + } + } + } + + // Ambiguous groups: score + greedy assign + for (key, new_indices) in &new_by_key { + let Some(old_indices) = old_by_key.get(key) else { + continue; + }; + + let avail_new: Vec = new_indices + .iter() + .filter(|&&ni| !new_matched[ni]) + .copied() + .collect(); + let avail_old: Vec = old_indices + .iter() + .filter(|&&oi| !old_matched[oi]) + .copied() + .collect(); + + if avail_new.is_empty() || avail_old.is_empty() { + continue; + } + if avail_new.len() == 1 && avail_old.len() == 1 { + let ni = avail_new[0]; + let oi = avail_old[0]; + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; + continue; + } + + let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); + let mut best_so_far = u32::MAX; + for &ni in &avail_new { + for &oi in &avail_old { + let cost = compute_change_count( + new_children[ni], + old_children[oi], + new_dom, + old_dom, + new_hashes, + old_hashes, + best_so_far, + depth, + ); + pairs.push((cost, ni, oi)); + if cost < best_so_far { + best_so_far = cost; + } + } + } + + pairs.sort_by_key(|&(cost, _, _)| cost); + + for &(_, ni, oi) in &pairs { + if new_matched[ni] || old_matched[oi] { + continue; + } + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; + } + } + + let unmatched_new = new_matched.iter().filter(|&&m| !m).count(); + let unmatched_old = old_matched.iter().filter(|&&m| !m).count(); + + ScoringMatchResult { + matched, + unmatched_new, + unmatched_old, + } +} + +/// Compute total change count between two WeakDom instances, including +/// recursive subtree scoring. Hash fast-path: if hashes match, return 0. +/// +/// Mutually recursive with `match_children_for_scoring`. +fn compute_change_count( new_ref: Ref, old_ref: Ref, new_dom: &WeakDom, old_dom: &WeakDom, new_hashes: Option<&HashMap>, old_hashes: Option<&HashMap>, + best_so_far: u32, + depth: u32, ) -> u32 { // Hash fast-path: identical subtree = 0 cost if let (Some(nh), Some(oh)) = (new_hashes, old_hashes) { @@ -199,6 +351,64 @@ fn count_own_diffs( } } + let mut cost = count_own_diffs(new_ref, old_ref, new_dom, old_dom); + if cost >= best_so_far || depth >= MAX_SCORING_DEPTH { + return cost; + } + + let new_inst = match new_dom.get_by_ref(new_ref) { + Some(i) => i, + None => return UNMATCHED_PENALTY, + }; + let old_inst = match old_dom.get_by_ref(old_ref) { + Some(i) => i, + None => return UNMATCHED_PENALTY, + }; + + let new_children = new_inst.children(); + let old_children = old_inst.children(); + + if new_children.is_empty() && old_children.is_empty() { + return cost; + } + + let scoring = match_children_for_scoring( + new_children, + old_children, + new_dom, + old_dom, + new_hashes, + old_hashes, + depth + 1, + ); + + for &(ni, oi) in &scoring.matched { + let remaining = best_so_far.saturating_sub(cost); + cost += compute_change_count( + new_children[ni], + old_children[oi], + new_dom, + old_dom, + new_hashes, + old_hashes, + remaining, + depth + 1, + ); + if cost >= best_so_far { + return cost; + } + } + + cost = cost + .saturating_add((scoring.unmatched_new + scoring.unmatched_old) as u32 * UNMATCHED_PENALTY); + + cost +} + +/// Count own property diffs between two WeakDom instances (flat, non-recursive). +/// Each differing property = +1. Children count diff = +1. +/// Does NOT include hash fast-path (handled by `compute_change_count`). +fn count_own_diffs(new_ref: Ref, old_ref: Ref, new_dom: &WeakDom, old_dom: &WeakDom) -> u32 { let new_inst = match new_dom.get_by_ref(new_ref) { Some(i) => i, None => return UNMATCHED_PENALTY, @@ -210,7 +420,6 @@ fn count_own_diffs( let mut cost: u32 = 0; - // Properties present on new side for (key, new_val) in new_inst.properties.iter() { if let Some(old_val) = old_inst.properties.get(key) { if !variant_eq(new_val, old_val) { @@ -221,14 +430,12 @@ fn count_own_diffs( } } - // Properties present only on old side for key in old_inst.properties.keys() { if !new_inst.properties.contains_key(key) { cost += 1; } } - // Children count diff if new_inst.children().len() != old_inst.children().len() { cost += 1; } diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 4838f53a..6a22917e 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -174,7 +174,7 @@ pub fn syncback_loop_with_stats( } log::debug!("Collecting referents for new DOM..."); - let deferred_referents = collect_referents(&new_tree, &pre_prune_paths); + let deferred_referents = collect_referents(&new_tree, &pre_prune_paths, None); // Remove any properties that are manually blocked from syncback via the // project file. @@ -451,6 +451,7 @@ pub fn syncback_loop_with_stats( HashSet::new() }; + let ref_path_map = std::cell::RefCell::new(HashMap::new()); let syncback_data = SyncbackData { vfs, old_tree, @@ -458,6 +459,7 @@ pub fn syncback_loop_with_stats( project, incremental, stats, + ref_path_map: &ref_path_map, }; // Always start with old reference for the Project middleware. @@ -609,6 +611,32 @@ pub fn syncback_loop_with_stats( snapshots.extend(syncback.children); } + // Post-process: fix Rojo_Ref_* attribute paths with correct dedup suffixes. + // During the walk, we recorded (Ref → final_path) in the ref_path_map. + // Compare against tentative_fs_path to find instances where dedup changed the path. + { + use ref_properties::tentative_fs_path_public; + + let final_map = ref_path_map.borrow(); + let mut substitutions: Vec<(String, String)> = Vec::new(); + for (&inst_ref, final_path) in final_map.iter() { + let tentative = tentative_fs_path_public(&new_tree, inst_ref); + if tentative != *final_path { + substitutions.push((tentative, final_path.clone())); + } + } + if !substitutions.is_empty() { + // Sort by length descending so longer paths are replaced first + // (prevents partial prefix matches on shorter paths) + substitutions.sort_by(|a, b| b.0.len().cmp(&a.0.len())); + log::debug!( + "Fixing {} Rojo_Ref_* paths with dedup suffixes", + substitutions.len() + ); + fs_snapshot.fix_ref_paths(&substitutions); + } + } + // In clean mode, remove any existing paths that weren't overwritten by new structure. // This ensures no orphaned files remain from the old project layout. if !incremental && !existing_paths.is_empty() { diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 988d48d2..a9301c84 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -15,6 +15,12 @@ use crate::{ REF_ID_ATTRIBUTE_NAME, REF_PATH_ATTRIBUTE_PREFIX, REF_POINTER_ATTRIBUTE_PREFIX, }; +/// Public wrapper for `tentative_fs_path` used by the post-processing step +/// in `syncback_loop` to compare tentative vs final paths. +pub fn tentative_fs_path_public(dom: &WeakDom, target_ref: Ref) -> String { + tentative_fs_path(dom, target_ref) +} + /// Compute a filesystem-name-compatible path for an instance in a bare WeakDom. /// /// Each path segment is a **tentative filesystem name**: the slugified instance @@ -172,16 +178,28 @@ pub fn collect_all_paths(dom: &WeakDom) -> HashMap { /// The `pre_prune_paths` parameter should contain paths for instances that may /// 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 { +/// +/// The `final_paths` parameter, when provided, contains the definitive +/// filesystem-name-based paths assigned during the syncback walk (including +/// dedup suffixes like `~1`). These take priority over `tentative_fs_path()`. +pub fn collect_referents( + dom: &WeakDom, + pre_prune_paths: &HashMap, + final_paths: Option<&HashMap>, +) -> RefLinks { let mut path_links: HashMap> = HashMap::new(); let id_links: HashMap> = HashMap::new(); let targets_needing_id: HashSet = HashSet::new(); - // With the dedup system, ALL filesystem paths are unique (filesystem names - // are unique among siblings by definition). Always use path-based linking. - // ID-based fallback ($id/Rojo_Target_*) is only for instances without - // filesystem backing (inside .rbxm files), which will be handled when the - // syncback flow is restructured to pass filesystem name info. + // NOTE: tentative_fs_path() does not include dedup suffixes (~N). For + // instances with duplicate names under the same parent, this produces + // ambiguous paths. Resolution via get_instance_by_path() returns the + // first matching child, which may be wrong. + // + // When `final_paths` is provided (from the syncback walk), it is used + // instead of tentative_fs_path() to get the correct dedup'd path. + // Otherwise, falls back to tentative_fs_path() (pre-dedup, potentially + // ambiguous for duplicate-named instances). let mut queue = VecDeque::new(); queue.push_back(dom.root_ref()); @@ -199,10 +217,12 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) } if dom.get_by_ref(*target_ref).is_some() { - // Target exists in DOM -- always use path-based system. - // Use tentative filesystem names so the path is compatible - // with get_instance_by_path() resolution. - let target_path = tentative_fs_path(dom, *target_ref); + // Target exists in DOM -- use path-based system. + // Prefer final_paths (dedup-aware) when available, + // fall back to tentative_fs_path (pre-dedup). + let target_path = final_paths + .and_then(|fp| fp.get(target_ref).cloned()) + .unwrap_or_else(|| tentative_fs_path(dom, *target_ref)); path_links.entry(inst_ref).or_default().push(PathRefLink { name: *prop_name, path: target_path, diff --git a/src/syncback/snapshot.rs b/src/syncback/snapshot.rs index dc20cb3e..87318625 100644 --- a/src/syncback/snapshot.rs +++ b/src/syncback/snapshot.rs @@ -1,6 +1,7 @@ use indexmap::IndexMap; use memofs::Vfs; -use std::collections::HashSet; +use std::cell::RefCell; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use crate::{ @@ -29,6 +30,10 @@ pub struct SyncbackData<'sync> { pub(super) incremental: bool, /// Statistics tracker for syncback issues. pub(super) stats: &'sync SyncbackStats, + /// Records (Ref → ref-path-string) for each instance as filenames are + /// assigned during the syncback walk. Used after the walk to fix + /// Rojo_Ref_* attribute paths with correct dedup suffixes. + pub(super) ref_path_map: &'sync RefCell>, } impl<'sync> SyncbackData<'sync> { @@ -87,6 +92,10 @@ impl<'sync> SyncbackSnapshot<'sync> { snapshot.path = self.path.join(&*name); snapshot.needs_meta_name = needs_meta_name; + // Record the Ref → ref-path mapping for dedup-aware ref linking. + // The ref path is built from the parent's entry + the child filename. + self.record_ref_path(new_ref, &name); + Ok((snapshot, needs_meta_name, dedup_key)) } @@ -126,9 +135,33 @@ impl<'sync> SyncbackSnapshot<'sync> { snapshot.path = base_path.join(&*name); snapshot.needs_meta_name = needs_meta_name; + // Record the Ref → ref-path mapping for dedup-aware ref linking. + self.record_ref_path(new_ref, &name); + Ok((snapshot, needs_meta_name, dedup_key)) } + /// Records the ref path for a child instance in the shared ref_path_map. + /// The ref path is: self's ref path + "/" + child_filename. + fn record_ref_path(&self, child_ref: Ref, child_filename: &str) { + let parent_path = self + .data + .ref_path_map + .borrow() + .get(&self.new) + .cloned() + .unwrap_or_default(); + let child_path = if parent_path.is_empty() { + child_filename.to_string() + } else { + format!("{}/{}", parent_path, child_filename) + }; + self.data + .ref_path_map + .borrow_mut() + .insert(child_ref, child_path); + } + /// Constructs a SyncbackSnapshot with the provided path and refs while /// inheriting the data of the this snapshot. #[inline] diff --git a/src/web/api.rs b/src/web/api.rs index 0eabb23a..4ac313d4 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -631,11 +631,18 @@ impl ApiService { } else { return None; }; - let escaped_name = crate::escape_ref_path_segment(&added.name); - let path = if parent_path.is_empty() { - escaped_name.into_owned() + // Use slugified name (matching filesystem conventions) instead of + // escape_ref_path_segment (which uses legacy \/ escaping that + // get_instance_by_path cannot resolve). + let fs_name = if crate::syncback::name_needs_slugify(&added.name) { + crate::syncback::slugify_name(&added.name) } else { - format!("{}/{}", parent_path, escaped_name) + added.name.clone() + }; + let path = if parent_path.is_empty() { + fs_name + } else { + format!("{}/{}", parent_path, fs_name) }; Some((*guid, path)) })