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)) })