rojo-rbx / rbx-dom

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

`Model.WorldPivotData` needs a custom property setter/getter #391

Closed kennethloeffler closed 4 months ago

kennethloeffler commented 4 months ago

As discovered in #238, rbx_dom_lua is missing a custom setter/getter for Model.WorldPivotData, which causes failure to sync model pivots downstream in Rojo.

We should add a custom setter and getter for this property that use PVInstance:PivotTo and PVInstance:GetPivot.

kennethloeffler commented 4 months ago

So... there's a complication I discovered while implementing this.

For the custom property getter/setter to work for Rojo, we also need to add an encoder/decoder for OptionalCFrame, like in #179. The problem is that OptionalCFrame can be None, which is serialized to JSON like this:

{ "OptionalCFrame": null }

Roblox's HttpService:JSONDecode decodes this as an empty Lua table, which breaks the assumption that all property value tables contain a key-value pair.

This can be worked around by treating an empty table as an unoccupied optional value. I'm open to other approaches though!

Dekkonot commented 4 months ago

Intuition says it'd be fine to treat an empty table as a sentinel value.

In practical terms, I only see this being a problem if Roblox adds other optional types down the line and we just start using one underlying type for it and there's some other use for {} as a real value for one of the optional types. Which is to say, there won't be an issue as long as we don't do anything dumb.