rojo-rbx / rbx-dom

Roblox DOM and (de)serialization implementation in Rust
MIT License
113 stars 46 forks source link

BrickColor cannot be set because it aliases to Color without handling the type difference #319

Closed boatbomber closed 1 year ago

boatbomber commented 1 year ago

We have BrickColor set as an alias for Color: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_dom_lua/src/database.json#L3422-L3426

However, this means that a property write will attempt to set a BrickColor Enum to the Color property that expects a Color3 value.

Setting Part.BrickColor to (BrickColor) Hot pink - setProperty:30 Error(Roblox: Unable to assign property Color. Color3 expected, got BrickColor PluginDebugService.user_Rojo.rbxm.Rojo.Packages.RbxDom.PropertyDescriptor:19 function set PluginDebugService.user_Rojo.rbxm.Rojo.Packages.RbxDom.PropertyDescriptor:77 function write PluginDebugService.user_Rojo.rbxm.Rojo.Plugin.Reconciler.setProperty:32 function setProperty

Dekkonot commented 1 year ago

This appears to be the fault of us aliasing BasePart.BrickColor to BasePart.Color. With the migration system we now have, it should be straightforward to move away from an alias and instead have the relationship between those two be a migration.

kennethloeffler commented 1 year ago

Looking into this more, and I can create a problem with the same cause in Lune:

local roblox = require("@lune/roblox")

local part = roblox.Instance.new("Part")
part.BrickColor = roblox.BrickColor.new(45)

roblox.serializeModel({ part })
Failed to write document to buffer - Property type mismatch: Expected Part.Color to be of type Color3uint8 or Color3, but it was of type BrickColor on instance ROOT.Part
[Stack Begin]
    Script '[C]'
    Script 'test', Line 6
[Stack End]

I think treating BasePart.BrickColor as a canonical, serializable property, or using a migration (if we're sure we always want it to turn into BasePart.Color) are both valid ways out here

boatbomber commented 1 year ago

I think migration is the way to go here. When you set Color in Studio, then BrickColor gets set to the nearest match value which indicates that BrickColor just sets Color to itself so the nearest value is therefore the BrickColor you chose.

Dekkonot commented 1 year ago

I think treating BasePart.BrickColor as a canonical, serializable property, or using a migration (if we're sure we always want it to turn into BasePart.Color) are both valid ways out here

I can see a scenario like this happening if we do a migration:

  1. Serialize a Part that has BrickColor set
  2. Deserialize the file in something like Lune
  3. Attempt to index Part.BrickColor and cry that it doesn't work

This is already possible with something like Font and FontFace though, and we've gotten no complaints.

I think other than that, it's pretty much desirable 100% of the time to convert BasePart.BrickColor into BasePart.Color. A migration feels like the way to go.