diff --git a/CHANGELOG.md b/CHANGELOG.md index cb267117..c08650a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ Making a new release? Simply add the new header with the version and date undern * Added a "Forget" option to the sync reminder notification to avoid being reminded for that place in the future ([#1215]) * Improves relative path calculation for sourcemap generation to avoid issues with Windows UNC paths. ([#1217]) * Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265]) +* Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 @@ -50,6 +51,7 @@ Making a new release? Simply add the new header with the version and date undern [#1215]: https://github.com/rojo-rbx/rojo/pull/1215 [#1217]: https://github.com/rojo-rbx/rojo/pull/1217 [#1265]: https://github.com/rojo-rbx/rojo/pull/1265 +[#1266]: https://github.com/rojo-rbx/rojo/pull/1266 ## [7.7.0-rc.1] (November 27th, 2025) diff --git a/plugin/src/Reconciler/countMatchingProperties.lua b/plugin/src/Reconciler/countMatchingProperties.lua new file mode 100644 index 00000000..0ddfdcc3 --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.lua @@ -0,0 +1,44 @@ +--[[ + Counts how many of a virtual instance's properties match the live values on a + candidate Roblox instance. `hydrate` uses this to break ties when several + existing children share the same Name and ClassName. + + This mirrors the read -> decode -> compare flow that `diff` uses, reusing the + same `getProperty`, `decodeValue`, and `trueEquals` helpers. +]] + +local getProperty = require(script.Parent.getProperty) +local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) + +local function countMatchingProperties(instance, virtualInstance, instanceMap) + local score = 0 + + for propertyName, virtualValue in virtualInstance.Properties do + -- Skip refs. During hydration the instanceMap is still being built + -- top-down, so a ref may point at an instance we haven't hydrated yet + -- and therefore can't decode reliably. Refs are also a poor + -- disambiguator between same-named siblings. + if next(virtualValue) == "Ref" then + continue + end + + local getSuccess, existingValue = getProperty(instance, propertyName) + if not getSuccess then + continue + end + + local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) + if not decodeSuccess then + continue + end + + if trueEquals(existingValue, decodedValue) then + score += 1 + end + end + + return score +end + +return countMatchingProperties diff --git a/plugin/src/Reconciler/countMatchingProperties.spec.lua b/plugin/src/Reconciler/countMatchingProperties.spec.lua new file mode 100644 index 00000000..3d6bb18d --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.spec.lua @@ -0,0 +1,91 @@ +return function() + local countMatchingProperties = require(script.Parent.countMatchingProperties) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + + it("counts properties whose values match the instance", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "hello" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("does not count properties whose values differ", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "different" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("counts multiple matching properties independently", function() + local instance = Instance.new("Part") + instance.Anchored = true + instance.CanCollide = false + + local virtualInstance = { + ClassName = "Part", + Name = "Part", + Properties = { + Anchored = { Bool = true }, + CanCollide = { Bool = false }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(2) + + -- Flip one so only a single property matches. + instance.CanCollide = true + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("skips unknown properties without counting or erroring", function() + local instance = Instance.new("Folder") + + local virtualInstance = { + ClassName = "Folder", + Name = "Folder", + Properties = { + FAKE_PROPERTY = { String = "nope" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("skips Ref properties without counting or erroring", function() + local instance = Instance.new("ObjectValue") + + local virtualInstance = { + ClassName = "ObjectValue", + Name = "ObjectValue", + Properties = { + -- A ref must be skipped rather than decoded: during hydration + -- the target may not be in the map yet. + Value = { Ref = "00000000000000000000000000000000" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) +end diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index c8da0939..73d41236 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -10,104 +10,12 @@ local invariant = require(script.Parent.Parent.invariant) 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 - return math.abs(a - b) < 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/hydrate.lua b/plugin/src/Reconciler/hydrate.lua index d7798160..fdd6c4a2 100644 --- a/plugin/src/Reconciler/hydrate.lua +++ b/plugin/src/Reconciler/hydrate.lua @@ -3,9 +3,22 @@ concrete instances and assigning them IDs. ]] -local invariant = require(script.Parent.Parent.invariant) +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) -local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) +local invariant = require(script.Parent.Parent.invariant) +local countMatchingProperties = require(script.Parent.countMatchingProperties) + +-- When several existing children share a Name and ClassName we disambiguate +-- them by scoring how well each one's properties match the virtual instance. +-- That scoring is far more expensive than a Name/ClassName check, so we only do +-- it for reasonably-sized groups. Larger groups (e.g. a folder with thousands of +-- identically-named parts) fall back to the original order-based matching, which +-- bounds the added work to roughly MAX_CANDIDATES_TO_SCORE^2 property reads per +-- group regardless of how large the group is. +local MAX_CANDIDATES_TO_SCORE = 32 + +local function hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) local virtualInstance = virtualInstances[rootId] if virtualInstance == nil then @@ -13,38 +26,163 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) end instanceMap:insert(rootId, rootInstance) + stats.hydrated += 1 local existingChildren = rootInstance:GetChildren() - -- For each existing child, we'll track whether it's been paired with an - -- instance that the Rojo server knows about. - local isExistingChildVisited = {} - for i = 1, #existingChildren do - isExistingChildVisited[i] = false + -- Group existing children by Name then ClassName so each virtual child can + -- find its candidate matches without scanning every sibling. This is what + -- keeps hydration fast for parents with thousands of children. Nesting the + -- two tables (rather than a combined key) keeps the Name and ClassName checks + -- exact, with no way for one to bleed into the other. + local buckets = {} + for _, childInstance in existingChildren do + -- We guard accessing Name and ClassName in order to avoid tripping over + -- children of DataModel that Rojo won't have permissions to access at all. + local accessSuccess, name, className = pcall(function() + return childInstance.Name, childInstance.ClassName + end) + if not accessSuccess then + continue + end + + local bucketsByClassName = buckets[name] + if bucketsByClassName == nil then + bucketsByClassName = {} + buckets[name] = bucketsByClassName + end + + local bucket = bucketsByClassName[className] + if bucket == nil then + bucket = { cursor = 1, instances = {} } + bucketsByClassName[className] = bucket + end + + table.insert(bucket.instances, childInstance) end + -- Tracks which existing children have already been paired, so one instance + -- isn't matched to two different virtual instances. + local visited = {} + for _, childId in ipairs(virtualInstance.Children) do local virtualChild = virtualInstances[childId] - for childIndex, childInstance in existingChildren do - if not isExistingChildVisited[childIndex] then - -- We guard accessing Name and ClassName in order to avoid - -- tripping over children of DataModel that Rojo won't have - -- permissions to access at all. - local accessSuccess, name, className = pcall(function() - return childInstance.Name, childInstance.ClassName - end) + local bucketsByClassName = buckets[virtualChild.Name] + local bucket = bucketsByClassName and bucketsByClassName[virtualChild.ClassName] + if bucket == nil then + -- No existing instance matches; diff will mark this id for creation. + Log.trace( + "hydrate: no existing instance matches {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end - -- This rule is very conservative and could be loosened in the - -- future, or more heuristics could be introduced. - if accessSuccess and name == virtualChild.Name and className == virtualChild.ClassName then - isExistingChildVisited[childIndex] = true - hydrate(instanceMap, virtualInstances, childId, childInstance) - break + local instances = bucket.instances + + -- Advance past any leading children that have already been paired. The + -- cursor makes order-based matching amortized O(1) per child even for + -- very large groups, rather than rescanning the visited prefix. + while bucket.cursor <= #instances and visited[instances[bucket.cursor]] do + bucket.cursor += 1 + end + if bucket.cursor > #instances then + -- Every matching instance has already been paired with an earlier id. + Log.trace( + "hydrate: no unpaired instance left for {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end + + -- The cursor points at the earliest unvisited child, so the slots from + -- here to the end bound how many candidates remain. Visited children + -- after the cursor (gaps) only appear once a group is small enough to be + -- scored -- the order-based path below always takes the earliest, which + -- keeps the visited region a contiguous prefix. So whenever this count + -- exceeds the cap it is exact, and we can pick the earliest match without + -- collecting anything. + local remaining = #instances - bucket.cursor + 1 + + local match + if remaining > MAX_CANDIDATES_TO_SCORE then + -- Too many to score affordably; take the earliest in child order, + -- reproducing the original Name + ClassName behavior. + match = instances[bucket.cursor] + Log.trace( + "hydrate: {} candidates named {} ({}) exceeds the scoring cap of {}; matching id {} by child order", + remaining, + virtualChild.Name, + virtualChild.ClassName, + MAX_CANDIDATES_TO_SCORE, + childId + ) + else + -- Collect the (at most `remaining`) unvisited candidates. + local candidates = {} + for index = bucket.cursor, #instances do + local childInstance = instances[index] + if not visited[childInstance] then + table.insert(candidates, childInstance) end end + + if #candidates == 1 then + -- Only one candidate, so there's nothing to disambiguate. + match = candidates[1] + else + -- Break the tie by choosing the candidate whose properties best + -- match the virtual instance, falling back to the earliest in + -- child order when scores are equal. + local bestScore = -1 + for _, childInstance in candidates do + local score = countMatchingProperties(childInstance, virtualChild, instanceMap) + if score > bestScore then + bestScore = score + match = childInstance + end + end + + stats.ambiguousGroups += 1 + stats.candidatesScored += #candidates + Log.trace( + "hydrate: disambiguated {} candidates named {} ({}) for id {} by property match (best score {})", + #candidates, + virtualChild.Name, + virtualChild.ClassName, + childId, + bestScore + ) + end end + + visited[match] = true + hydrateInner(stats, instanceMap, virtualInstances, childId, match) end end +local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) + -- Tallies of the work hydration did, surfaced in a single debug log below so + -- the cost of property-based disambiguation is visible without per-node spam. + local stats = { + hydrated = 0, + ambiguousGroups = 0, + candidatesScored = 0, + } + + hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) + + Log.debug( + "Hydrated {} instances ({} ambiguous name+class groups, {} candidates scored)", + stats.hydrated, + stats.ambiguousGroups, + stats.candidatesScored + ) +end + return hydrate diff --git a/plugin/src/Reconciler/hydrate.spec.lua b/plugin/src/Reconciler/hydrate.spec.lua index 226d37f3..7f6fea7c 100644 --- a/plugin/src/Reconciler/hydrate.spec.lua +++ b/plugin/src/Reconciler/hydrate.spec.lua @@ -126,4 +126,140 @@ return function() expect(knownInstances.fromIds["CHILD1"]).to.equal(child1) expect(knownInstances.fromIds["CHILD2"]).to.equal(child2) end) + + it("should disambiguate duplicate-named siblings by matching properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "first" } }, + Children = {}, + }, + + CHILD_B = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "second" } }, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + -- Created in the reverse order of the virtual children, so a purely + -- order-based tiebreak would mis-pair them. + local child1 = Instance.new("StringValue") + child1.Name = "a" + child1.Value = "second" + child1.Parent = rootInstance + + local child2 = Instance.new("StringValue") + child2.Name = "a" + child2.Value = "first" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child2) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child1) + end) + + it("should fall back to child order for duplicate-named siblings with no distinguishing properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + + CHILD_B = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local child1 = Instance.new("Folder") + child1.Name = "a" + child1.Parent = rootInstance + + local child2 = Instance.new("Folder") + child2.Name = "a" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + -- With equal scores the earliest unvisited child wins, preserving the + -- original order-based behavior. + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child1) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child2) + end) + + it("should fall back to child order for very large duplicate-named groups", function() + -- More candidates than hydrate is willing to score at once. The group + -- must fall back to order-based matching, so virtual child N pairs with + -- existing child N regardless of properties. + local count = 64 + + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local expectedInstances = {} + for i = 1, count do + local id = "CHILD_" .. i + table.insert(virtualInstances.ROOT.Children, id) + virtualInstances[id] = { + ClassName = "StringValue", + Name = "a", + -- Distinct values that, if scored, would pair by value rather + -- than by order. + Properties = { Value = { String = "value " .. i } }, + Children = {}, + } + + local child = Instance.new("StringValue") + child.Name = "a" + child.Value = "value " .. (count - i + 1) + child.Parent = rootInstance + expectedInstances[id] = child + end + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(count + 1) + for id, expectedInstance in expectedInstances do + expect(knownInstances.fromIds[id]).to.equal(expectedInstance) + end + end) end diff --git a/plugin/src/Reconciler/trueEquals.lua b/plugin/src/Reconciler/trueEquals.lua new file mode 100644 index 00000000..7dadf476 --- /dev/null +++ b/plugin/src/Reconciler/trueEquals.lua @@ -0,0 +1,100 @@ +--[[ + Fuzzy value-equality used to compare a decoded virtual property value against + the live value read from a real instance. Shared by `diff` (to decide whether + a property changed) and `hydrate` (to score candidate instances). +]] + +local function fuzzyEq(a: number, b: number, epsilon: number): boolean + return math.abs(a - b) < 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