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

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

Closed
kennethloeffler opened this issue Feb 16, 2024 · 2 comments · Fixed by #393
Closed

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

kennethloeffler opened this issue Feb 16, 2024 · 2 comments · Fixed by #393

Comments

@kennethloeffler
Copy link
Member

kennethloeffler commented Feb 16, 2024

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
Copy link
Member Author

kennethloeffler commented Feb 18, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants