diff --git a/plugin/modules/testez b/plugin/modules/testez index 6e9157db..25d957d4 160000 --- a/plugin/modules/testez +++ b/plugin/modules/testez @@ -1 +1 @@ -Subproject commit 6e9157db3c1a8b1558c54c50065e3562cab362e1 +Subproject commit 25d957d4d5c4c02a52843ef43e72f21f973c2908 diff --git a/plugin/rbx_dom_lua/PropertyDescriptor.lua b/plugin/rbx_dom_lua/PropertyDescriptor.lua index dcb53632..3d4c2bd5 100644 --- a/plugin/rbx_dom_lua/PropertyDescriptor.lua +++ b/plugin/rbx_dom_lua/PropertyDescriptor.lua @@ -20,7 +20,21 @@ local function set(container, key, value) end function PropertyDescriptor.fromRaw(data, className, propertyName) + local key, value = next(data.DataType) + return setmetatable({ + -- The meanings of the key and value in DataType differ when the type of + -- the property is Enum. When the property is of type Enum, the key is + -- the name of the type: + -- + -- { Enum = "" } + -- + -- When the property is not of type Enum, the value is the name of the + -- type: + -- + -- { Value = "" } + dataType = key == "Enum" and key or value, + scriptability = data.Scriptability, className = className, name = propertyName, @@ -77,4 +91,4 @@ function PropertyDescriptor:write(instance, value) end end -return PropertyDescriptor \ No newline at end of file +return PropertyDescriptor diff --git a/plugin/src/ChangeBatcher/createPatchSet.lua b/plugin/src/ChangeBatcher/createPatchSet.lua new file mode 100644 index 00000000..e4c0fc0a --- /dev/null +++ b/plugin/src/ChangeBatcher/createPatchSet.lua @@ -0,0 +1,40 @@ +--[[ + Take an InstanceMap and a dictionary mapping instances to sets of property + names. Populate a patch with the encoded values of all the given properties + on all the given instances (or, if any changes set Parent to nil, removals + of instances) and return the patch. +]] + +local Log = require(script.Parent.Parent.Parent.Log) + +local PatchSet = require(script.Parent.Parent.PatchSet) + +local encodePatchUpdate = require(script.Parent.encodePatchUpdate) + +return function(instanceMap, propertyChanges) + local patch = PatchSet.newEmpty() + + for instance, properties in pairs(propertyChanges) do + local instanceId = instanceMap.fromInstances[instance] + + if instanceId == nil then + Log.warn("Ignoring change for instance {:?} as it is unknown to Rojo", instance) + continue + end + + if properties.Parent then + if instance.Parent == nil then + table.insert(patch.removed, instanceId) + else + Log.warn("Cannot sync non-nil Parent property changes yet") + end + else + local update = encodePatchUpdate(instance, instanceId, properties) + table.insert(patch.updated, update) + end + + propertyChanges[instance] = nil + end + + return patch +end diff --git a/plugin/src/ChangeBatcher/createPatchSet.spec.lua b/plugin/src/ChangeBatcher/createPatchSet.spec.lua new file mode 100644 index 00000000..55528df6 --- /dev/null +++ b/plugin/src/ChangeBatcher/createPatchSet.spec.lua @@ -0,0 +1,74 @@ +return function() + local PatchSet = require(script.Parent.Parent.PatchSet) + local InstanceMap = require(script.Parent.Parent.InstanceMap) + + local createPatchSet = require(script.Parent.createPatchSet) + + it("should return a patch", function() + local patch = createPatchSet(InstanceMap.new(), {}) + + assert(PatchSet.validate(patch)) + end) + + it("should contain updates for every instance with property changes", function() + local instanceMap = InstanceMap.new() + + local part1 = Instance.new("Part") + instanceMap:insert("PART_1", part1) + + local part2 = Instance.new("Part") + instanceMap:insert("PART_2", part2) + + local changes = { + [part1] = { + Position = true, + Size = true, + Color = true, + }, + [part2] = { + CFrame = true, + Velocity = true, + Transparency = true, + }, + } + + local patch = createPatchSet(instanceMap, changes) + + expect(#patch.updated).to.equal(2) + end) + + it("should not contain any updates for removed instances", function() + local instanceMap = InstanceMap.new() + + local part1 = Instance.new("Part") + instanceMap:insert("PART_1", part1) + + local changes = { + [part1] = { + Parent = true, + Position = true, + Size = true, + }, + } + + local patch = createPatchSet(instanceMap, changes) + + expect(#patch.removed).to.equal(1) + expect(#patch.updated).to.equal(0) + end) + + it("should remove instances from the property change table", function() + local instanceMap = InstanceMap.new() + + local part1 = Instance.new("Part") + instanceMap:insert("PART_1", part1) + + local changes = { + [part1] = {}, + } + + createPatchSet(instanceMap, changes) + + expect(next(changes)).to.equal(nil) + end) +end diff --git a/plugin/src/ChangeBatcher/encodePatchUpdate.lua b/plugin/src/ChangeBatcher/encodePatchUpdate.lua new file mode 100644 index 00000000..a3af8069 --- /dev/null +++ b/plugin/src/ChangeBatcher/encodePatchUpdate.lua @@ -0,0 +1,39 @@ +local Log = require(script.Parent.Parent.Parent.Log) +local RbxDom = require(script.Parent.Parent.Parent.RbxDom) + +local encodeProperty = require(script.Parent.encodeProperty) + +return function(instance, instanceId, properties) + local update = { + id = instanceId, + changedProperties = {}, + } + + for propertyName in pairs(properties) do + if propertyName == "Name" then + update.changedName = instance.Name + else + local descriptor = RbxDom.findCanonicalPropertyDescriptor(instance.ClassName, propertyName) + + if not descriptor then + Log.debug("Could not sync back property {:?}.{}", instance, propertyName) + continue + end + + local encodeSuccess, encodeResult = encodeProperty(instance, propertyName, descriptor) + + if not encodeSuccess then + Log.debug("Could not sync back property {:?}.{}: {}", instance, propertyName, encodeResult) + continue + end + + update.changedProperties[propertyName] = encodeResult + end + end + + if next(update.changedProperties) == nil and update.changedName == nil then + return nil + end + + return update +end diff --git a/plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua b/plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua new file mode 100644 index 00000000..9945ae34 --- /dev/null +++ b/plugin/src/ChangeBatcher/encodePatchUpdate.spec.lua @@ -0,0 +1,62 @@ +return function() + local encodePatchUpdate = require(script.Parent.encodePatchUpdate) + + it("should return an update when there are property changes", function() + local part = Instance.new("Part") + local properties = { + CFrame = true, + Color = true, + } + local update = encodePatchUpdate(part, "PART", properties) + + expect(update.id).to.equal("PART") + expect(update.changedProperties.CFrame).to.be.ok() + expect(update.changedProperties.Color).to.be.ok() + end) + + it("should return nil when there are no property changes", function() + local part = Instance.new("Part") + local properties = { + NonExistentProperty = true, + } + local update = encodePatchUpdate(part, "PART", properties) + + expect(update).to.equal(nil) + end) + + it("should set changedName in the update when the instance's Name changes", function() + local part = Instance.new("Part") + local properties = { + Name = true, + } + + part.Name = "We'reGettingToTheCoolPart" + + local update = encodePatchUpdate(part, "PART", properties) + + expect(update.changedName).to.equal("We'reGettingToTheCoolPart") + end) + + it("should correctly encode property values", function() + local part = Instance.new("Part") + local properties = { + Position = true, + Color = true, + } + + part.Position = Vector3.new(0, 100, 0) + part.Color = Color3.new(0.8, 0.2, 0.9) + + local update = encodePatchUpdate(part, "PART", properties) + local position = update.changedProperties.Position + local color = update.changedProperties.Color + + expect(position.Vector3[1]).to.equal(0) + expect(position.Vector3[2]).to.equal(100) + expect(position.Vector3[3]).to.equal(0) + + expect(color.Color3[1]).to.be.near(0.8, 0.01) + expect(color.Color3[2]).to.be.near(0.2, 0.01) + expect(color.Color3[3]).to.be.near(0.9, 0.01) + end) +end diff --git a/plugin/src/ChangeBatcher/encodeProperty.lua b/plugin/src/ChangeBatcher/encodeProperty.lua new file mode 100644 index 00000000..e6ad95a2 --- /dev/null +++ b/plugin/src/ChangeBatcher/encodeProperty.lua @@ -0,0 +1,21 @@ +local Log = require(script.Parent.Parent.Parent.Log) +local RbxDom = require(script.Parent.Parent.Parent.RbxDom) + +return function(instance, propertyName, propertyDescriptor) + local readSuccess, readResult = propertyDescriptor:read(instance) + + if not readSuccess then + Log.warn("Could not sync back property {:?}.{}: {}", instance, propertyName, readResult) + return false, nil + end + + local dataType = propertyDescriptor.dataType + local encodeSuccess, encodeResult = RbxDom.EncodedValue.encode(readResult, dataType) + + if not encodeSuccess then + Log.warn("Could not sync back property {:?}.{}: {}", instance, propertyName, encodeResult) + return false, nil + end + + return true, encodeResult +end diff --git a/plugin/src/ChangeBatcher/init.lua b/plugin/src/ChangeBatcher/init.lua new file mode 100644 index 00000000..5d320ce2 --- /dev/null +++ b/plugin/src/ChangeBatcher/init.lua @@ -0,0 +1,81 @@ +--[[ + The ChangeBatcher is responsible for collecting and dispatching changes made + to tracked instances during two-way sync. +]] + +local RunService = game:GetService("RunService") + +local PatchSet = require(script.Parent.PatchSet) + +local createPatchSet = require(script.createPatchSet) + +local ChangeBatcher = {} +ChangeBatcher.__index = ChangeBatcher + +local BATCH_INTERVAL = 0.2 + +function ChangeBatcher.new(instanceMap, onChangesFlushed) + local self + + local renderSteppedConnection = RunService.RenderStepped:Connect(function(dt) + self:__cycle(dt) + end) + + self = setmetatable({ + __accumulator = 0, + __renderSteppedConnection = renderSteppedConnection, + __instanceMap = instanceMap, + __onChangesFlushed = onChangesFlushed, + __pendingPropertyChanges = {}, + }, ChangeBatcher) + + return self +end + +function ChangeBatcher:stop() + self.__renderSteppedConnection:Disconnect() + self.__pendingPropertyChanges = {} +end + +function ChangeBatcher:add(instance, propertyName) + local properties = self.__pendingPropertyChanges[instance] + + if not properties then + properties = {} + self.__pendingPropertyChanges[instance] = properties + end + + properties[propertyName] = true +end + +function ChangeBatcher:__cycle(dt) + self.__accumulator += dt + + if self.__accumulator >= BATCH_INTERVAL then + self.__accumulator -= BATCH_INTERVAL + + local patch = self:__flush() + + if patch then + self.__onChangesFlushed(patch) + end + end + + self.__instanceMap:unpauseAllInstances() +end + +function ChangeBatcher:__flush() + if next(self.__pendingPropertyChanges) == nil then + return nil + end + + local patch = createPatchSet(self.__instanceMap, self.__pendingPropertyChanges) + + if PatchSet.isEmpty(patch) then + return nil + end + + return patch +end + +return ChangeBatcher diff --git a/plugin/src/ChangeBatcher/init.spec.lua b/plugin/src/ChangeBatcher/init.spec.lua new file mode 100644 index 00000000..21173233 --- /dev/null +++ b/plugin/src/ChangeBatcher/init.spec.lua @@ -0,0 +1,101 @@ +return function() + local ChangeBatcher = require(script.Parent) + local InstanceMap = require(script.Parent.Parent.InstanceMap) + local PatchSet = require(script.Parent.Parent.PatchSet) + + local noop = function() end + + describe("new", function() + it("should create a new ChangeBatcher", function() + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, noop) + + expect(changeBatcher.__pendingPropertyChanges).to.be.a("table") + expect(next(changeBatcher.__pendingPropertyChanges)).to.equal(nil) + expect(changeBatcher.__onChangesFlushed).to.equal(noop) + expect(changeBatcher.__instanceMap).to.equal(instanceMap) + expect(typeof(changeBatcher.__renderSteppedConnection)).to.equal("RBXScriptConnection") + end) + end) + + describe("stop", function() + it("should disconnect the RenderStepped connection", function() + local changeBatcher = ChangeBatcher.new(InstanceMap.new(), noop) + + changeBatcher:stop() + + expect(changeBatcher.__renderSteppedConnection.Connected).to.equal(false) + end) + end) + + describe("add", function() + it("should add property changes to be considered for the current batch", function() + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, noop) + local part = Instance.new("Part") + + instanceMap:insert("PART", part) + changeBatcher:add(part, "Name") + + local properties = changeBatcher.__pendingPropertyChanges[part] + + expect(properties).to.be.a("table") + expect(properties.Name).to.be.ok() + + changeBatcher:add(part, "Position") + expect(properties.Position).to.be.ok() + end) + end) + + describe("__cycle", function() + it("should immediately unpause any paused instances after each cycle", function() + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, noop) + local part = Instance.new("Part") + + instanceMap.pausedUpdateInstances[part] = true + + changeBatcher:__cycle(0) + + expect(instanceMap.pausedUpdateInstances[part]).to.equal(nil) + end) + end) + + describe("__flush", function() + it("should return nil when there are no changes to process", function() + local changeBatcher = ChangeBatcher.new(InstanceMap.new(), noop) + expect(changeBatcher:__flush()).to.equal(nil) + end) + + it("should return a patch when there are changes to process and the resulting patch is non-empty", function() + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, noop) + local part = Instance.new("Part") + + instanceMap:insert("PART", part) + + changeBatcher.__pendingPropertyChanges[part] = { + Position = true, + Name = true, + } + + local patch = changeBatcher:__flush() + + assert(PatchSet.validate(patch)) + end) + + it("should return nil when there are changes to process and the resulting patch is empty", function() + local instanceMap = InstanceMap.new() + local changeBatcher = ChangeBatcher.new(instanceMap, noop) + local part = Instance.new("Part") + + instanceMap:insert("PART", part) + + changeBatcher.__pendingPropertyChanges[part] = { + NonExistentProperty = true, + } + + expect(changeBatcher:__flush()).to.equal(nil) + end) + end) +end diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index 7196d915..2d9cbffb 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -1,3 +1,5 @@ +local RunService = game:GetService("RunService") + local Log = require(script.Parent.Parent.Log) --[[ @@ -135,29 +137,31 @@ function InstanceMap:destroyId(id) end --[[ - Pause updates for an instance momentarily and invoke a callback. - - If the callback throws an error, InstanceMap will still be kept in a - consistent state. + Pause updates for an instance. ]] -function InstanceMap:pauseInstance(instance, callback) +function InstanceMap:pauseInstance(instance) local id = self.fromInstances[instance] - -- If we don't know about this instance, ignore it and do not invoke the - -- callback. + -- If we don't know about this instance, ignore it. if id == nil then return end self.pausedUpdateInstances[instance] = true - local success, result = xpcall(callback, debug.traceback) - self.pausedUpdateInstances[instance] = false +end - if success then - return result - else - error(result, 2) - end +--[[ + Unpause updates for an instance. +]] +function InstanceMap:unpauseInstance(instance) + self.pausedUpdateInstances[instance] = nil +end + +--[[ + Unpause updates for all instances. +]] +function InstanceMap:unpauseAllInstances() + table.clear(self.pausedUpdateInstances) end function InstanceMap:__connectSignals(instance) @@ -200,6 +204,12 @@ function InstanceMap:__maybeFireInstanceChanged(instance, propertyName) return end + if RunService:IsRunning() then + -- We probably don't want to pick up property changes to save to the + -- filesystem in a running game. + return + end + self.onInstanceChanged(instance, propertyName) end @@ -222,4 +232,4 @@ function InstanceMap:__disconnectSignals(instance) end end -return InstanceMap \ No newline at end of file +return InstanceMap diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index a061bea7..345a3bf7 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -77,6 +77,10 @@ local function applyPatch(instanceMap, patch) continue end + -- Pause updates on this instance to avoid picking up our changes when + -- two-way sync is enabled. + instanceMap:pauseInstance(instance) + -- Track any part of this update that could not be applied. local unappliedUpdate = { id = update.id, @@ -197,4 +201,4 @@ local function applyPatch(instanceMap, patch) return unappliedPatch end -return applyPatch \ No newline at end of file +return applyPatch diff --git a/plugin/src/Reconciler/reify.spec.lua b/plugin/src/Reconciler/reify.spec.lua index 607811d6..a13b204a 100644 --- a/plugin/src/Reconciler/reify.spec.lua +++ b/plugin/src/Reconciler/reify.spec.lua @@ -255,7 +255,7 @@ return function() Name = "Child A", Properties = { Value = { - Ref = "Child B", + Ref = "CHILD_B", }, }, Children = {}, @@ -287,7 +287,7 @@ return function() -- constructed as part of a recursive call before the parent has totally -- finished. Given deferred refs, this should not fail, but it is a good -- case to test. - it("should apply properties containing refs to later siblings correctly", function() + it("should apply properties containing refs to later children correctly", function() local virtualInstances = { ROOT = { ClassName = "ObjectValue", @@ -344,4 +344,4 @@ return function() expect(update.id).to.equal("ROOT") expect(update.changedProperties.Value).to.equal(virtualInstances["ROOT"].Properties.Value) end) -end \ No newline at end of file +end diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index b40d8e81..d0bff1f3 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -5,6 +5,7 @@ local Log = require(script.Parent.Parent.Log) local Fmt = require(script.Parent.Parent.Fmt) local t = require(script.Parent.Parent.t) +local ChangeBatcher = require(script.Parent.ChangeBatcher) local InstanceMap = require(script.Parent.InstanceMap) local PatchSet = require(script.Parent.PatchSet) local Reconciler = require(script.Parent.Reconciler) @@ -56,10 +57,19 @@ function ServeSession.new(options) -- Declare self ahead of time to capture it in a closure local self local function onInstanceChanged(instance, propertyName) - self:__onInstanceChanged(instance, propertyName) + if not self.__twoWaySync then + return + end + + self.__changeBatcher:add(instance, propertyName) + end + + local function onChangesFlushed(patch) + self.__apiContext:write(patch) end local instanceMap = InstanceMap.new(onInstanceChanged) + local changeBatcher = ChangeBatcher.new(instanceMap, onChangesFlushed) local reconciler = Reconciler.new(instanceMap) local connections = {} @@ -82,6 +92,7 @@ function ServeSession.new(options) __twoWaySync = options.twoWaySync, __reconciler = reconciler, __instanceMap = instanceMap, + __changeBatcher = changeBatcher, __statusChangedCallback = nil, __connections = connections, } @@ -179,55 +190,6 @@ function ServeSession:__onActiveScriptChanged(activeScript) self.__apiContext:open(scriptId) end -function ServeSession:__onInstanceChanged(instance, propertyName) - if not self.__twoWaySync then - return - end - - local instanceId = self.__instanceMap.fromInstances[instance] - - if instanceId == nil then - Log.warn("Ignoring change for instance {:?} as it is unknown to Rojo", instance) - return - end - - local remove = nil - - local update = { - id = instanceId, - changedProperties = {}, - } - - if propertyName == "Name" then - update.changedName = instance.Name - elseif propertyName == "Parent" then - if instance.Parent == nil then - update = nil - remove = instanceId - else - Log.warn("Cannot sync non-nil Parent property changes yet") - return - end - else - local success, encoded = self.__reconciler:encodeApiValue(instance[propertyName]) - - if not success then - Log.warn("Could not sync back property {:?}.{}", instance, propertyName) - return - end - - update.changedProperties[propertyName] = encoded - end - - local patch = { - removed = {remove}, - added = {}, - updated = {update}, - } - - self.__apiContext:write(patch) -end - function ServeSession:__initialSync(rootInstanceId) return self.__apiContext:read({ rootInstanceId }) :andThen(function(readResponseBody) @@ -290,6 +252,7 @@ function ServeSession:__stopInternal(err) self:__setStatus(Status.Disconnected, err) self.__apiContext:disconnect() self.__instanceMap:stop() + self.__changeBatcher:stop() for _, connection in ipairs(self.__connections) do connection:Disconnect() @@ -305,4 +268,4 @@ function ServeSession:__setStatus(status, detail) end end -return ServeSession \ No newline at end of file +return ServeSession diff --git a/plugin/test-place.project.json b/plugin/test-place.project.json index 7bc42eb6..b75fcaae 100644 --- a/plugin/test-place.project.json +++ b/plugin/test-place.project.json @@ -9,7 +9,7 @@ }, "TestEZ": { - "$path": "modules/testez/lib" + "$path": "modules/testez" } }, @@ -25,4 +25,4 @@ } } } -} \ No newline at end of file +}