Re-add the hack to write NeedsPivotMigration as false for models (#1027)

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Rojo Changelog
 
 ## Unreleased Changes
+* Fixed an edge case that caused model pivots to not be built correctly in some cases ([#1027])
 * Add `blockedPlaceIds` project config field to allow blocking place ids from being live synced ([#1021])
 * Adds support for `.plugin.lua(u)` files - this applies the `Plugin` RunContext. ([#1008])
 * Added support for Roblox's `Content` type. This replaces the old `Content` type with `ContentId` to reflect Roblox's change.
@@ -110,6 +111,7 @@
 [#988]: https://github.com/rojo-rbx/rojo/pull/988
 [#1008]: https://github.com/rojo-rbx/rojo/pull/1008
 [#1021]: https://github.com/rojo-rbx/rojo/pull/1021
+[#1027]: https://github.com/rojo-rbx/rojo/pull/1027
 
 ## [7.4.4] - August 22nd, 2024
 * Fixed issue with reading attributes from `Lighting` in new place files

rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_class_name.snap

@@ -6,6 +6,7 @@ expression: contents
   <Item class="Model" referent="0">
     <Properties>
       <string name="Name">init_meta_class_name</string>
+      <bool name="NeedsPivotMigration">false</bool>
     </Properties>
   </Item>
 </roblox>

rojo-test/build-test-snapshots/end_to_end__tests__build__rbxmx_in_folder.snap

@@ -1,7 +1,6 @@
 ---
 source: tests/tests/build.rs
 expression: contents
-
 ---
 <roblox version="4">
   <Item class="Folder" referent="0">
@@ -25,6 +24,7 @@ expression: contents
           <R21>0</R21>
           <R22>1</R22>
         </CoordinateFrame>
+        <bool name="NeedsPivotMigration">false</bool>
         <Ref name="PrimaryPart">null</Ref>
         <BinaryString name="Tags"></BinaryString>
       </Properties>

rojo-test/build-test-snapshots/end_to_end__tests__build__unresolved_values.snap

@@ -1,7 +1,6 @@
 ---
 source: tests/tests/build.rs
 expression: contents
-
 ---
 <roblox version="4">
   <Item class="DataModel" referent="0">
@@ -22,6 +21,7 @@ expression: contents
     <Item class="Workspace" referent="2">
       <Properties>
         <string name="Name">Workspace</string>
+        <bool name="NeedsPivotMigration">false</bool>
       </Properties>
       <Item class="BoolValue" referent="3">
         <Properties>

rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_all-2.snap

@@ -1,7 +1,6 @@
 ---
 source: tests/tests/serve.rs
 expression: "read_response.intern_and_redact(&mut redactions, root_id)"
-
 ---
 instances:
   id-2:
@@ -22,7 +21,8 @@ instances:
       ignoreUnknownInstances: false
     Name: test
     Parent: id-2
-    Properties: {}
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
 messageCursor: 1
 sessionId: id-1
-

rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_subscribe.snap

@@ -1,7 +1,6 @@
 ---
 source: tests/tests/serve.rs
 expression: "subscribe_response.intern_and_redact(&mut redactions, ())"
-
 ---
 messageCursor: 1
 messages:
@@ -14,8 +13,9 @@ messages:
           ignoreUnknownInstances: false
         Name: test
         Parent: id-2
-        Properties: {}
+        Properties:
+          NeedsPivotMigration:
+            Bool: false
     removed: []
     updated: []
 sessionId: id-1
-

rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all-2.snap

@@ -0,0 +1,64 @@
+---
+source: tests/tests/serve.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: DataModel
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: pivot_migration
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  id-3:
+    Children:
+      - id-4
+      - id-5
+      - id-6
+    ClassName: Workspace
+    Id: id-3
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: Workspace
+    Parent: id-2
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+  id-4:
+    Children: []
+    ClassName: Model
+    Id: id-4
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: Model
+    Parent: id-3
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+  id-5:
+    Children: []
+    ClassName: Tool
+    Id: id-5
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: Tool
+    Parent: id-3
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+  id-6:
+    Children: []
+    ClassName: Actor
+    Id: id-6
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: Actor
+    Parent: id-3
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+messageCursor: 1
+sessionId: id-1

rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all.snap

@@ -0,0 +1,27 @@
+---
+source: tests/tests/serve.rs
+expression: "subscribe_response.intern_and_redact(&mut redactions, ())"
+---
+messageCursor: 1
+messages:
+  - added:
+      id-6:
+        Children: []
+        ClassName: Actor
+        Id: id-6
+        Metadata:
+          ignoreUnknownInstances: true
+        Name: Actor
+        Parent: id-3
+        Properties:
+          NeedsPivotMigration:
+            Bool: false
+    removed: []
+    updated:
+      - changedClassName: ~
+        changedMetadata:
+          ignoreUnknownInstances: true
+        changedName: ~
+        changedProperties: {}
+        id: id-3
+sessionId: id-1

rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_all.snap

@@ -0,0 +1,52 @@
+---
+source: tests/tests/serve.rs
+expression: "read_response.intern_and_redact(&mut redactions, root_id)"
+---
+instances:
+  id-2:
+    Children:
+      - id-3
+    ClassName: DataModel
+    Id: id-2
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: pivot_migration
+    Parent: "00000000000000000000000000000000"
+    Properties: {}
+  id-3:
+    Children:
+      - id-4
+      - id-5
+    ClassName: Workspace
+    Id: id-3
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: Workspace
+    Parent: id-2
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+  id-4:
+    Children: []
+    ClassName: Model
+    Id: id-4
+    Metadata:
+      ignoreUnknownInstances: true
+    Name: Model
+    Parent: id-3
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+  id-5:
+    Children: []
+    ClassName: Tool
+    Id: id-5
+    Metadata:
+      ignoreUnknownInstances: false
+    Name: Tool
+    Parent: id-3
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
+messageCursor: 0
+sessionId: id-1

rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap

@@ -0,0 +1,13 @@
+---
+source: tests/tests/serve.rs
+expression: redactions.redacted_yaml(info)
+---
+expectedPlaceIds: ~
+gameId: ~
+placeId: ~
+projectName: pivot_migration
+protocolVersion: 4
+rootInstanceId: id-2
+serverVersion: "[server-version]"
+sessionId: id-1
+unexpectedPlaceIds: ~

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all-2.snap

@@ -31,6 +31,8 @@ instances:
         Attributes:
           Rojo_Target_PrimaryPart:
             String: project target
+      NeedsPivotMigration:
+        Bool: false
       PrimaryPart:
         Ref: id-9
   id-2:
@@ -55,7 +57,9 @@ instances:
       ignoreUnknownInstances: true
     Name: Workspace
     Parent: id-2
-    Properties: {}
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
   id-4:
     Children: []
     ClassName: ObjectValue
@@ -124,6 +128,8 @@ instances:
         Attributes:
           Rojo_Target_PrimaryPart:
             String: model target 2
+      NeedsPivotMigration:
+        Bool: false
       PrimaryPart:
         Ref: id-7
   id-9:
@@ -138,4 +144,3 @@ instances:
     Properties: {}
 messageCursor: 1
 sessionId: id-1
-

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all.snap

@@ -40,7 +40,9 @@ instances:
       ignoreUnknownInstances: true
     Name: Workspace
     Parent: id-2
-    Properties: {}
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
   id-4:
     Children: []
     ClassName: ObjectValue
@@ -104,6 +106,8 @@ instances:
         Attributes:
           Rojo_Target_PrimaryPart:
             String: model target
+      NeedsPivotMigration:
+        Bool: false
       PrimaryPart:
         Ref: id-7
   id-9:
@@ -118,4 +122,3 @@ instances:
     Properties: {}
 messageCursor: 0
 sessionId: id-1
-

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap

@@ -40,7 +40,9 @@ instances:
       ignoreUnknownInstances: true
     Name: Workspace
     Parent: id-2
-    Properties: {}
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
   id-4:
     Children: []
     ClassName: ObjectValue
@@ -104,8 +106,12 @@ instances:
         Attributes:
           Rojo_Target_PrimaryPart:
             String: model target
+      NeedsPivotMigration:
+        Bool: false
       PrimaryPart:
         Ref: id-7
+      Scale:
+        Float32: 1
   id-9:
     Children:
       - id-10
@@ -116,5 +122,5 @@ instances:
     Name: ProjectTarget
     Parent: id-3
     Properties: {}
-messageCursor: 0
+messageCursor: 1
 sessionId: id-1

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap

@@ -40,7 +40,9 @@ instances:
       ignoreUnknownInstances: true
     Name: Workspace
     Parent: id-2
-    Properties: {}
+    Properties:
+      NeedsPivotMigration:
+        Bool: false
   id-4:
     Children: []
     ClassName: ObjectValue
@@ -104,6 +106,8 @@ instances:
         Attributes:
           Rojo_Target_PrimaryPart:
             String: model target
+      NeedsPivotMigration:
+        Bool: false
       PrimaryPart:
         Ref: id-7
   id-9:

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_subscribe.snap

@@ -0,0 +1,17 @@
+---
+source: tests/tests/serve.rs
+expression: "subscribe_response.intern_and_redact(&mut redactions, ())"
+---
+messageCursor: 1
+messages:
+  - added: {}
+    removed: []
+    updated:
+      - changedClassName: ~
+        changedMetadata: ~
+        changedName: ~
+        changedProperties:
+          Scale:
+            Float32: 1
+        id: id-8
+sessionId: id-1

rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_subscribe.snap

@@ -18,6 +18,8 @@ messages:
             Attributes:
               Rojo_Target_PrimaryPart:
                 String: project target
+          NeedsPivotMigration:
+            Bool: false
           PrimaryPart:
             Ref: id-9
     removed: []
@@ -43,4 +45,3 @@ messages:
           PrimaryPart: ~
         id: id-8
 sessionId: id-1
-

rojo-test/serve-tests/pivot_migration/Tool.model.json

@@ -0,0 +1,3 @@
+{
+  "className": "Tool"
+}

rojo-test/serve-tests/pivot_migration/default.project.json

@@ -0,0 +1,14 @@
+{
+  "name": "pivot_migration",
+  "tree": {
+    "$className": "DataModel",
+    "Workspace": {
+      "Model": {
+        "$className": "Model"
+      },
+      "Tool": {
+        "$path": "Tool.model.json"
+      }
+    }
+  }
+}

src/snapshot/patch_compute.rs

@@ -163,6 +163,34 @@ fn compute_property_patches(
         }
     }
 
+    // !!!!!!!!!! UGLY HACK !!!!!!!!!!
+    //
+    // See RojoTree::insert_instance. Adjust that code also if you are touching this.
+    let actual_class = changed_class_name.unwrap_or(instance.class_name());
+    match actual_class.as_str() {
+        "Model" | "Actor" | "Tool" | "HopperBin" | "Flag" | "WorldModel" | "Workspace"
+        | "Status" => {
+            let migration_prop = ustr("NeedsPivotMigration");
+            // We want to just ignore this if it's being removed by a patch.
+            // Normally this would not matter because serving != building but
+            // if we start syncing models using SerializationService
+            // (or GetObjects) it will affect how Studio deserializes things.
+            if !instance.properties().contains_key(&migration_prop) {
+                changed_properties.insert(migration_prop, Some(Variant::Bool(false)));
+            }
+            match changed_properties.get(&migration_prop) {
+                Some(Some(Variant::Bool(_))) => {}
+                Some(None) => {
+                    changed_properties.remove(&migration_prop);
+                }
+                _ => {
+                    changed_properties.insert(migration_prop, Some(Variant::Bool(false)));
+                }
+            }
+        }
+        _ => {}
+    };
+
     if changed_properties.is_empty()
         && changed_name.is_none()
         && changed_class_name.is_none()

src/snapshot/tree.rs

@@ -94,9 +94,33 @@ impl RojoTree {
     }
 
     pub fn insert_instance(&mut self, parent_ref: Ref, snapshot: InstanceSnapshot) -> Ref {
+        // !!!!!!!!!! UGLY HACK !!!!!!!!!!
+        // ! If you are going to change this, go change it in patch_compute/compute_property_patches too
+        //
+        // This is a set of special cases working around a more general problem upstream
+        // in rbx-dom that causes pivots to not build to file correctly, described in
+        // github.com/rojo-rbx/rojo/issues/628 and github.com/rojo-rbx/rbx-dom/issues/385
+        //
+        // We need to insert the NeedsPivotMigration property with a value of false on
+        // every instance that inherits from Model for pivots to build correctly.
+        let hack_needs_pivot_migration = match snapshot.class_name.as_ref() {
+            // This is not a future proof way to do this but the last time a
+            // descendant of Model was added was in 2020 so it's probably fine.
+            "Model" | "Actor" | "Tool" | "HopperBin" | "Flag" | "WorldModel" | "Workspace"
+            | "Status"
+                if !snapshot
+                    .properties
+                    .contains_key(&ustr("NeedsPivotMigration")) =>
+            {
+                vec![("NeedsPivotMigration", Variant::Bool(false))]
+            }
+            _ => Vec::new(),
+        };
+
         let builder = InstanceBuilder::empty()
             .with_class(snapshot.class_name)
             .with_name(snapshot.name.into_owned())
+            .with_properties(hack_needs_pivot_migration)
             .with_properties(snapshot.properties);
 
         let referent = self.inner.insert(parent_ref, builder);

tests/tests/serve.rs

@@ -502,12 +502,35 @@ fn ref_properties_patch_update() {
             read_response.intern_and_redact(&mut redactions, root_id)
         );
 
-        let project_path = session.path().join("default.project.json");
+        let target_path = session.path().join("ModelTarget.model.json");
-        let mut project_content = fs::read(&project_path).unwrap();
-        // Adding a newline to force the change processor to pick up a change.
-        project_content.push(b'\n');
 
-        fs::write(project_path, project_content).unwrap();
+        // Inserting scale just to force the change processor to run
+        fs::write(
+            target_path,
+            r#"{
+            "id": "model target",
+            "className": "Folder",
+            "children": [
+                {
+                    "name": "ModelPointer",
+                    "className": "Model",
+                    "attributes": {
+                        "Rojo_Target_PrimaryPart": "model target"
+                    },
+                    "properties": {
+                        "Scale": 1
+                    }
+                }
+            ]
+        }"#,
+        )
+        .unwrap();
+
+        let subscribe_response = session.get_api_subscribe(0).unwrap();
+        assert_yaml_snapshot!(
+            "ref_properties_patch_update_subscribe",
+            subscribe_response.intern_and_redact(&mut redactions, ())
+        );
 
         let read_response = session.get_api_read(root_id).unwrap();
         assert_yaml_snapshot!(
@@ -516,3 +539,55 @@ fn ref_properties_patch_update() {
         );
     });
 }
+
+#[test]
+fn model_pivot_migration() {
+    run_serve_test("pivot_migration", |session, mut redactions| {
+        let info = session.get_api_rojo().unwrap();
+        let root_id = info.root_instance_id;
+
+        assert_yaml_snapshot!("pivot_migration_info", redactions.redacted_yaml(info));
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "pivot_migration_all",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+
+        let project_path = session.path().join("default.project.json");
+
+        fs::write(
+            project_path,
+            r#"{
+            "name": "pivot_migration",
+            "tree": {
+                "$className": "DataModel",
+                "Workspace": {
+                    "Model": {
+                        "$className": "Model"
+                    },
+                    "Tool": {
+                        "$path": "Tool.model.json"
+                    },
+                    "Actor": {
+                        "$className": "Actor"
+                    }
+                }
+            }
+        }"#,
+        )
+        .unwrap();
+
+        let subscribe_response = session.get_api_subscribe(0).unwrap();
+        assert_yaml_snapshot!(
+            "model_pivot_migration_all",
+            subscribe_response.intern_and_redact(&mut redactions, ())
+        );
+
+        let read_response = session.get_api_read(root_id).unwrap();
+        assert_yaml_snapshot!(
+            "model_pivot_migration_all-2",
+            read_response.intern_and_redact(&mut redactions, root_id)
+        );
+    });
+}