From 9bf3a78b22e7cd38a395a9f90355124613c6ecbc Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 10:54:39 -0700 Subject: [PATCH 01/10] Update test files to use .luau instead of .lua --- ..._to_end__rojo_test__syncback_util__all_middleware.snap | 8 +++----- ..._rojo_test__syncback_util__project_all_middleware.snap | 8 +++----- ...nd_to_end__rojo_test__syncback_util__project_init.snap | 4 +--- .../{init.client.lua => init.client.luau} | 0 .../src/dir/init_module_script/{init.lua => init.luau} | 0 .../{init.server.lua => init.server.luau} | 0 .../{init.client.lua => init.client.luau} | 0 .../src/init_module_script/{init.lua => init.luau} | 0 .../{init.server.lua => init.server.luau} | 0 .../project_init/expected/src/{init.lua => init.luau} | 0 10 files changed, 7 insertions(+), 13 deletions(-) rename rojo-test/syncback-tests/all_middleware/expected/src/dir/init_client_script/{init.client.lua => init.client.luau} (100%) rename rojo-test/syncback-tests/all_middleware/expected/src/dir/init_module_script/{init.lua => init.luau} (100%) rename rojo-test/syncback-tests/all_middleware/expected/src/dir/init_server_script/{init.server.lua => init.server.luau} (100%) rename rojo-test/syncback-tests/project_all_middleware/expected/src/init_client_script/{init.client.lua => init.client.luau} (100%) rename rojo-test/syncback-tests/project_all_middleware/expected/src/init_module_script/{init.lua => init.luau} (100%) rename rojo-test/syncback-tests/project_all_middleware/expected/src/init_server_script/{init.server.lua => init.server.luau} (100%) rename rojo-test/syncback-tests/project_init/expected/src/{init.lua => init.luau} (100%) 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 5b10b4aec..f4d242a62 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 @@ -1,15 +1,14 @@ --- source: tests/rojo_test/syncback_util.rs -assertion_line: 48 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" --- added_files: - "src\\csv.csv" - "src\\csv_init\\init.csv" - "src\\dir\\client_script.client.luau" - - "src\\dir\\init_client_script\\init.client.lua" - - "src\\dir\\init_module_script\\init.lua" - - "src\\dir\\init_server_script\\init.server.lua" + - "src\\dir\\init_client_script\\init.client.luau" + - "src\\dir\\init_module_script\\init.luau" + - "src\\dir\\init_server_script\\init.server.luau" - "src\\dir\\module_script.luau" - "src\\dir\\server_script.server.luau" - "src\\model_json.model.json" @@ -26,4 +25,3 @@ added_dirs: - "src\\dir\\init_server_script" 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 119ed2573..6757cc9a1 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 @@ -1,15 +1,14 @@ --- source: tests/rojo_test/syncback_util.rs -assertion_line: 48 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" --- added_files: - src/client_script.client.luau - src/csv.csv - "src/csv_init\\init.csv" - - "src/init_client_script\\init.client.lua" - - "src/init_module_script\\init.lua" - - "src/init_server_script\\init.server.lua" + - "src/init_client_script\\init.client.luau" + - "src/init_module_script\\init.luau" + - "src/init_server_script\\init.server.luau" - src/model_json.model.json - src/module_script.luau - src/project_json.project.json @@ -23,4 +22,3 @@ added_dirs: - src/init_server_script removed_files: [] removed_dirs: [] - diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_init.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_init.snap index dc907e7a1..5eb051109 100644 --- a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_init.snap +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_init.snap @@ -1,12 +1,10 @@ --- source: tests/rojo_test/syncback_util.rs -assertion_line: 48 expression: "visualize_fs_snapshot(&fs_snapshot, &output_path)" --- added_files: - - "src\\init.lua" + - "src\\init.luau" added_dirs: - src removed_files: [] removed_dirs: [] - diff --git a/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_client_script/init.client.lua b/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_client_script/init.client.luau similarity index 100% rename from rojo-test/syncback-tests/all_middleware/expected/src/dir/init_client_script/init.client.lua rename to rojo-test/syncback-tests/all_middleware/expected/src/dir/init_client_script/init.client.luau diff --git a/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_module_script/init.lua b/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_module_script/init.luau similarity index 100% rename from rojo-test/syncback-tests/all_middleware/expected/src/dir/init_module_script/init.lua rename to rojo-test/syncback-tests/all_middleware/expected/src/dir/init_module_script/init.luau diff --git a/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_server_script/init.server.lua b/rojo-test/syncback-tests/all_middleware/expected/src/dir/init_server_script/init.server.luau similarity index 100% rename from rojo-test/syncback-tests/all_middleware/expected/src/dir/init_server_script/init.server.lua rename to rojo-test/syncback-tests/all_middleware/expected/src/dir/init_server_script/init.server.luau diff --git a/rojo-test/syncback-tests/project_all_middleware/expected/src/init_client_script/init.client.lua b/rojo-test/syncback-tests/project_all_middleware/expected/src/init_client_script/init.client.luau similarity index 100% rename from rojo-test/syncback-tests/project_all_middleware/expected/src/init_client_script/init.client.lua rename to rojo-test/syncback-tests/project_all_middleware/expected/src/init_client_script/init.client.luau diff --git a/rojo-test/syncback-tests/project_all_middleware/expected/src/init_module_script/init.lua b/rojo-test/syncback-tests/project_all_middleware/expected/src/init_module_script/init.luau similarity index 100% rename from rojo-test/syncback-tests/project_all_middleware/expected/src/init_module_script/init.lua rename to rojo-test/syncback-tests/project_all_middleware/expected/src/init_module_script/init.luau diff --git a/rojo-test/syncback-tests/project_all_middleware/expected/src/init_server_script/init.server.lua b/rojo-test/syncback-tests/project_all_middleware/expected/src/init_server_script/init.server.luau similarity index 100% rename from rojo-test/syncback-tests/project_all_middleware/expected/src/init_server_script/init.server.lua rename to rojo-test/syncback-tests/project_all_middleware/expected/src/init_server_script/init.server.luau diff --git a/rojo-test/syncback-tests/project_init/expected/src/init.lua b/rojo-test/syncback-tests/project_init/expected/src/init.luau similarity index 100% rename from rojo-test/syncback-tests/project_init/expected/src/init.lua rename to rojo-test/syncback-tests/project_init/expected/src/init.luau From 32753ab6cd7628cb1f373074e84a26af81d4c8b7 Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 11:02:53 -0700 Subject: [PATCH 02/10] Correct typos in a few comments --- src/snapshot_middleware/mod.rs | 2 +- src/syncback/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/snapshot_middleware/mod.rs b/src/snapshot_middleware/mod.rs index 029f4f305..f6b593bbc 100644 --- a/src/snapshot_middleware/mod.rs +++ b/src/snapshot_middleware/mod.rs @@ -289,7 +289,7 @@ impl Middleware { /// set properties without needing a meta.json file. /// /// It does not cover middleware like `ServerScript` or `Csv` because they - /// need a meta.json file set properties that aren't their designated + /// need a meta.json file to set properties that aren't their designated /// 'special' properties. #[inline] pub fn handles_own_properties(&self) -> bool { diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 09b9c12a0..bf93bd400 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -36,7 +36,7 @@ pub use snapshot::{SyncbackData, SyncbackSnapshot}; /// The name of an enviroment variable to use to override the behavior of /// syncback on model files. -/// By default, syncabck will use `Rbxm` for model files. +/// By default, syncback will use `Rbxm` for model files. /// If this is set to `1`, it will instead use `Rbxmx`. If it is set to `2`, /// it will use `JsonModel`. /// From d1f5a090fdf59d2f4bf06d50c898e17e26afacf3 Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 11:03:01 -0700 Subject: [PATCH 03/10] Remove comment I forgot I left --- src/syncback/ref_properties.rs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 26544d9b8..955072668 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -193,16 +193,3 @@ fn get_existing_id(inst: &Instance) -> Option<&str> { None } } - -/* -When loading IDs we need to create a list and if there's a collision, -cause it to have a stroke. If that works to catch the duplicates then we just -need to re-serialize the IDs that collide. I think it's probably acceptable to -just pick one of the two if we can't tell the difference between them and give -the other a new ID. Maybe we emit a warning. - -This is gonna require a redo of the linking a bit, since it'll mean that we -can't just blindly use the old IDs. The plus side is that it means we can -collect these in a way that isn't awful via mapping an ID to a referent, so -it may end up improving the overall quality of the above code. -*/ From 444cf37c90c31c933834d1c7b64574b8f9c63f4b Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 11:05:00 -0700 Subject: [PATCH 04/10] Cave and finally include UniqueId in the all middleware tests --- rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx | 1 + .../project_all_middleware/expected/src/rbxmx.rbxmx | 1 + 2 files changed, 2 insertions(+) diff --git a/rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx b/rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx index 6e43e0368..6c66a17ad 100644 --- a/rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx +++ b/rojo-test/syncback-tests/all_middleware/expected/src/rbxmx.rbxmx @@ -8,6 +8,7 @@ -1 fireplace addicted army cow stock + 56ddeac77c86b55f061dbf5000007cca \ No newline at end of file diff --git a/rojo-test/syncback-tests/project_all_middleware/expected/src/rbxmx.rbxmx b/rojo-test/syncback-tests/project_all_middleware/expected/src/rbxmx.rbxmx index 50a48b184..e609d6ba4 100644 --- a/rojo-test/syncback-tests/project_all_middleware/expected/src/rbxmx.rbxmx +++ b/rojo-test/syncback-tests/project_all_middleware/expected/src/rbxmx.rbxmx @@ -8,6 +8,7 @@ -1 ripe alike review heart dry + 288c54239308069206387a3a00004126 \ No newline at end of file From a3c78e460c55043bae63997120bbafca5942c4c1 Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 11:30:46 -0700 Subject: [PATCH 05/10] Remove superfluous instance snapshot creation from syncback --- src/snapshot/instance_snapshot.rs | 11 ----------- src/snapshot_middleware/csv.rs | 6 +----- src/snapshot_middleware/dir.rs | 1 - src/snapshot_middleware/json_model.rs | 1 - src/snapshot_middleware/lua.rs | 6 +----- src/snapshot_middleware/project.rs | 1 - src/snapshot_middleware/rbxm.rs | 1 - src/snapshot_middleware/rbxmx.rs | 1 - src/snapshot_middleware/txt.rs | 1 - src/syncback/mod.rs | 8 ++------ 10 files changed, 4 insertions(+), 33 deletions(-) diff --git a/src/snapshot/instance_snapshot.rs b/src/snapshot/instance_snapshot.rs index 5337e529f..0b3a4fca3 100644 --- a/src/snapshot/instance_snapshot.rs +++ b/src/snapshot/instance_snapshot.rs @@ -127,17 +127,6 @@ impl InstanceSnapshot { children, } } - - pub fn from_instance(instance: &Instance) -> Self { - Self { - snapshot_id: instance.referent(), - metadata: InstanceMetadata::new(), - name: Cow::Owned(instance.name.clone()), - class_name: Cow::Owned(instance.class.clone()), - properties: instance.properties.clone(), - children: Vec::new(), - } - } } impl Default for InstanceSnapshot { diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 8c0d5fa4d..290ec870e 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -125,7 +125,6 @@ pub fn syncback_csv<'sync>( } Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), fs_snapshot, children: Vec::new(), removed_children: Vec::new(), @@ -161,10 +160,7 @@ pub fn syncback_csv_init<'sync>( } } - Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), - ..dir_syncback - }) + Ok(dir_syncback) } /// Struct that holds any valid row from a Roblox CSV translation table. diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index e63ea5b4a..aaaa4fa38 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -202,7 +202,6 @@ pub fn syncback_dir_no_meta<'sync>( fs_snapshot.add_dir(&snapshot.path); Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), fs_snapshot, children, removed_children, diff --git a/src/snapshot_middleware/json_model.rs b/src/snapshot_middleware/json_model.rs index 8b479677e..25a6884de 100644 --- a/src/snapshot_middleware/json_model.rs +++ b/src/snapshot_middleware/json_model.rs @@ -74,7 +74,6 @@ pub fn syncback_json_model<'sync>( model.name = None; Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(snapshot.new_inst()), fs_snapshot: FsSnapshot::new().with_added_file( &snapshot.path, serde_json::to_vec_pretty(&model).context("failed to serialize new JSON Model")?, diff --git a/src/snapshot_middleware/lua.rs b/src/snapshot_middleware/lua.rs index b705ae628..3477b35cc 100644 --- a/src/snapshot_middleware/lua.rs +++ b/src/snapshot_middleware/lua.rs @@ -151,7 +151,6 @@ pub fn syncback_lua<'sync>( } Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), fs_snapshot, // Scripts don't have a child! children: Vec::new(), @@ -192,10 +191,7 @@ pub fn syncback_lua_init<'sync>( } } - Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), - ..dir_syncback - }) + Ok(dir_syncback) } #[cfg(test)] diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index b22da30dc..214340091 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -513,7 +513,6 @@ pub fn syncback_project<'sync>( } Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(snapshot.new_inst()), fs_snapshot, children: descendant_snapshots, removed_children: removed_descendants, diff --git a/src/snapshot_middleware/rbxm.rs b/src/snapshot_middleware/rbxm.rs index 75a53c717..f94c4f5ef 100644 --- a/src/snapshot_middleware/rbxm.rs +++ b/src/snapshot_middleware/rbxm.rs @@ -54,7 +54,6 @@ pub fn syncback_rbxm<'sync>( .context("failed to serialize new rbxm")?; Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(inst), fs_snapshot: FsSnapshot::new().with_added_file(&snapshot.path, serialized), children: Vec::new(), removed_children: Vec::new(), diff --git a/src/snapshot_middleware/rbxmx.rs b/src/snapshot_middleware/rbxmx.rs index 52551cd5e..0cc84d3e2 100644 --- a/src/snapshot_middleware/rbxmx.rs +++ b/src/snapshot_middleware/rbxmx.rs @@ -65,7 +65,6 @@ pub fn syncback_rbxmx<'sync>( .context("failed to serialize new rbxmx")?; Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(inst), fs_snapshot: FsSnapshot::new().with_added_file(&snapshot.path, serialized), children: Vec::new(), removed_children: Vec::new(), diff --git a/src/snapshot_middleware/txt.rs b/src/snapshot_middleware/txt.rs index 6eb0cf53e..27e70496b 100644 --- a/src/snapshot_middleware/txt.rs +++ b/src/snapshot_middleware/txt.rs @@ -75,7 +75,6 @@ pub fn syncback_txt<'sync>( } Ok(SyncbackReturn { - inst_snapshot: InstanceSnapshot::from_instance(new_inst), fs_snapshot, children: Vec::new(), removed_children: Vec::new(), diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index bf93bd400..5e85c4d39 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -21,7 +21,7 @@ use std::{ use crate::{ glob::Glob, - snapshot::{InstanceSnapshot, InstanceWithMeta, RojoTree}, + snapshot::{InstanceWithMeta, RojoTree}, snapshot_middleware::Middleware, syncback::ref_properties::link_referents, Project, @@ -137,7 +137,6 @@ pub fn syncback_loop( middleware: Some(Middleware::Project), }]; - let mut replacements = Vec::new(); let mut fs_snapshot = FsSnapshot::new(); 'syncback: while let Some(snapshot) = snapshots.pop() { @@ -228,9 +227,7 @@ pub fn syncback_loop( } } - if let Some(old_inst) = snapshot.old_inst() { - replacements.push((old_inst.parent(), syncback.inst_snapshot)); - } + // TODO provide replacement snapshots for e.g. two way sync fs_snapshot.merge(syncback.fs_snapshot); @@ -241,7 +238,6 @@ pub fn syncback_loop( } pub struct SyncbackReturn<'sync> { - pub inst_snapshot: InstanceSnapshot, pub fs_snapshot: FsSnapshot, pub children: Vec>, pub removed_children: Vec>, From 3602555dd26e47cbe27907d254d96ba1e3ed4f69 Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 16:17:55 -0700 Subject: [PATCH 06/10] Actually transfer children in model files correctly --- src/cli/syncback.rs | 88 ++++++++++++++++++++++++++++++--------------- 1 file changed, 59 insertions(+), 29 deletions(-) diff --git a/src/cli/syncback.rs b/src/cli/syncback.rs index 4ce557ffd..78bd0f7cc 100644 --- a/src/cli/syncback.rs +++ b/src/cli/syncback.rs @@ -79,6 +79,19 @@ impl SyncbackCommand { log::debug!("Old root: {}", dom_old.inner().root().class); log::debug!("New root: {}", dom_new.root().class); + if log::log_enabled!(log::Level::Trace) { + log::trace!("Children of old root:"); + for child in dom_old.inner().root().children() { + let inst = dom_old.get_instance(*child).unwrap(); + log::trace!("{} (class: {})", inst.name(), inst.class_name()); + } + log::trace!("Children of new root:"); + for child in dom_new.root().children() { + let inst = dom_new.get_by_ref(*child).unwrap(); + log::trace!("{} (class: {})", inst.name, inst.class); + } + } + let start = Instant::now(); log::info!("Beginning syncback..."); let snapshot = syncback_loop( @@ -137,49 +150,66 @@ impl SyncbackCommand { fn read_dom(path: &Path, file_kind: FileKind) -> anyhow::Result { let content = File::open(path)?; - Ok(match file_kind { + match file_kind { FileKind::Rbxl => { // HACK: A custom reflection database is used to deserialize UniqueId let db = binary_db().expect("A custom ReflectionDatabase to deserialize UniqueId"); let binary_decoder = rbx_binary::Deserializer::new().reflection_database(&db); - binary_decoder.deserialize(content)? + binary_decoder.deserialize(content).with_context(|| { + format!( + "Could not deserialize binary place file at {}", + path.display() + ) + }) } - FileKind::Rbxlx => rbx_xml::from_reader(content, xml_decode_config())?, + FileKind::Rbxlx => rbx_xml::from_reader(content, xml_decode_config()) + .with_context(|| format!("Could not deserialize XML place file at {}", path.display())), FileKind::Rbxm => { // HACK: A custom reflection database is used to deserialize UniqueId let db = binary_db().expect("A custom ReflectionDatabase to deserialize UniqueId"); let binary_decoder = rbx_binary::Deserializer::new().reflection_database(&db); - let temp_tree = binary_decoder.deserialize(content)?; - - let root_children = temp_tree.root().children(); - if root_children.len() != 1 { - anyhow::bail!( - "Rojo does not currently support models with more \ - than one Instance at the Root!" - ); - } - let real_root = temp_tree.get_by_ref(root_children[0]).unwrap(); - let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class)); - temp_tree.clone_multiple_into_external(real_root.children(), &mut new_tree); - - new_tree + let temp_tree = binary_decoder.deserialize(content).with_context(|| { + format!( + "Could not deserialize binary model file at {}", + path.display() + ) + })?; + + process_model_dom(temp_tree) } FileKind::Rbxmx => { - let temp_tree = rbx_xml::from_reader(content, xml_decode_config())?; - let root_children = temp_tree.root().children(); - if root_children.len() != 1 { - anyhow::bail!( - "Rojo does not currently support models with more \ - than one Instance at the Root!" - ); - } - let real_root = temp_tree.get_by_ref(root_children[0]).unwrap(); - let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class)); - temp_tree.clone_multiple_into_external(real_root.children(), &mut new_tree); + let temp_tree = + rbx_xml::from_reader(content, xml_decode_config()).with_context(|| { + format!("Could not deserialize XML model file at {}", path.display()) + })?; + process_model_dom(temp_tree) + } + } +} +fn process_model_dom(dom: WeakDom) -> anyhow::Result { + let temp_children = dom.root().children(); + if temp_children.len() == 1 { + let real_root = dom.get_by_ref(temp_children[0]).unwrap(); + let mut new_tree = WeakDom::new(InstanceBuilder::new(&real_root.class)); + for (name, property) in &real_root.properties { new_tree + .root_mut() + .properties + .insert(name.to_owned(), property.to_owned()); } - }) + + let children = dom.clone_multiple_into_external(real_root.children(), &mut new_tree); + for child in children { + new_tree.transfer_within(child, new_tree.root_ref()); + } + Ok(new_tree) + } else { + anyhow::bail!( + "Rojo does not currently support models with more \ + than one Instance at the Root!" + ); + } } fn xml_decode_config() -> rbx_xml::DecodeOptions<'static> { From 933973c1be204404e9cbc3183c095805c0d096cb Mon Sep 17 00:00:00 2001 From: Micah Date: Tue, 2 Jul 2024 16:40:12 -0700 Subject: [PATCH 07/10] Turn invariant into an error because it turns out it can just happen lol --- src/snapshot_middleware/project.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 214340091..a973d32bd 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -454,18 +454,19 @@ pub fn syncback_project<'sync>( } let new_equivalent = new_child_map.remove(child_name); let old_equivalent = old_child_map.remove(child_name.as_str()); - // The panic below should never happen. If it does, something's gone - // wrong with the Instance matching for nodes. match (new_equivalent, old_equivalent) { (Some(new), Some(old)) => node_queue.push_back((child_node, old, new)), (_, None) => anyhow::bail!( "The child '{child_name}' of Instance '{}' would be removed.\n\ - Syncback cannot add or remove Instances from project {}", old_inst.name(), project_path.display()), - (None, _) => panic!( - "Invariant violated: the Instance matching of project nodes is flawed somehow.\n\ - Specifically, a child named {} of the node {} did not exist in the old tree.", - child_name, old_inst.name() + Syncback cannot add or remove Instances from project {}", + old_inst.name(), + project_path.display() ), + (None, _) => anyhow::bail!( + "The child '{child_name}' of Instance '{}' is present only in a project file,\n\ + and not the provided file. Syncback cannot add or remove Instances from project:\n{}.", + old_inst.name(), project_path.display(), + ) } } From 8a890aa5395720a9d2544d908778e7d27575bbdf Mon Sep 17 00:00:00 2001 From: Micah Date: Mon, 8 Jul 2024 11:21:00 -0700 Subject: [PATCH 08/10] Revert changing CSV field to examples, add alias instead --- .../end_to_end__tests__build__csv_bug_145.snap | 2 +- .../end_to_end__tests__build__csv_in_folder.snap | 2 +- src/snapshot_middleware/csv.rs | 13 +++++++++---- ...napshot_middleware__csv__test__csv_from_vfs.snap | 2 +- ...apshot_middleware__csv__test__csv_with_meta.snap | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap index 4bb0f95a5..ab7470b31 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_bug_145.snap @@ -10,7 +10,7 @@ expression: contents normal - [{"key":"Count","examples":"A number demonstrating issue 145","source":"3","values":{"es":"7"}}] + [{"key":"Count","example":"A number demonstrating issue 145","source":"3","values":{"es":"7"}}] diff --git a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap index 42cbea1f5..d60fca025 100644 --- a/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap +++ b/rojo-test/build-test-snapshots/end_to_end__tests__build__csv_in_folder.snap @@ -10,7 +10,7 @@ expression: contents normal - [{"key":"Ack","examples":"An exclamation of despair","source":"Ack!","values":{"es":"¡Ay!"}}] + [{"key":"Ack","example":"An exclamation of despair","source":"Ack!","values":{"es":"¡Ay!"}}] diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 290ec870e..f1e2c833b 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -176,8 +176,13 @@ struct LocalizationEntry<'a> { #[serde(skip_serializing_if = "Option::is_none")] context: Option>, - #[serde(skip_serializing_if = "Option::is_none")] - examples: Option>, + // Roblox writes `examples` for LocalizationTable's Content property, which + // causes it to not roundtrip correctly. + // This is reported here: https://devforum.roblox.com/t/2908720. + // + // To support their mistake, we support an alias named `examples`. + #[serde(skip_serializing_if = "Option::is_none", alias = "examples")] + example: Option>, #[serde(skip_serializing_if = "Option::is_none")] source: Option>, @@ -220,7 +225,7 @@ fn convert_localization_csv(contents: &[u8]) -> Result { "Key" => entry.key = Some(Cow::Borrowed(value)), "Source" => entry.source = Some(Cow::Borrowed(value)), "Context" => entry.context = Some(Cow::Borrowed(value)), - "Example" => entry.examples = Some(Cow::Borrowed(value)), + "Example" => entry.example = Some(Cow::Borrowed(value)), _ => { entry .values @@ -274,7 +279,7 @@ fn localization_to_csv(csv_contents: &str) -> anyhow::Result> { record.push(entry.key.as_deref().unwrap_or_default()); record.push(entry.source.as_deref().unwrap_or_default()); record.push(entry.context.as_deref().unwrap_or_default()); - record.push(entry.examples.as_deref().unwrap_or_default()); + record.push(entry.example.as_deref().unwrap_or_default()); let values = &entry.values; for header in &extra_headers { diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap index 6ad72e733..df634792f 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_from_vfs.snap @@ -18,6 +18,6 @@ name: foo class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"examples\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: [] diff --git a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap index 389a8a588..8e97b5050 100644 --- a/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap +++ b/src/snapshot_middleware/snapshots/librojo__snapshot_middleware__csv__test__csv_with_meta.snap @@ -18,6 +18,6 @@ name: foo class_name: LocalizationTable properties: Contents: - String: "[{\"key\":\"Ack\",\"examples\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" + String: "[{\"key\":\"Ack\",\"example\":\"An exclamation of despair\",\"source\":\"Ack!\",\"values\":{\"es\":\"¡Ay!\"}}]" children: [] From 13af7090dec14d62f63911f3234997189c144653 Mon Sep 17 00:00:00 2001 From: Micah Date: Wed, 10 Jul 2024 11:56:50 -0700 Subject: [PATCH 09/10] Add some comments to syncback core loop --- src/syncback/mod.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 5e85c4d39..2945b0c18 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -59,12 +59,18 @@ pub fn syncback_loop( .map(|rules| rules.compile_globs()) .transpose()?; + // Strip out any objects from the new tree that aren't in the old tree. This + // is necessary so that hashing the roots of each tree won't always result + // in different hashes. Shout out to Roblox for serializing a bunch of + // Services nobody cares about. log::debug!("Pruning new tree"); strip_unknown_root_children(&mut new_tree, old_tree); log::debug!("Collecting referents for new DOM..."); let deferred_referents = collect_referents(&new_tree); + // Remove any properties that are manually blocked from syncback via the + // project file. log::debug!("Pre-filtering properties on DOMs"); for referent in descendants(&new_tree, new_tree.root_ref()) { let new_inst = new_tree.get_by_ref_mut(referent).unwrap(); @@ -83,6 +89,8 @@ pub fn syncback_loop( } } } + + // Handle removing the current camera. if let Some(syncback_rules) = &project.syncback_rules { if !syncback_rules.sync_current_camera.unwrap_or_default() { log::debug!("Removing CurrentCamera from new DOM"); @@ -90,7 +98,8 @@ pub fn syncback_loop( for child_ref in new_tree.root().children() { let inst = new_tree.get_by_ref(*child_ref).unwrap(); if inst.class == "Workspace" { - camera_ref = inst.properties.get("CurrentCamera") + camera_ref = inst.properties.get("CurrentCamera"); + break; } } if let Some(Variant::Ref(camera_ref)) = camera_ref { @@ -113,6 +122,9 @@ pub fn syncback_loop( log::debug!("Skipping referent linking as per project syncback rules"); } + // As with pruning the children of the new root, we need to ensure the roots + // for both DOMs have the same name otherwise their hashes will always be + // different. new_tree.root_mut().name.clone_from(&project.name); log::debug!("Hashing project DOM"); From 541fa3dd6298ec8a75dcb0cb459109918d119bdb Mon Sep 17 00:00:00 2001 From: Micah Date: Wed, 10 Jul 2024 11:57:04 -0700 Subject: [PATCH 10/10] Fix outdated comment in referent linking --- src/syncback/ref_properties.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index 955072668..e763f2e3c 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -28,8 +28,7 @@ struct RefLink { /// Iterates through a WeakDom and collects referent properties. /// -/// They can be linked to a dom later using the `link` method on the returned -/// struct. +/// They can be linked to a dom later using `link_referents`. pub fn collect_referents(dom: &WeakDom) -> RefLinks { let mut existing_ids = HashMap::new(); let mut need_rewrite = Vec::new();