hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor ChangeBatcher tests to remove skipped count checks

Browse source 
- 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.
This commit is contained in:
Joe 2026-02-17 02:54:49 -06:00
parent d6162ac210
commit e313f61515
7 changed files with 53 additions and 132 deletions
Download patch file Download diff file Expand all files Collapse all files

27
plugin/src/ChangeBatcher/encodeInstance.stress.spec.lua
View file

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

2
plugin/src/Reconciler/matching.lua
View file

@ -13,8 +13,6 @@
unmatchedStudio (array of studioInstance)
]]
local Log = require(script.Parent.Parent.Log)
local Matching = {}
--[[

25
src/rojo_ref.rs
View file

@ -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 {

11
src/snapshot/matching.rs
View file

@ -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<InstanceSnapshot>],
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<bool> = vec![false; snap_available.len()];
// Inherit pass1 match state and extend with pass3 matches.
let mut snap_matched: Vec<bool> = already_matched_snap.to_vec();
// Group remaining by name.
// Group remaining (unmatched) by name.
let mut snap_by_name: HashMap<&str, Vec<usize>> = 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);
}

5
src/snapshot/tree.rs
View file

@ -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;

42
src/syncback/matching.rs
View file

@ -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<Ref> = Vec::new();
let mut matched_old: Vec<Ref> = Vec::new();
let mut matched_new: HashSet<Ref> = HashSet::new();
let mut matched_old: HashSet<Ref> = 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<Ref>> = 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<Ref> = Vec::new();
let mut matched_old: Vec<Ref> = Vec::new();
let mut matched_new: HashSet<Ref> = HashSet::new();
let mut matched_old: HashSet<Ref> = 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<Ref> = 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<Ref> = 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<Ref> = Vec::new();
let mut matched_old: Vec<Ref> = Vec::new();
let mut matched_new: HashSet<Ref> = HashSet::new();
let mut matched_old: HashSet<Ref> = 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);
}
}

73
src/syncback/ref_properties.rs
View file

@ -51,79 +51,6 @@ pub fn collect_all_paths(dom: &WeakDom) -> HashMap<Ref, String> {
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<Ref> {
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<Ref>> = 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<Ref>,
) -> 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(&current_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.
///