From 506a60d0be2f14bb216975ab4a446162fce630d2 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Tue, 30 Jan 2024 12:51:45 -0800 Subject: [PATCH 1/3] Play Solo & Test Server auto connect (#840) When enabled, the `baseurl` of the session is written to `workspace:SetAttribute("__Rojo_ConnectionUrl")` so that the test server can connect to that session automatically. This works for Play Solo and Local Test Server. It is marked experimental for now (and disabled by default) since connecting during a playtest session is... not polished. Rojo may overwrite things and cause headaches. Further work can be done later. --- CHANGELOG.md | 2 + plugin/src/App/StatusPages/Settings/init.lua | 35 ++++++--- plugin/src/App/init.lua | 76 ++++++++++++++++++-- plugin/src/Settings.lua | 1 + 4 files changed, 99 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70a7d5a81..efc9405ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased Changes * Added popout diff visualizer for table properties like Attributes and Tags ([#834]) * Updated Theme to use Studio colors ([#838]) +* Added experimental setting for Auto Connect in playtests ([#840]) * Projects may now specify rules for syncing files as if they had a different file extension. ([#813]) This is specified via a new field on project files, `syncRules`: @@ -53,6 +54,7 @@ [#813]: https://github.com/rojo-rbx/rojo/pull/813 [#834]: https://github.com/rojo-rbx/rojo/pull/834 [#838]: https://github.com/rojo-rbx/rojo/pull/838 +[#840]: https://github.com/rojo-rbx/rojo/pull/840 ## [7.4.0] - January 16, 2024 * Improved the visualization for array properties like Tags ([#829]) diff --git a/plugin/src/App/StatusPages/Settings/init.lua b/plugin/src/App/StatusPages/Settings/init.lua index 688be85a3..a33eb1b54 100644 --- a/plugin/src/App/StatusPages/Settings/init.lua +++ b/plugin/src/App/StatusPages/Settings/init.lua @@ -75,6 +75,12 @@ function SettingsPage:init() end function SettingsPage:render() + local layoutOrder = 0 + local function layoutIncrement() + layoutOrder += 1 + return layoutOrder + end + return Theme.with(function(theme) theme = theme.Settings @@ -86,7 +92,7 @@ function SettingsPage:render() Navbar = e(Navbar, { onBack = self.props.onBack, transparency = self.props.transparency, - layoutOrder = 0, + layoutOrder = layoutIncrement(), }), ShowNotifications = e(Setting, { @@ -94,7 +100,7 @@ function SettingsPage:render() name = "Show Notifications", description = "Popup notifications in viewport", transparency = self.props.transparency, - layoutOrder = 1, + layoutOrder = layoutIncrement(), }), SyncReminder = e(Setting, { @@ -103,7 +109,7 @@ function SettingsPage:render() description = "Notify to sync when opening a place that has previously been synced", transparency = self.props.transparency, visible = Settings:getBinding("showNotifications"), - layoutOrder = 2, + layoutOrder = layoutIncrement(), }), ConfirmationBehavior = e(Setting, { @@ -111,7 +117,7 @@ function SettingsPage:render() name = "Confirmation Behavior", description = "When to prompt for confirmation before syncing", transparency = self.props.transparency, - layoutOrder = 3, + layoutOrder = layoutIncrement(), options = confirmationBehaviors, }), @@ -121,7 +127,7 @@ function SettingsPage:render() name = "Confirmation Threshold", description = "How many modified instances to be considered a large change", transparency = self.props.transparency, - layoutOrder = 4, + layoutOrder = layoutIncrement(), visible = Settings:getBinding("confirmationBehavior"):map(function(value) return value == "Large Changes" end), @@ -152,7 +158,16 @@ function SettingsPage:render() name = "Play Sounds", description = "Toggle sound effects", transparency = self.props.transparency, - layoutOrder = 5, + layoutOrder = layoutIncrement(), + }), + + AutoConnectPlaytestServer = e(Setting, { + id = "autoConnectPlaytestServer", + name = "Auto Connect Playtest Server", + description = "Automatically connect game server to Rojo when playtesting while connected in Edit", + experimental = true, + transparency = self.props.transparency, + layoutOrder = layoutIncrement(), }), OpenScriptsExternally = e(Setting, { @@ -162,7 +177,7 @@ function SettingsPage:render() locked = self.props.syncActive, experimental = true, transparency = self.props.transparency, - layoutOrder = 6, + layoutOrder = layoutIncrement(), }), TwoWaySync = e(Setting, { @@ -172,7 +187,7 @@ function SettingsPage:render() locked = self.props.syncActive, experimental = true, transparency = self.props.transparency, - layoutOrder = 7, + layoutOrder = layoutIncrement(), }), LogLevel = e(Setting, { @@ -180,7 +195,7 @@ function SettingsPage:render() name = "Log Level", description = "Plugin output verbosity level", transparency = self.props.transparency, - layoutOrder = 100, + layoutOrder = layoutIncrement(), options = invertedLevels, showReset = Settings:getBinding("logLevel"):map(function(value) @@ -196,7 +211,7 @@ function SettingsPage:render() name = "Typechecking", description = "Toggle typechecking on the API surface", transparency = self.props.transparency, - layoutOrder = 101, + layoutOrder = layoutIncrement(), }), Layout = e("UIListLayout", { diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 22f277f9f..ecb807f05 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -158,11 +158,29 @@ function App:init() }, }) end + + if self:isAutoConnectPlaytestServerAvailable() then + self:useRunningConnectionInfo() + self:startSession() + end + self.autoConnectPlaytestServerListener = Settings:onChanged("autoConnectPlaytestServer", function(enabled) + if enabled then + if self:isAutoConnectPlaytestServerWriteable() and self.serveSession ~= nil then + -- Write the existing session + local baseUrl = self.serveSession.__apiContext.__baseUrl + self:setRunningConnectionInfo(baseUrl) + end + else + self:clearRunningConnectionInfo() + end + end) end function App:willUnmount() self.waypointConnection:Disconnect() self.confirmationBindable:Destroy() + self.autoConnectPlaytestServerListener() + self:clearRunningConnectionInfo() end function App:addNotification( @@ -278,10 +296,7 @@ function App:getHostAndPort() local host = self.host:getValue() local port = self.port:getValue() - local host = if #host > 0 then host else Config.defaultHost - local port = if #port > 0 then port else Config.defaultPort - - return host, port + return if #host > 0 then host else Config.defaultHost, if #port > 0 then port else Config.defaultPort end function App:isSyncLockAvailable() @@ -349,6 +364,49 @@ function App:releaseSyncLock() Log.trace("Could not relase sync lock because it is owned by {}", lock.Value) end +function App:isAutoConnectPlaytestServerAvailable() + return RunService:IsRunMode() + and RunService:IsServer() + and Settings:get("autoConnectPlaytestServer") + and workspace:GetAttribute("__Rojo_ConnectionUrl") +end + +function App:isAutoConnectPlaytestServerWriteable() + return RunService:IsEdit() and Settings:get("autoConnectPlaytestServer") +end + +function App:setRunningConnectionInfo(baseUrl: string) + if not self:isAutoConnectPlaytestServerWriteable() then + return + end + + Log.trace("Setting connection info for play solo auto-connect") + workspace:SetAttribute("__Rojo_ConnectionUrl", baseUrl) +end + +function App:clearRunningConnectionInfo() + if not RunService:IsEdit() then + -- Only write connection info from edit mode + return + end + + Log.trace("Clearing connection info for play solo auto-connect") + workspace:SetAttribute("__Rojo_ConnectionUrl", nil) +end + +function App:useRunningConnectionInfo() + local connectionInfo = workspace:GetAttribute("__Rojo_ConnectionUrl") + if not connectionInfo then + return + end + + Log.trace("Using connection info for play solo auto-connect") + local host, port = string.match(connectionInfo, "^(.+):(.-)$") + + self.setHost(host) + self.setPort(port) +end + function App:startSession() local claimedLock, priorOwner = self:claimSyncLock() if not claimedLock then @@ -441,6 +499,7 @@ function App:startSession() self:addNotification("Connecting to session...") elseif status == ServeSession.Status.Connected then self.knownProjects[details] = true + self:setRunningConnectionInfo(baseUrl) local address = ("%s:%s"):format(host, port) self:setState({ @@ -453,6 +512,7 @@ function App:startSession() elseif status == ServeSession.Status.Disconnected then self.serveSession = nil self:releaseSyncLock() + self:clearRunningConnectionInfo() self:setState({ patchData = { patch = PatchSet.newEmpty(), @@ -488,6 +548,12 @@ function App:startSession() return "Accept" end + -- Play solo auto-connect does not require confirmation + if self:isAutoConnectPlaytestServerAvailable() then + Log.trace("Accepting patch without confirmation because play solo auto-connect is enabled") + return "Accept" + end + local confirmationBehavior = Settings:get("confirmationBehavior") if confirmationBehavior == "Initial" then -- Only confirm if we haven't synced this project yet this session @@ -603,7 +669,7 @@ function App:render() value = self.props.plugin, }, { e(Theme.StudioProvider, nil, { - e(Tooltip.Provider, nil, { + tooltip = e(Tooltip.Provider, nil, { gui = e(StudioPluginGui, { id = pluginName, title = pluginName, diff --git a/plugin/src/Settings.lua b/plugin/src/Settings.lua index 194dd4c7a..b5be22a76 100644 --- a/plugin/src/Settings.lua +++ b/plugin/src/Settings.lua @@ -14,6 +14,7 @@ local defaultSettings = { twoWaySync = false, showNotifications = true, syncReminder = true, + autoConnectPlaytestServer = false, confirmationBehavior = "Initial", largeChangesConfirmationThreshold = 5, playSounds = true, From 106a01223e84c4dcf40ab5cbf7fb164e832d6b63 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Wed, 31 Jan 2024 14:45:28 -0800 Subject: [PATCH 2/3] Show failed additions and removals in visualizer (#844) --- plugin/src/PatchTree.lua | 72 ++++++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index e67dbcd0b..d68bdcd70 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -426,22 +426,66 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch) -- Update isWarning metadata for _, failedChange in unappliedPatch.updated do local node = tree:getNode(failedChange.id) - if node then - node.isWarning = true - Log.trace("Marked node as warning: {} {}", node.id, node.name) - - if node.changeList then - for _, change in node.changeList do - if failedChange.changedProperties[change[1]] then - Log.trace(" Marked property as warning: {}", change[1]) - if change[4] == nil then - change[4] = {} - end - change[4].isWarning = true - end - end + if not node then + continue + end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) + + if not node.changeList then + continue + end + for _, change in node.changeList do + if not failedChange.changedProperties[change[1]] then + -- This change didn't fail + continue + end + if change[4] == nil then + change[4] = { isWarning = true } + else + change[4].isWarning = true + end + Log.trace(" Marked property as warning: {}.{}", node.name, change[1]) + end + end + for failedAdditionId in unappliedPatch.added do + local node = tree:getNode(failedAdditionId) + if not node then + continue + end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) + + if not node.changeList then + continue + end + for _, change in node.changeList do + -- Failed addition means that all properties failed to be added + if change[4] == nil then + change[4] = { isWarning = true } + else + change[4].isWarning = true end + Log.trace(" Marked property as warning: {}.{}", node.name, change[1]) + end + end + for _, failedRemovalIdOrInstance in unappliedPatch.removed do + local failedRemovalId = if Types.RbxId(failedRemovalIdOrInstance) + then failedRemovalIdOrInstance + else instanceMap.fromInstances[failedRemovalIdOrInstance] + if not failedRemovalId then + continue + end + + local node = tree:getNode(failedRemovalId) + if not node then + continue end + + node.isWarning = true + Log.trace("Marked node as warning: {} {}", node.id, node.name) end -- Update if instances exist From f3b0b0027ed41939e3eaeab760777ec4f085aac3 Mon Sep 17 00:00:00 2001 From: boatbomber Date: Wed, 31 Jan 2024 17:07:01 -0800 Subject: [PATCH 3/3] Catch more sync failures (#845) - Catch removal failures - Catch name change failures - Don't remove IDs for instances if they weren't actually destroyed --- plugin/src/InstanceMap.lua | 22 ++++++++++++---------- plugin/src/PatchTree.lua | 11 ++++++++--- plugin/src/Reconciler/applyPatch.lua | 21 ++++++++++++++++----- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/plugin/src/InstanceMap.lua b/plugin/src/InstanceMap.lua index 91a1d4cdc..b035d423c 100644 --- a/plugin/src/InstanceMap.lua +++ b/plugin/src/InstanceMap.lua @@ -113,27 +113,29 @@ end function InstanceMap:destroyInstance(instance) local id = self.fromInstances[instance] + local descendants = instance:GetDescendants() + instance:Destroy() + + -- After the instance is successfully destroyed, + -- we can remove all the id mappings + if id ~= nil then self:removeId(id) end - for _, descendantInstance in ipairs(instance:GetDescendants()) do + for _, descendantInstance in descendants do self:removeInstance(descendantInstance) end - - instance:Destroy() end function InstanceMap:destroyId(id) local instance = self.fromIds[id] - self:removeId(id) - if instance ~= nil then - for _, descendantInstance in ipairs(instance:GetDescendants()) do - self:removeInstance(descendantInstance) - end - - instance:Destroy() + self:destroyInstance(instance) + else + -- There is no instance with this id, so we can just remove the id + -- without worrying about instance destruction + self:removeId(id) end end diff --git a/plugin/src/PatchTree.lua b/plugin/src/PatchTree.lua index d68bdcd70..e469e2112 100644 --- a/plugin/src/PatchTree.lua +++ b/plugin/src/PatchTree.lua @@ -437,8 +437,13 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch) continue end for _, change in node.changeList do - if not failedChange.changedProperties[change[1]] then - -- This change didn't fail + local property = change[1] + local propertyFailedToApply = if property == "Name" + then failedChange.changedName ~= nil -- Name is not in changedProperties, so it needs a special case + else failedChange.changedProperties[property] ~= nil + + if not propertyFailedToApply then + -- This change didn't fail, no need to mark continue end if change[4] == nil then @@ -446,7 +451,7 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch) else change[4].isWarning = true end - Log.trace(" Marked property as warning: {}.{}", node.name, change[1]) + Log.trace(" Marked property as warning: {}.{}", node.name, property) end end for failedAdditionId in unappliedPatch.added do diff --git a/plugin/src/Reconciler/applyPatch.lua b/plugin/src/Reconciler/applyPatch.lua index 4faa73105..a66be3a21 100644 --- a/plugin/src/Reconciler/applyPatch.lua +++ b/plugin/src/Reconciler/applyPatch.lua @@ -25,10 +25,15 @@ local function applyPatch(instanceMap, patch) local unappliedPatch = PatchSet.newEmpty() for _, removedIdOrInstance in ipairs(patch.removed) do - if Types.RbxId(removedIdOrInstance) then - instanceMap:destroyId(removedIdOrInstance) - else - instanceMap:destroyInstance(removedIdOrInstance) + local ok = pcall(function() + if Types.RbxId(removedIdOrInstance) then + instanceMap:destroyId(removedIdOrInstance) + else + instanceMap:destroyInstance(removedIdOrInstance) + end + end) + if not ok then + table.insert(unappliedPatch.removed, removedIdOrInstance) end end @@ -170,7 +175,13 @@ local function applyPatch(instanceMap, patch) end if update.changedName ~= nil then - instance.Name = update.changedName + local ok = pcall(function() + instance.Name = update.changedName + end) + if not ok then + unappliedUpdate.changedName = update.changedName + partiallyApplied = true + end end if update.changedMetadata ~= nil then