diff --git a/CHANGELOG.md b/CHANGELOG.md index cff39688..00524693 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Making a new release? Simply add the new header with the version and date undern * Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) * Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267]) * `rojo serve` now validates the `Host`/`Origin` headers to protect the local/private server against DNS rebinding, gates `/api/open` to local clients, and warns when bound to a network-reachable address. The accepted hosts can be extended with the `--allowed-hosts` option or a project's `serveAllowedHosts` field, for example to reach a network-exposed server by hostname. ([#1270]) +* Fixed syncback not removing stale `$properties` entries when Studio resets a property to its engine default. ([#1244]) [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 @@ -60,6 +61,7 @@ Making a new release? Simply add the new header with the version and date undern [#1266]: https://github.com/rojo-rbx/rojo/pull/1266 [#1267]: https://github.com/rojo-rbx/rojo/pull/1267 [#1270]: https://github.com/rojo-rbx/rojo/pull/1270 +[#1244]: https://github.com/rojo-rbx/rojo/pull/1244 ## [7.7.0-rc.1] (November 27th, 2025) diff --git a/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap new file mode 100644 index 00000000..3ef76951 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__rojo_test__syncback_util__project_default_properties_remove-stdout.snap @@ -0,0 +1,5 @@ +--- +source: tests/rojo_test/syncback_util.rs +expression: "String::from_utf8_lossy(&output.stdout)" +--- +Writing default.project.json diff --git a/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap new file mode 100644 index 00000000..1b8d07d2 --- /dev/null +++ b/rojo-test/syncback-test-snapshots/end_to_end__tests__syncback__project_default_properties_remove-default.project.json.snap @@ -0,0 +1,59 @@ +--- +source: tests/tests/syncback.rs +expression: default.project.json +--- +{ + "name": "SyncbackTest", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "TestPart": { + "$className": "Part", + "$properties": { + "Anchored": true, + "Color": { + "Color3uint8": [ + 0, + 0, + 255 + ] + } + } + }, + "$properties": { + "EnableSLIMAvatars": { + "Enum": 0 + }, + "ImprovedAnimationConstraint": { + "Enum": 0 + }, + "ImprovedPhysicsReplication": { + "Enum": 0 + }, + "LayeredClothingCacheOptimizations": { + "Enum": 0 + }, + "MeshStreamingAndImprovedLods": { + "Enum": 0 + }, + "NextGenerationReplication": { + "Enum": 0 + }, + "PlayerScriptsUseInputActionSystem": { + "Enum": 0 + }, + "UseFixedSimulation": { + "Enum": 0 + }, + "UseNewLuauTypeSolver": "Disabled", + "ValidateEnabledProximityPrompt": { + "Enum": 0 + } + }, + "$attributes": { + "Rojo_Target_CurrentCamera": "302d573157260ee80a3baa32000003b5" + } + } + } +} diff --git a/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json b/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json new file mode 100644 index 00000000..6f0b1ade --- /dev/null +++ b/rojo-test/syncback-tests/project_default_properties_remove/input-project/default.project.json @@ -0,0 +1,21 @@ +{ + "name": "SyncbackTest", + "tree": { + "$className": "DataModel", + "Workspace": { + "$className": "Workspace", + "TestPart": { + "$className": "Part", + "$properties": { + "Transparency": 1.0, + "Anchored": true, + "Color": [ + 1.0, + 0.0, + 0.0 + ] + } + } + } + } +} \ No newline at end of file diff --git a/rojo-test/syncback-tests/project_default_properties_remove/input.rbxl b/rojo-test/syncback-tests/project_default_properties_remove/input.rbxl new file mode 100644 index 00000000..7ca3700a Binary files /dev/null and b/rojo-test/syncback-tests/project_default_properties_remove/input.rbxl differ diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index c114ac5d..09d286de 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -344,6 +344,11 @@ pub fn syncback_project<'sync>( let mut new_child_map = HashMap::new(); let mut node_changed_map = Vec::new(); + // Tracks whether any stale default-valued properties were removed from + // project nodes. If so, we must reserialize even if + // project_node_should_reserialize wouldn't otherwise detect a change + // (it only compares node properties forward, not in reverse). + let mut removed_stale_properties = false; let mut node_queue = VecDeque::with_capacity(1); node_queue.push_back((&mut project.tree, old_inst, snapshot.new_inst())); @@ -402,10 +407,12 @@ pub fn syncback_project<'sync>( // We only want to set properties if it needs it. if !middleware.handles_own_properties() { - project_node_property_syncback_path(snapshot, new_inst, node); + removed_stale_properties |= + project_node_property_syncback_path(snapshot, new_inst, node); } } else { - project_node_property_syncback_no_path(snapshot, new_inst, node); + removed_stale_properties |= + project_node_property_syncback_no_path(snapshot, new_inst, node); } for child_ref in new_inst.children() { @@ -507,12 +514,18 @@ 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)? { - fs_snapshot.add_file(project_path, serde_json::to_vec_pretty(&project)?); - break; + let mut needs_reserialize = removed_stale_properties; + if !needs_reserialize { + for (node_properties, node_attributes, old_inst) in node_changed_map { + if project_node_should_reserialize(node_properties, node_attributes, old_inst)? { + needs_reserialize = true; + break; + } } } + if needs_reserialize { + fs_snapshot.add_file(project_path, serde_json::to_vec_pretty(&project)?); + } Ok(SyncbackReturn { fs_snapshot, @@ -521,15 +534,18 @@ pub fn syncback_project<'sync>( }) } +/// Syncs properties from the new instance into the project node. +/// Returns `true` if any stale properties were removed (i.e. properties +/// that existed in the project node but are now at their engine default). fn project_node_property_syncback( _snapshot: &SyncbackSnapshot, filtered_properties: UstrMap<&Variant>, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let properties = &mut node.properties; let mut attributes = BTreeMap::new(); - for (name, value) in filtered_properties { + for (&name, &value) in &filtered_properties { match value { Variant::Attributes(attrs) => { for (attr_name, attr_value) in attrs.iter() { @@ -552,14 +568,48 @@ fn project_node_property_syncback( } } } + + // Remove stale properties: entries that exist in the project node's + // $properties but are no longer in the filtered (non-default) properties + // from the instance. This handles the case where Studio resets a property + // to its engine default — filter_properties won't include it, so we need + // to clean up the now-stale project entry. + let class_data = rbx_reflection_database::get() + .ok() + .and_then(|db| db.classes.get(new_inst.class.as_str())); + let len_before = properties.len(); + properties.retain(|prop_name, _| { + if filtered_properties.contains_key(prop_name) { + return true; + } + // Only remove if the property has a known default value in the + // reflection database. If there's no default, the property might be + // absent from the instance for other reasons (e.g. unknown property), + // so we conservatively keep it. + if let Some(data) = &class_data { + if data.default_properties.contains_key(prop_name.as_str()) { + log::debug!( + "Removing stale property '{}' from project node for class '{}': \ + value has been reset to engine default", + prop_name, + new_inst.class + ); + return false; + } + } + true + }); + let removed_stale = properties.len() < len_before; + node.attributes = attributes; + removed_stale } fn project_node_property_syncback_path( snapshot: &SyncbackSnapshot, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let filtered_properties = snapshot .get_path_filtered_properties(new_inst.referent()) .unwrap(); @@ -570,7 +620,7 @@ fn project_node_property_syncback_no_path( snapshot: &SyncbackSnapshot, new_inst: &Instance, node: &mut ProjectNode, -) { +) -> bool { let filtered_properties = filter_properties(snapshot.project(), new_inst); project_node_property_syncback(snapshot, filtered_properties, new_inst, node) } diff --git a/tests/tests/syncback.rs b/tests/tests/syncback.rs index 31553496..cffccccb 100644 --- a/tests/tests/syncback.rs +++ b/tests/tests/syncback.rs @@ -86,4 +86,7 @@ syncback_tests! { sync_rules => ["src/module.modulescript", "src/text.text"], // Ensures that the `syncUnscriptable` setting works unscriptable_properties => ["default.project.json"], + // Ensures that syncback correctly removes default values from projects rather + // than leaving an incorrect value. + project_default_properties_remove => ["default.project.json"], }