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

Add Model.WorldPivotData custom getter/setter #393

Merged

Conversation

kennethloeffler
Copy link
Member

This PR closes #391 by adding a custom getter/setter for Model.WorldPivotData.

I'm a little shaky about what ought to happen when the setter receives a nil. It likely means that the model's PrimaryPart was set to nil elsewhere... I think the safest option is probably to do nothing?

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I'm not sure about the PrimaryPart thing. GetPivot works just fine without it, so requiring a PrimaryPart for it feels odd!

On the other hand, I get it and think us being conservative here is probably fine. Roblox doesn't necessarily have a way to convery "optional cframe" from within their engine, so it working regardless may just be an artifact of that.

Going to hesitantly approve this because it works and I understand the reasoning. If it ends up being a mistake, it's not like it's a huge ordeal to correct.

@Dekkonot Dekkonot merged commit 3f3d64a into rojo-rbx:master Feb 20, 2024
2 checks passed
@kennethloeffler
Copy link
Member Author

So, in my dazed confusion I did mess this up a little bit.

Model.WorldPivotData is only unoccupied for a newly created Model. If I set the pivot whatsoever, the property remains occupied regardless of if the model has a PrimaryPart or not. With this in mind, the if instance.PrimaryPart == nil then bit is crap and should be removed.

This also means that when we want the property to be unoccupied (i.e. when the setter receives a nil), we may have to create a new Model instance. I'm not sure if this level of accuracy is necessary, but we should keep it in mind

@kennethloeffler kennethloeffler deleted the world-pivot-data-custom-getter-setter branch February 20, 2024 17:38
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.

Model.WorldPivotData needs a custom property setter/getter
2 participants