Fix WorldPivotData not being written in studio

[blackshibe]

fixes issue mentioned in rojo-rbx/rojo#628

I don't know if there is anything implemented to do this without using the Custom scriptable property.
Review welcome.

[LPGhatguy]

Huh, this is kinda weird. I wonder what's up with this diff!

[blackshibe]

It'll break everything randomly and we don't even know it yet.

[LPGhatguy]

WorldPivotData is kind of a funny name. That's what this property serializes as, but we should surface it as WorldPivot to the user. There is a little bit of guidance in this document that might help: https://dom.rojo.space/patching-database.html

You can also look at the patches in instance.yml that add support for attributes. They serialize as AttributesSerialize but we expose them as Attributes.

[blackshibe]

I made it use an alias like you suggested in the latest commit.

[blackshibe]

WorldPivotData can either be a CFrame or an OptionalCFrame. I don't know what that depends on, but models seem to be resaved to use OptionalCFrame every time. Is there anything implemented to handle cases like this in the reflection db?

[blackshibe]

WorldPivotData can either be a CFrame or an OptionalCFrame. I don't know what that depends on, but models seem to be resaved to use OptionalCFrame every time. Is there anything implemented to handle cases like this in the reflection db?

I made a questionable? fix that just treats CFrames like OptionalCFrames in rbx_binary, if this problem is encountered. I don't know if this occurs in rbxlx since every rbxlx file I have uses OptionalCoordinateFrame as the XML tag.

[blackshibe]

Hilarious. I'm unable to modify WorldPivot's DataType in the patch file since Change doesn't have support for it.

[blackshibe]

NeedsPivotMigration has to be defined as false inside workspace for the WorldPivot CFrame to be correct when opening the rbxlx.

I don't know what to do about this.

[ThoughtSpinnr]

NeedsPivotMigration has to be defined as false inside workspace for the WorldPivot CFrame to be correct when opening the rbxlx.

Can confirm, as well as @MaximumADHD. This cannot be worked around by setting the property in the rojo project file since it's an unknown property.

[LPGhatguy]

Can confirm, as well as @MaximumADHD. This cannot be worked around by setting the property in the rojo project file since it's an unknown property.

You can always set a property in Rojo, you just have to spell out the type. You aren't stopped from setting properties that Rojo doesn't know about.

"$properties": {
    "NeedsPivotMigration": { "Bool": false }
}

[Dekkonot]

I'm assuming no because there's been no comment on this in 7 months, but is this still something that needs to be resolved?

[nezuo]

@Dekkonot I went ahead and tested it with Rojo 7.3.0 and this is still an issue.

[kennethloeffler]

Hey @blackshibe, do you have any roblox model files that exhibit this behavior that you can PR to rojo-rbx/rbx-test-files? We like to have real files to test against whenever we run into discrepancies like this one, both to ensure that the fix works, and also to prevent future regressions.

[blackshibe]

I haven't touched this in a while but I can try to give you a few soon

[blackshibe]

@kennethloeffler

the issue mentioned in the original post for this issue has a model as well as the intended behavior.

I simplified it down to this:

I created a model with a single part in the center of the world, at a pivot position of 2,2,2.

After exporting it to file, and dragging it into rojo's path, then putting it in workspace, the pivot position is at 0,0,0 (the model center)

model.zip

I'm looking over the branch changes to verify whether it's fixed

[Crystalflxme]

Hi, are there any updates on this?

[blackshibe]

I know the JSON database equivalent for the Model class that fixes this issue but the model-pivot.yml patchfile doesn't seem to apply it. I shelved work on this because I have no idea why the patchfile doesn't change anything - but I do have a local build with a manually edited database where pivot seems to work

database.json for WorldPivot inside Rojo - this works if put inside the plugin directly:

        "WorldPivot": {
          "Name": "WorldPivot",
          "Scriptability": "ReadWrite",
          "DataType": {
            "Value": "CFrame"
          },
          "Tags": [
            "NotReplicated"
          ],
          "Kind": {
            "Canonical": {
              "SerializesAs": "WorldPivotData"
            }
          }
        },
        "WorldPivotData": {
          "AliasFor": "WorldPivot",
          "Name": "WorldPivotData",
          "Scriptability": "None",
          "DataType": {
            "Value": "OptionalCFrame"
          },
          "Tags": [
            "Hidden"
          ],
          "Kind": {
            "Alias": {
              "AliasFor": "WorldPivot"
            }
          }
        }

patchfile in latest commits.

[Crystalflxme]

I'm not well acquainted with rbx-dom, but I'm willing to help with testing! I believe this bug is causing issues with my game's workflow, by using Lune to manage assets - losing pivots on models.

[Dekkonot]

Hi, sorry for taking so long to follow up on this. I've been busy with work so I haven't given this the attention it deserves. If you'll have yet more patience, I should be able to give it some time either this week or next week to look into this.

If you want to do some stuff in the mean time, I have a bit of feedback for this pull request. The big thing I'm noticing at a glance is that we use rbx_reflector instead of generate_reflection now (this is confusing and I'm planning on removing generate_reflection soon) and this needs a test file just to verify we've actually fixed it and to prevent regressions. The test file would have to be submitted to rbx-test-files.

[kennethloeffler]

I think there are two different issues being conflated here.

First, model pivots don't work with rojo build. After some investigation, there's actually nothing wrong with how rbx-dom serializes model pivots. The incorrectness turns out to be caused by the absence of the Workspace.NeedsPivotMigration property. If this property is present and set to false, then the resulting place file will work correctly when opened in Roblox Studio. This PR does not address this problem.

Second, Rojo's plugin does not set model pivots. The cause is pretty straightforward: rbx_dom_lua is missing a custom getter/setter for Model.WorldPivotData. While it'll work to alias this property to Model.WorldPivot, and serialize Model.WorldPivot as Model.WorldPivotData, I think it's simpler to add the custom getter/setter.

[Crystalflxme]

Regarding NeedsPivotMigration, should this be something Rojo implicitly does from now on? I haven't been able to find much documentation on PVInstance.NeedsPivotMigration, so I'm not entirely certain if it should be something set for every PVInstance or just the workspace.

[kennethloeffler]

Regarding NeedsPivotMigration, should this be something Rojo implicitly does from now on? I haven't been able to find much documentation on PVInstance.NeedsPivotMigration, so I'm not entirely certain if it should be something set for every PVInstance or just the workspace.

There is no documentation for this property because it's internal, and likely a mechanism that Roblox used to fix a mistake they made in the early rollout of pivots. It's a member of Model, so every instance of a Model subclass should probably have it set to false. With typical usage, this constraint usually is satisfied when using the binary format (with the notable exception of Workspace) due to some implementation details, but to be correct in all cases we'll need to do more, yes

[Crystalflxme]

I'm looking into working on the above changes. Should I create a new PR to address both problems, starting over?

[kennethloeffler]

I'm looking into working on the above changes. Should I create a new PR to address both problems, starting over?

I've created an issue at #385 for the "missing Model.NeedsPivotMigration" problem - this particular issue might be rather thorny for you to comfortably take on, but I'd greatly appreciate if you created an issue over at https://github.com/rojo-rbx/rojo/issues/new for the sync failure problem and attempt a fix in a new PR! Make sure to include a reproduction in your issue so we can verify that the fix works, and with luck it will end up in Rojo 7.4.1.

[kennethloeffler]

@Crystalflxme @blackshibe I've created an issue over at rojo-rbx/rojo#866 and a corresponding one for this project at #391, and I've also hacked a fix together for the NeedsPivotMigration problem at rojo-rbx/rojo#865. I'll be closing this PR now since we're addressing the issues discovered here through other means, but thank you everyone for your interest and help!