diff --git a/CHANGELOG.md b/CHANGELOG.md index 91ecd079..bb5a8d98 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_class_name.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_class_name.snap index 889fca12..cf9c2d2a 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_class_name.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__init_meta_class_name.snap @@ -6,6 +6,7 @@ expression: contents init_meta_class_name + false diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__rbxmx_in_folder.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__rbxmx_in_folder.snap index db602853..5518a83d 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__rbxmx_in_folder.snap +++ b/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 - --- @@ -25,6 +24,7 @@ expression: contents 0 1 + false null diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__unresolved_values.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__unresolved_values.snap index 8b9eac0e..805b1d50 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__unresolved_values.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__unresolved_values.snap @@ -1,7 +1,6 @@ --- source: tests/tests/build.rs expression: contents - --- @@ -22,6 +21,7 @@ expression: contents Workspace + false diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_all-2.snap index 6f02d667..b6ccce20 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_all-2.snap +++ b/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 - diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_subscribe.snap index bb747b86..95306f15 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__empty_json_model_subscribe.snap +++ b/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 - diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all-2.snap new file mode 100644 index 00000000..e69d71f4 --- /dev/null +++ b/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 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__model_pivot_migration_all.snap new file mode 100644 index 00000000..be5a46a8 --- /dev/null +++ b/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 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_all.snap new file mode 100644 index 00000000..7e47619d --- /dev/null +++ b/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 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__pivot_migration_info.snap new file mode 100644 index 00000000..325e4488 --- /dev/null +++ b/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: ~ diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all-2.snap index 63f6c550..7f895b96 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all-2.snap +++ b/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 - diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all.snap index 7fe614cd..26e1914d 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_all.snap +++ b/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 - diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap index f29e43e6..da499167 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all-2.snap +++ b/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 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap index f29e43e6..26e1914d 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_all.snap +++ b/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: diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_patch_update_subscribe.snap new file mode 100644 index 00000000..5ee76986 --- /dev/null +++ b/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 diff --git a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_subscribe.snap b/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_subscribe.snap index 4500894a..37563913 100644 --- a/rojo-test/serve-test-snapshots/end_to_end__tests__serve__ref_properties_subscribe.snap +++ b/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 - diff --git a/rojo-test/serve-tests/pivot_migration/Tool.model.json b/rojo-test/serve-tests/pivot_migration/Tool.model.json new file mode 100644 index 00000000..683e403c --- /dev/null +++ b/rojo-test/serve-tests/pivot_migration/Tool.model.json @@ -0,0 +1,3 @@ +{ + "className": "Tool" +} \ No newline at end of file diff --git a/rojo-test/serve-tests/pivot_migration/default.project.json b/rojo-test/serve-tests/pivot_migration/default.project.json new file mode 100644 index 00000000..7a2c548f --- /dev/null +++ b/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" + } + } + } +} \ No newline at end of file diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index c240e1e9..cc3fbbbb 100644 --- a/src/snapshot/patch_compute.rs +++ b/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() diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index 950b6279..c2cabac5 100644 --- a/src/snapshot/tree.rs +++ b/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); diff --git a/tests/tests/serve.rs b/tests/tests/serve.rs index 67ee7dbe..94dec37e 100644 --- a/tests/tests/serve.rs +++ b/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 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'); + let target_path = session.path().join("ModelTarget.model.json"); - 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) + ); + }); +}