diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__all_middleware.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__all_middleware.snap index 3db4bb1f6..f001225cd 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__all_middleware.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__all_middleware.snap @@ -17,9 +17,12 @@ added_files: - src/rbxmx.rbxmx - src/text.txt added_dirs: + - src - src/csv_init + - src/dir - src/dir/init_client_script - src/dir/init_module_script - src/dir/init_server_script + - src/dir_with_meta removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__child_but_not.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__child_but_not.snap index 06a661b0c..280232eb3 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__child_but_not.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__child_but_not.snap @@ -5,6 +5,8 @@ expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" added_files: - OnlyOneCopy/child_of_one.luau - ReplicatedStorage/child_replicated_storage.luau -added_dirs: [] +added_dirs: + - OnlyOneCopy + - ReplicatedStorage removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_paths_removing.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_paths_removing.snap index c2a0b917e..46b76d56a 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_paths_removing.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_paths_removing.snap @@ -1,6 +1,5 @@ --- source: tests/rojo_test/syncback_util.rs -assertion_line: 48 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" --- added_files: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees.snap index dcdb221c8..3730bf053 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees.snap @@ -5,6 +5,7 @@ expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" added_files: - src/IncludeMe/.gitkeep added_dirs: + - src - src/IncludeMe removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing.snap index 9fa9aa7a2..46b76d56a 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ignore_trees_removing.snap @@ -1,9 +1,9 @@ --- source: tests/rojo_test/syncback_util.rs -assertion_line: 45 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" --- added_files: [] -added_dirs: [] +added_dirs: + - src removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_model.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_model.snap new file mode 100644 index 000000000..e945f5d7a --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_model.snap @@ -0,0 +1,10 @@ +--- +source: tests/rojo_test/syncback_util.rs +expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" +--- +added_files: + - ChangedProperties.model.json + - default.project.json +added_dirs: [] +removed_files: [] +removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_models.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_models.snap new file mode 100644 index 000000000..61e4629e1 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__json_models.snap @@ -0,0 +1,12 @@ +--- +source: tests/rojo_test/syncback_util.rs +expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" +--- +added_files: + - ChangedClassName/ChangedClassName.model.json + - ChangedProperties.model.json + - default.project.json +added_dirs: + - ChangedClassName +removed_files: [] +removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird.snap index f45539f41..a3a522539 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__nested_projects_weird.snap @@ -5,6 +5,7 @@ expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" added_files: - src/modules/ClientModule.luau - src/modules/ServerModule.luau -added_dirs: [] +added_dirs: + - src/modules removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_all_middleware.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_all_middleware.snap index 4050efd19..ab0bb69cc 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_all_middleware.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_all_middleware.snap @@ -17,6 +17,8 @@ added_files: - src/text.txt added_dirs: - src/csv_init + - src/dir + - src/dir_with_meta - src/init_client_script - src/init_module_script - src/init_server_script diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback.snap index 6b308f1d7..daf6a8e58 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__rbxm_fallback.snap @@ -6,6 +6,7 @@ added_files: - ReplicatedStorage/ChildWithDuplicates.rbxm - ReplicatedStorage/ChildWithoutDuplicates/Child/.gitkeep added_dirs: + - ReplicatedStorage - ReplicatedStorage/ChildWithoutDuplicates - ReplicatedStorage/ChildWithoutDuplicates/Child removed_files: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties.snap index 77d69e6e7..a67fbf168 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties.snap @@ -5,6 +5,7 @@ expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" added_files: - src/pointer.model.json - src/target.model.json -added_dirs: [] +added_dirs: + - src removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_update.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_update.snap index 77d69e6e7..a67fbf168 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_update.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__ref_properties_update.snap @@ -5,6 +5,7 @@ expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" added_files: - src/pointer.model.json - src/target.model.json -added_dirs: [] +added_dirs: + - src removed_files: [] removed_dirs: [] diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__remove_default_props.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__remove_default_props.snap new file mode 100644 index 000000000..cd0063571 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__remove_default_props.snap @@ -0,0 +1,16 @@ +--- +source: tests/rojo_test/syncback_util.rs +expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" +--- +added_files: + - AdjacentMetadataScript/AdjacentMetadataScript.meta.json + - AdjacentMetadataScript/AdjacentMetadataScript.server.lua + - DirectoryMetadataPart/Part/init.meta.json + - Part.model.json + - default.project.json +added_dirs: + - AdjacentMetadataScript + - DirectoryMetadataPart + - DirectoryMetadataPart/Part +removed_files: [] +removed_dirs: [] diff --git a/rojo-test/syncback-tests/ignore_paths_removing/expected/default.project.json b/rojo-test/syncback-tests/ignore_paths_removing/expected/default.project.json index deefcff22..89b0cb041 100644 --- a/rojo-test/syncback-tests/ignore_paths_removing/expected/default.project.json +++ b/rojo-test/syncback-tests/ignore_paths_removing/expected/default.project.json @@ -4,18 +4,6 @@ "$className": "DataModel", "Workspace": { "$properties": { - "CSGAsyncDynamicCollision": { - "Enum": 0 - }, - "DecreaseMinimumPartDensityMode": { - "Enum": 0 - }, - "MoverConstraintRootBehavior": { - "Enum": 0 - }, - "RenderingCacheOptimizations": { - "Enum": 0 - }, "SignalBehavior": "Deferred", "StreamOutBehavior": "Opportunistic", "StreamingEnabled": true, diff --git a/rojo-test/syncback-tests/json_models/expected/ChangedClassName/ChangedClassName.model.json b/rojo-test/syncback-tests/json_models/expected/ChangedClassName/ChangedClassName.model.json new file mode 100644 index 000000000..61a0557ce --- /dev/null +++ b/rojo-test/syncback-tests/json_models/expected/ChangedClassName/ChangedClassName.model.json @@ -0,0 +1,3 @@ +{ + "className": "RemoteFunction" +} diff --git a/rojo-test/syncback-tests/json_models/expected/ChangedProperties.model.json b/rojo-test/syncback-tests/json_models/expected/ChangedProperties.model.json new file mode 100644 index 000000000..9f3c95a32 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/expected/ChangedProperties.model.json @@ -0,0 +1,13 @@ +{ + "className": "Part", + "properties": { + "BottomSurface": "Smooth", + "Size": [ + 4.0, + 1.0, + 2.0 + ], + "TopSurface": "Smooth", + "Transparency": 1.0 + } +} diff --git a/rojo-test/syncback-tests/json_models/expected/NoChangeShouldntReserialize.model.json b/rojo-test/syncback-tests/json_models/expected/NoChangeShouldntReserialize.model.json new file mode 100644 index 000000000..9f3c95a32 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/expected/NoChangeShouldntReserialize.model.json @@ -0,0 +1,13 @@ +{ + "className": "Part", + "properties": { + "BottomSurface": "Smooth", + "Size": [ + 4.0, + 1.0, + 2.0 + ], + "TopSurface": "Smooth", + "Transparency": 1.0 + } +} diff --git a/rojo-test/syncback-tests/json_models/expected/default.project.json b/rojo-test/syncback-tests/json_models/expected/default.project.json new file mode 100644 index 000000000..0a13f50b8 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/expected/default.project.json @@ -0,0 +1,17 @@ +{ + "name": "json_model", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "ChangedClassName": { + "$path": "ChangedClassName" + }, + "ChangedProperties": { + "$path": "ChangedProperties.model.json" + }, + "NoChangeShouldntReserialize": { + "$path": "NoChangeShouldntReserialize.model.json" + } + } + } +} diff --git a/rojo-test/syncback-tests/json_models/input.rbxl b/rojo-test/syncback-tests/json_models/input.rbxl new file mode 100644 index 000000000..fbe9ca30a Binary files /dev/null and b/rojo-test/syncback-tests/json_models/input.rbxl differ diff --git a/rojo-test/syncback-tests/json_models/output/ChangedClassName/ChangedClassName.model.json b/rojo-test/syncback-tests/json_models/output/ChangedClassName/ChangedClassName.model.json new file mode 100644 index 000000000..dc005afc5 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/output/ChangedClassName/ChangedClassName.model.json @@ -0,0 +1,3 @@ +{ + "className": "RemoteEvent" +} diff --git a/rojo-test/syncback-tests/json_models/output/ChangedProperties.model.json b/rojo-test/syncback-tests/json_models/output/ChangedProperties.model.json new file mode 100644 index 000000000..c7dea6dd0 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/output/ChangedProperties.model.json @@ -0,0 +1,3 @@ +{ + "className": "Part" +} diff --git a/rojo-test/syncback-tests/json_models/output/NoChangeShouldntReserialize.model.json b/rojo-test/syncback-tests/json_models/output/NoChangeShouldntReserialize.model.json new file mode 100644 index 000000000..9f3c95a32 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/output/NoChangeShouldntReserialize.model.json @@ -0,0 +1,13 @@ +{ + "className": "Part", + "properties": { + "BottomSurface": "Smooth", + "Size": [ + 4.0, + 1.0, + 2.0 + ], + "TopSurface": "Smooth", + "Transparency": 1.0 + } +} diff --git a/rojo-test/syncback-tests/json_models/output/default.project.json b/rojo-test/syncback-tests/json_models/output/default.project.json new file mode 100644 index 000000000..0a13f50b8 --- /dev/null +++ b/rojo-test/syncback-tests/json_models/output/default.project.json @@ -0,0 +1,17 @@ +{ + "name": "json_model", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "ChangedClassName": { + "$path": "ChangedClassName" + }, + "ChangedProperties": { + "$path": "ChangedProperties.model.json" + }, + "NoChangeShouldntReserialize": { + "$path": "NoChangeShouldntReserialize.model.json" + } + } + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/expected/AdjacentMetadataScript/AdjacentMetadataScript.meta.json b/rojo-test/syncback-tests/remove_default_props/expected/AdjacentMetadataScript/AdjacentMetadataScript.meta.json new file mode 100644 index 000000000..cfadea686 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/expected/AdjacentMetadataScript/AdjacentMetadataScript.meta.json @@ -0,0 +1,5 @@ +{ + "properties": { + "Disabled": true + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/expected/AdjacentMetadataScript/AdjacentMetadataScript.server.lua b/rojo-test/syncback-tests/remove_default_props/expected/AdjacentMetadataScript/AdjacentMetadataScript.server.lua new file mode 100644 index 000000000..e69de29bb diff --git a/rojo-test/syncback-tests/remove_default_props/expected/DirectoryMetadataPart/Part/init.meta.json b/rojo-test/syncback-tests/remove_default_props/expected/DirectoryMetadataPart/Part/init.meta.json new file mode 100644 index 000000000..138a460e9 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/expected/DirectoryMetadataPart/Part/init.meta.json @@ -0,0 +1,6 @@ +{ + "properties": { + "Anchored": true + }, + "className": "Part" +} diff --git a/rojo-test/syncback-tests/remove_default_props/expected/Part.model.json b/rojo-test/syncback-tests/remove_default_props/expected/Part.model.json new file mode 100644 index 000000000..78bdb8435 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/expected/Part.model.json @@ -0,0 +1,10 @@ +{ + "className": "Part", + "properties": { + "Size": [ + 1.0, + 2.0, + 3.0 + ] + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/expected/default.project.json b/rojo-test/syncback-tests/remove_default_props/expected/default.project.json new file mode 100644 index 000000000..8adb932da --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/expected/default.project.json @@ -0,0 +1,23 @@ +{ + "name": "remove_default_props", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "AdjacentMetadataScript": { + "$path": "AdjacentMetadataScript" + }, + "DirectoryMetadataPart": { + "$path": "DirectoryMetadataPart" + }, + "JsonModelPart": { + "$path": "Part.model.json" + }, + "ProjectFilePart": { + "$className": "Part", + "$properties": { + "Transparency": 1.0 + } + } + } + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/input.rbxl b/rojo-test/syncback-tests/remove_default_props/input.rbxl new file mode 100644 index 000000000..9f20f6b05 Binary files /dev/null and b/rojo-test/syncback-tests/remove_default_props/input.rbxl differ diff --git a/rojo-test/syncback-tests/remove_default_props/output/AdjacentMetadataScript/AdjacentMetadataScript.meta.json b/rojo-test/syncback-tests/remove_default_props/output/AdjacentMetadataScript/AdjacentMetadataScript.meta.json new file mode 100644 index 000000000..650d48392 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/output/AdjacentMetadataScript/AdjacentMetadataScript.meta.json @@ -0,0 +1,7 @@ +{ + "properties": { + "Disabled": true, + "RunContext": "Client", + "Archivable": false + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/output/AdjacentMetadataScript/AdjacentMetadataScript.server.lua b/rojo-test/syncback-tests/remove_default_props/output/AdjacentMetadataScript/AdjacentMetadataScript.server.lua new file mode 100644 index 000000000..e69de29bb diff --git a/rojo-test/syncback-tests/remove_default_props/output/DirectoryMetadataPart/Part/init.meta.json b/rojo-test/syncback-tests/remove_default_props/output/DirectoryMetadataPart/Part/init.meta.json new file mode 100644 index 000000000..d79c6a7b8 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/output/DirectoryMetadataPart/Part/init.meta.json @@ -0,0 +1,8 @@ +{ + "properties": { + "CanCollide": false, + "Massless": false, + "Anchored": true + }, + "className": "Part" +} diff --git a/rojo-test/syncback-tests/remove_default_props/output/Part.model.json b/rojo-test/syncback-tests/remove_default_props/output/Part.model.json new file mode 100644 index 000000000..49e050081 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/output/Part.model.json @@ -0,0 +1,12 @@ +{ + "className": "Part", + "properties": { + "Size": [ + 1.0, + 2.0, + 3.0 + ], + "Anchored": true, + "Massless": true + } +} diff --git a/rojo-test/syncback-tests/remove_default_props/output/default.project.json b/rojo-test/syncback-tests/remove_default_props/output/default.project.json new file mode 100644 index 000000000..226de3725 --- /dev/null +++ b/rojo-test/syncback-tests/remove_default_props/output/default.project.json @@ -0,0 +1,29 @@ +{ + "name": "remove_default_props", + "tree": { + "$className": "DataModel", + "ReplicatedStorage": { + "AdjacentMetadataScript": { + "$path": "AdjacentMetadataScript" + }, + "DirectoryMetadataPart": { + "$path": "DirectoryMetadataPart" + }, + "JsonModelPart": { + "$path": "Part.model.json" + }, + "ProjectFilePart": { + "$className": "Part", + "$properties": { + "Size": [ + 1.0, + 2.0, + 3.0 + ], + "Transparency": 1.0, + "CanCollide": false + } + } + } + } +} diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index c176066c3..18c8cd090 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -1,6 +1,6 @@ use std::{ borrow::Cow, - collections::{BTreeMap, HashMap}, + collections::{BTreeMap, HashMap, VecDeque}, path::Path, str, }; @@ -17,6 +17,8 @@ use crate::{ RojoRef, }; +use super::{node_should_reserialize, populate_unresolved_properties}; + pub fn snapshot_json_model( context: &InstanceContext, vfs: &Vfs, @@ -67,17 +69,62 @@ pub fn snapshot_json_model( pub fn syncback_json_model<'sync>( snapshot: &SyncbackSnapshot<'sync>, ) -> anyhow::Result> { + fn serialize<'sync>(path: &Path, model: &JsonModel) -> anyhow::Result> { + Ok(SyncbackReturn { + fs_snapshot: FsSnapshot::new().with_added_file( + path, + serde_json::to_vec_pretty(model).context("failed to serialize new JSON Model")?, + ), + children: Vec::new(), + removed_children: Vec::new(), + }) + } let mut property_buffer = Vec::with_capacity(snapshot.new_inst().properties.len()); - let mut model = json_model_from_pair(snapshot, &mut property_buffer, snapshot.new); + let mut root_model = json_model_from_pair(snapshot, &mut property_buffer, snapshot.new); // We don't need the name on the root, but we do for children. - model.name = None; + root_model.name = None; + + let mut node_queue = VecDeque::new(); + node_queue.push_back((&root_model, snapshot.old_inst(), Some(snapshot.new_inst()))); + + while let Some((model, old_inst, new_inst)) = node_queue.pop_front() { + let Some(old_inst) = old_inst else { + return serialize(&snapshot.path, &root_model); + }; + + let Some(new_inst) = new_inst else { + return serialize(&snapshot.path, &root_model); + }; + + if old_inst.class_name() != new_inst.class { + return serialize(&snapshot.path, &root_model); + } + + if old_inst.name() != new_inst.name { + return serialize(&snapshot.path, &root_model); + } + + if node_should_reserialize(&model.properties, &model.attributes, old_inst)? { + return serialize(&snapshot.path, &root_model); + } + + for ((child_model, old_child_ref), new_child_ref) in model + .children + .iter() + .zip(old_inst.children().iter()) + .zip(new_inst.children().iter()) + { + node_queue.push_back(( + child_model, + snapshot.get_old_instance(*old_child_ref), + snapshot.get_new_instance(*new_child_ref), + )) + } + } Ok(SyncbackReturn { - fs_snapshot: FsSnapshot::new().with_added_file( - &snapshot.path, - serde_json::to_vec_pretty(&model).context("failed to serialize new JSON Model")?, - ), + fs_snapshot: FsSnapshot::new(), children: Vec::new(), removed_children: Vec::new(), }) @@ -94,36 +141,12 @@ fn json_model_from_pair<'sync>( filter_properties_preallocated(snapshot.project(), new_inst, prop_buffer); - let mut properties = BTreeMap::new(); - let mut attributes = BTreeMap::new(); - for (name, value) in prop_buffer.drain(..) { - match value { - Variant::Attributes(attrs) => { - for (attr_name, attr_value) in attrs.iter() { - // We (probably) don't want to preserve internal attributes, - // only user defined ones. - if attr_name.starts_with("RBX") { - continue; - } - attributes.insert( - attr_name.clone(), - UnresolvedValue::from_variant_unambiguous(attr_value.clone()), - ); - } - } - Variant::SharedString(_) => { - log::warn!( - "Rojo cannot serialize the property {}.{name} in model.json files.\n\ - If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", new_inst.class, snapshot.get_new_inst_path(new)) - } - _ => { - properties.insert( - name.to_owned(), - UnresolvedValue::from_variant(value.clone(), &new_inst.class, name), - ); - } - } - } + let (properties, attributes) = { + let prop_buffer: &_ = prop_buffer; + populate_unresolved_properties(snapshot, new_inst, prop_buffer.iter().copied()) + }; + + prop_buffer.clear(); let mut children = Vec::with_capacity(new_inst.children().len()); diff --git a/src/snapshot_middleware/meta_file.rs b/src/snapshot_middleware/meta_file.rs index 3004ad2aa..efa617fa5 100644 --- a/src/snapshot_middleware/meta_file.rs +++ b/src/snapshot_middleware/meta_file.rs @@ -6,13 +6,15 @@ use std::{ use anyhow::{format_err, Context}; use memofs::{IoResultExt as _, Vfs}; -use rbx_dom_weak::types::{Attributes, Variant}; +use rbx_dom_weak::types::Attributes; use serde::{Deserialize, Serialize}; use crate::{ resolution::UnresolvedValue, snapshot::InstanceSnapshot, syncback::SyncbackSnapshot, RojoRef, }; +use super::{node_should_reserialize, populate_unresolved_properties}; + /// Represents metadata in a sibling file with the same basename. /// /// As an example, hello.meta.json next to hello.lua would allow assigning @@ -55,8 +57,6 @@ impl AdjacentMetadata { snapshot: &SyncbackSnapshot, path: PathBuf, ) -> anyhow::Result> { - let mut properties = BTreeMap::new(); - let mut attributes = BTreeMap::new(); // TODO make this more granular. // I am breaking the cycle of bad TODOs. This is in reference to the fact // that right now, this will just not write any metadata at all for @@ -80,34 +80,16 @@ impl AdjacentMetadata { .map(|inst| inst.metadata().ignore_unknown_instances) .unwrap_or_default(); - let class = &snapshot.new_inst().class; - for (name, value) in snapshot.get_path_filtered_properties(snapshot.new).unwrap() { - match value { - Variant::Attributes(attrs) => { - for (attr_name, attr_value) in attrs.iter() { - // We (probably) don't want to preserve internal - // attributes, only user defined ones. - if attr_name.starts_with("RBX") { - continue; - } - attributes.insert( - attr_name.clone(), - UnresolvedValue::from_variant_unambiguous(attr_value.clone()), - ); - } - } - Variant::SharedString(_) => { - log::warn!( - "Rojo cannot serialize the property {}.{name} in meta.json files.\n\ - If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", class, snapshot.get_new_inst_path(snapshot.new)) - } - _ => { - properties.insert( - name.to_owned(), - UnresolvedValue::from_variant(value.clone(), class, name), - ); - } - } + let (properties, attributes) = populate_unresolved_properties( + snapshot, + snapshot.new_inst(), + snapshot.get_path_filtered_properties(snapshot.new).unwrap(), + ); + + if let Some(old_inst) = snapshot.old_inst() { + if !node_should_reserialize(&properties, &attributes, old_inst)? { + return Ok(None); + }; } Ok(Some(Self { @@ -241,8 +223,6 @@ impl DirectoryMetadata { snapshot: &SyncbackSnapshot, path: PathBuf, ) -> anyhow::Result> { - let mut properties = BTreeMap::new(); - let mut attributes = BTreeMap::new(); // TODO make this more granular. // I am breaking the cycle of bad TODOs. This is in reference to the fact // that right now, this will just not write any metadata at all for @@ -266,34 +246,16 @@ impl DirectoryMetadata { .map(|inst| inst.metadata().ignore_unknown_instances) .unwrap_or_default(); - let class = &snapshot.new_inst().class; - for (name, value) in snapshot.get_path_filtered_properties(snapshot.new).unwrap() { - match value { - Variant::Attributes(attrs) => { - for (name, value) in attrs.iter() { - // We (probably) don't want to preserve internal - // attributes, only user defined ones. - if name.starts_with("RBX") { - continue; - } - attributes.insert( - name.to_owned(), - UnresolvedValue::from_variant_unambiguous(value.clone()), - ); - } - } - Variant::SharedString(_) => { - log::warn!( - "Rojo cannot serialize the property {}.{name} in meta.json files.\n\ - If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", class, snapshot.get_new_inst_path(snapshot.new)) - } - _ => { - properties.insert( - name.to_owned(), - UnresolvedValue::from_variant(value.clone(), class, name), - ); - } - } + let (properties, attributes) = populate_unresolved_properties( + snapshot, + snapshot.new_inst(), + snapshot.get_path_filtered_properties(snapshot.new).unwrap(), + ); + + if let Some(old_inst) = snapshot.old_inst() { + if !node_should_reserialize(&properties, &attributes, old_inst)? { + return Ok(None); + }; } Ok(Some(Self { diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 4b1c00f76..881c0993b 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -19,17 +19,22 @@ mod txt; mod util; use std::{ + collections::BTreeMap, path::{Path, PathBuf}, sync::OnceLock, }; use anyhow::Context; use memofs::{IoResultExt, Vfs}; +use rbx_dom_weak::{types::Variant, Instance}; use serde::{Deserialize, Serialize}; use crate::{ glob::Glob, + resolution::UnresolvedValue, syncback::{SyncbackReturn, SyncbackSnapshot}, + variant_eq::variant_eq, + InstanceWithMeta, }; use crate::{ snapshot::{InstanceContext, InstanceSnapshot, SyncRule}, @@ -388,3 +393,171 @@ pub fn default_sync_rules() -> &'static [SyncRule] { ] }) } + +/// Generates and returns two maps of unresolved properties and attributes, +/// respectively, appropriate for producing JSON or other file representations +/// that use unresolved variants. This function ignores any properties that are +/// at their default values. It also ignores any reserved attributes (i.e. ones +/// with names starting with `RBX`). +fn populate_unresolved_properties<'prop, I>( + snapshot: &SyncbackSnapshot, + new_inst: &Instance, + properties: I, +) -> ( + BTreeMap, + BTreeMap, +) +where + I: IntoIterator, +{ + let db = rbx_reflection_database::get(); + let class_descriptor = db.classes.get(new_inst.class.as_str()); + let mut new_properties = BTreeMap::new(); + let mut new_attributes = BTreeMap::new(); + + for (name, value) in properties { + match value { + Variant::Attributes(attrs) => { + for (attr_name, attr_value) in attrs.iter() { + // We (probably) don't want to preserve internal attributes, + // only user defined ones. + if attr_name.starts_with("RBX") { + continue; + } + new_attributes.insert( + attr_name.clone(), + UnresolvedValue::from_variant_unambiguous(attr_value.clone()), + ); + } + } + Variant::SharedString(_) => { + log::warn!( + "Rojo cannot serialize the property {}.{name} in JSON files.\n\ + If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", + new_inst.class, snapshot.get_new_inst_path(new_inst.referent()) + ) + } + _ => { + let new_prop_is_default = if let Some(class_descriptor) = class_descriptor { + if let Some(default) = db.find_default_property(class_descriptor, name) { + variant_eq(value, default) + } else { + false + } + } else { + false + }; + + if !new_prop_is_default { + new_properties.insert( + name.to_owned(), + UnresolvedValue::from_variant(value.clone(), &new_inst.class, name), + ); + } + } + } + } + + (new_properties, new_attributes) +} + +fn node_should_reserialize( + node_properties: &BTreeMap, + node_attributes: &BTreeMap, + instance: InstanceWithMeta, +) -> anyhow::Result { + for (prop_name, unresolved_node_value) in node_properties { + if let Some(inst_value) = instance.properties().get(prop_name) { + let node_value = unresolved_node_value + .clone() + .resolve(instance.class_name(), prop_name)?; + if !variant_eq(inst_value, &node_value) { + return Ok(true); + } + } else { + return Ok(true); + } + } + + // At this point, we know that the old instance contains at least all the + // properties specified by the new node, and that none of the properties + // specified by the new node differ from their old values. + // + // Because node properties at default values are represented by omission, we + // still need to determine whether any properties missing from the new node + // are at non-default values on the old instance. Otherwise, we will fail to + // reserialize the node when properties change from non-default values to + // default values. + let db = rbx_reflection_database::get(); + let maybe_class_descriptor = db.classes.get(instance.class_name()); + for (prop_name, inst_value) in instance.properties() { + // We skip attributes because they are compared later in this function. + if prop_name == "Attributes" { + continue; + } + + // If the new node contains this property, then further checks are + // unnecessary, as we've already compared such properties in the + // previous loop. + if node_properties.contains_key(prop_name) { + continue; + } + + // If a class descriptor cannot be found, then the reflection database + // might be out of date. + // + // We should always reserialize the node in this case, otherwise we may + // fail to reproduce properties of classes unknown to the reflection + // database. + let Some(class_descriptor) = maybe_class_descriptor else { + return Ok(true); + }; + + // If a default value for this property cannot be found, then the + // reflection database might be out of date, or this property simply + // does not have a default value. + // + // We should always reserialize the node in this case, otherwise we may + // fail to reproduce properties that do not have defaults in the + // reflection database. + let Some(default_value) = db.find_default_property(class_descriptor, prop_name) else { + return Ok(true); + }; + + // If the old value for this property is non-default, and the new node does + // not specify this property, it means that its value has changed to the + // default, and the new node must be reserialized. + if !variant_eq(inst_value, default_value) { + return Ok(true); + } + } + + match instance.properties().get("Attributes") { + Some(Variant::Attributes(inst_attributes)) => { + // This will also catch if one is empty but the other isn't + if node_attributes.len() != inst_attributes.len() { + Ok(true) + } else { + for (attr_name, unresolved_node_value) in node_attributes { + if let Some(inst_value) = inst_attributes.get(attr_name.as_str()) { + let node_value = unresolved_node_value.clone().resolve_unambiguous()?; + if !variant_eq(inst_value, &node_value) { + return Ok(true); + } + } else { + return Ok(true); + } + } + Ok(false) + } + } + Some(_) => Ok(true), + None => { + if !node_attributes.is_empty() { + Ok(true) + } else { + Ok(false) + } + } + } +} diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 0cd0e3599..bf89d79e9 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -1,6 +1,6 @@ use std::{ borrow::Cow, - collections::{BTreeMap, HashMap, VecDeque}, + collections::{HashMap, VecDeque}, path::Path, }; @@ -14,18 +14,16 @@ use rbx_reflection::ClassTag; use crate::{ project::{PathNode, Project, ProjectNode}, - resolution::UnresolvedValue, snapshot::{ - InstanceContext, InstanceMetadata, InstanceSnapshot, InstanceWithMeta, InstigatingSource, - PathIgnoreRule, SyncRule, + InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource, PathIgnoreRule, + SyncRule, }, - snapshot_middleware::Middleware, + snapshot_middleware::{node_should_reserialize, Middleware}, syncback::{filter_properties, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, - variant_eq::variant_eq, RojoRef, }; -use super::{emit_legacy_scripts_default, snapshot_from_vfs}; +use super::{emit_legacy_scripts_default, populate_unresolved_properties, snapshot_from_vfs}; pub fn snapshot_project( context: &InstanceContext, @@ -512,7 +510,7 @@ pub fn syncback_project<'sync>( let mut fs_snapshot = FsSnapshot::new(); for (node_properties, node_attributes, old_inst) in node_changed_map { - if project_node_should_reserialize(node_properties, node_attributes, old_inst)? { + if node_should_reserialize(node_properties, node_attributes, old_inst)? { fs_snapshot.add_file(project_path, serde_json::to_vec_pretty(&project)?); break; } @@ -531,37 +529,10 @@ fn project_node_property_syncback<'inst>( new_inst: &Instance, node: &mut ProjectNode, ) { - let properties = &mut node.properties; - let mut attributes = BTreeMap::new(); - for (name, value) in filtered_properties { - match value { - Variant::Attributes(attrs) => { - for (attr_name, attr_value) in attrs.iter() { - // We (probably) don't want to preserve internal attributes, - // only user defined ones. - if attr_name.starts_with("RBX") { - continue; - } - attributes.insert( - attr_name.clone(), - UnresolvedValue::from_variant_unambiguous(attr_value.clone()), - ); - } - } - Variant::SharedString(_) => { - log::warn!( - "Rojo cannot serialize the property {}.{name} in project files.\n\ - If this is not acceptable, resave the Instance at '{}' manually as an RBXM or RBXMX.", new_inst.class, snapshot.get_new_inst_path(new_inst.referent()) - ); - } - _ => { - properties.insert( - name.to_string(), - UnresolvedValue::from_variant(value.clone(), &new_inst.class, name), - ); - } - } - } + let (properties, attributes) = + populate_unresolved_properties(snapshot, new_inst, filtered_properties); + + node.properties = properties; node.attributes = attributes; } @@ -585,54 +556,6 @@ fn project_node_property_syncback_no_path( project_node_property_syncback(snapshot, filtered_properties, new_inst, node) } -fn project_node_should_reserialize( - node_properties: &BTreeMap, - node_attributes: &BTreeMap, - instance: InstanceWithMeta, -) -> anyhow::Result { - for (prop_name, unresolved_node_value) in node_properties { - if let Some(inst_value) = instance.properties().get(prop_name) { - let node_value = unresolved_node_value - .clone() - .resolve(instance.class_name(), prop_name)?; - if !variant_eq(inst_value, &node_value) { - return Ok(true); - } - } else { - return Ok(true); - } - } - - match instance.properties().get("Attributes") { - Some(Variant::Attributes(inst_attributes)) => { - // This will also catch if one is empty but the other isn't - if node_attributes.len() != inst_attributes.len() { - Ok(true) - } else { - for (attr_name, unresolved_node_value) in node_attributes { - if let Some(inst_value) = inst_attributes.get(attr_name.as_str()) { - let node_value = unresolved_node_value.clone().resolve_unambiguous()?; - if !variant_eq(inst_value, &node_value) { - return Ok(true); - } - } else { - return Ok(true); - } - } - Ok(false) - } - } - Some(_) => Ok(true), - None => { - if !node_attributes.is_empty() { - Ok(true) - } else { - Ok(false) - } - } - } -} - fn infer_class_name(name: &str, parent_class: Option<&str>) -> Option> { // If className wasn't defined from another source, we may be able // to infer one. diff --git a/src/syncback/property_filter.rs b/src/syncback/property_filter.rs index fdbcd1dad..a07776185 100644 --- a/src/syncback/property_filter.rs +++ b/src/syncback/property_filter.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use rbx_dom_weak::{types::Variant, Instance}; use rbx_reflection::{PropertyKind, PropertySerialization, Scriptability}; -use crate::{variant_eq::variant_eq, Project}; +use crate::Project; /// Returns a map of properties from `inst` that are both allowed under the /// user-provided settings, are not their default value, and serialize. @@ -54,27 +54,11 @@ pub fn filter_properties_preallocated<'inst>( false }; - if let Some(class_data) = class_data { - let defaults = &class_data.default_properties; - for (name, value) in &inst.properties { - if predicate(name, value) { - continue; - } - if let Some(default) = defaults.get(name.as_str()) { - if !variant_eq(value, default) { - allocation.push((name, value)); - } - } else { - allocation.push((name, value)); - } - } - } else { - for (name, value) in &inst.properties { - if predicate(name, value) { - continue; - } - allocation.push((name, value)); + for (name, value) in &inst.properties { + if predicate(name, value) { + continue; } + allocation.push((name, value)); } } diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 1d9e937c0..7a816ce52 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -32,4 +32,6 @@ syncback_basic_test! { ignore_trees, ignore_paths_removing, ignore_trees_removing, + remove_default_props, + json_models, }