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

Override the default value of WorldPivotData to be null #450

Merged
merged 4 commits into from
Sep 24, 2024

Conversation

Dekkonot
Copy link
Member

Model.WorldPivotData isn't serialized as a real value by Roblox unless it's explicitly set. We should probably mimic this behavior.

The reason it hasn't been this value is because of pivot migrations. If a Model is inserted without NeedsPivotMigration set to false (as is the case in rbx-reflector), Roblox generates a pivot and serializes it, rather than leaving it blank.

If you instead insert a model that has NeedsPivotMigration set to false but doesn't have any other properties set, you can see that WorldPivotData is left blank.

Copy link
Member

@kennethloeffler kennethloeffler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the reasoning, and I can verify that

If you instead insert a model that has NeedsPivotMigration set to false but doesn't have any other properties set, you can see that WorldPivotData is left blank.

matches the behavior of Roblox Studio's serializer, so LGTM!

I wonder if there are any other defaults that depend on the values of other properties... 🤔

@Dekkonot
Copy link
Member Author

I wonder if there are any other defaults that depend on the values of other properties... 🤔

That one might be hard to test, but we could always go and insert every Instance manually and see if anything differs dramatically...

@Dekkonot Dekkonot merged commit 4530e2b into rojo-rbx:master Sep 24, 2024
3 checks passed
@Dekkonot Dekkonot deleted the worldpivotdata-null branch September 24, 2024 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants