Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin doesn't report Workspace.PlayerCharacterDestroyBehavior failing to be set #874

Open
nezuo opened this issue Feb 21, 2024 · 1 comment
Labels
impact: small Minor papercuts in Rojo that don't warrant immediate resolutoin. scope: plugin Relevant to the Roblox Studio plugin size: small status: ready for work Ready to go! We would accept a PR to do this.

Comments

@nezuo
Copy link
Contributor

nezuo commented Feb 21, 2024

Reproduction steps:

  1. Initialize a new Rojo project.
  2. Set the Workspace.PlayerCharacterDestroyBehavior to "Enabled" in the project file.
  3. Run rojo serve.
  4. Open a blank studio place (don't build this project because it will have the updated property) and sync.
  5. Rojo won't show a warning.

This issue may exist for more properties but this is the one I tested.

@kennethloeffler kennethloeffler added scope: plugin Relevant to the Roblox Studio plugin size: small impact: small Minor papercuts in Rojo that don't warrant immediate resolutoin. status: ready for work Ready to go! We would accept a PR to do this. labels Feb 21, 2024
@kennethloeffler
Copy link
Member

kennethloeffler commented Mar 13, 2024

This occurs because Workspace.PlayerCharacterDestroyBehavior is unreadable from Lua. In the plugin's diff function, unreadable properties are skipped:

local changedProperties = {}
for propertyName, virtualValue in pairs(virtualInstance.Properties) do
local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName)
if getProperySuccess then
local existingValue = existingValueOrErr
local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap)
if decodeSuccess then
if not trueEquals(existingValue, decodedValue) then
Log.debug(
"{}.{} changed from '{}' to '{}'",
instance:GetFullName(),
propertyName,
existingValue,
decodedValue
)
changedProperties[propertyName] = virtualValue
end
else
Log.warn(
"Failed to decode property {}.{}. Encoded property was: {:#?}",
virtualInstance.ClassName,
propertyName,
virtualValue
)
end
else
local err = existingValueOrErr
if err.kind == Error.UnknownProperty then
Log.trace("Skipping unknown property {}.{}", err.details.className, err.details.propertyName)
else
Log.trace("Skipping unreadable property {}.{}", err.details.className, err.details.propertyName)
end
end
end

Because properties unreadable from Lua are skipped here, they never make it to patch application, which is where the "unapplied patch" logic actually lives, which is what informs the patch visualizer warnings:

if update.changedProperties ~= nil then
for propertyName, propertyValue in pairs(update.changedProperties) do
local decodeSuccess, decodedValue = decodeValue(propertyValue, instanceMap)
if not decodeSuccess then
unappliedUpdate.changedProperties[propertyName] = propertyValue
partiallyApplied = true
continue
end
local setPropertySuccess = setProperty(instance, propertyName, decodedValue)
if not setPropertySuccess then
unappliedUpdate.changedProperties[propertyName] = propertyValue
partiallyApplied = true
end
end
end
if partiallyApplied then
table.insert(unappliedPatch.updated, unappliedUpdate)
end

I think to fix this, we'll have to track any properties we were unable to diff in the same way we track which properties we were unable to set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: small Minor papercuts in Rojo that don't warrant immediate resolutoin. scope: plugin Relevant to the Roblox Studio plugin size: small status: ready for work Ready to go! We would accept a PR to do this.
Projects
None yet
Development

No branches or pull requests

2 participants