diff --git a/.cursor/plans/fix_live_syncback_service_properties_d7c2c927.plan.md b/.cursor/plans/fix_live_syncback_service_properties_d7c2c927.plan.md new file mode 100644 index 00000000..43dce05b --- /dev/null +++ b/.cursor/plans/fix_live_syncback_service_properties_d7c2c927.plan.md @@ -0,0 +1,235 @@ +--- +name: Fix Live Syncback Service Properties +overview: Fix live syncback to preserve service-level properties and respect syncCurrentCamera by encoding service properties/attributes/tags/refs alongside the existing rbxm children data. +todos: + - id: plugin-encode-service + content: Create encodeService helper in plugin that returns className, data, properties, attributes, tags, refs + status: completed + - id: plugin-use-helper + content: Update performSyncback to use encodeService, remove children-only guard + status: completed + - id: server-service-chunk + content: Extend ServiceChunk struct with properties, attributes, tags, refs fields + status: completed + - id: server-apply-props + content: Update build_dom_from_chunks to apply service properties/attributes/tags and resolve refs + status: completed + - id: test-helper + content: Update make_service_chunk helpers and make_rbxl_from_chunks + status: completed + - id: fixture-update + content: Update live_syncback fixture project with service $path entries + status: completed + - id: roundtrip-update + content: Update roundtrip test to include service properties + status: completed + - id: new-tests + content: Add parity tests for service properties, childless services, and Camera removal + status: completed +isProject: false +--- + +# Fix Live Syncback Service Properties + +## Root Cause + +1. **Service properties lost** -- Plugin serializes `service:GetChildren()` only. Service-level properties never sent. +2. **Childless services skipped** -- `#service:GetChildren() > 0` guard skips services like VoiceChatService. +3. **Camera not removed** -- `syncback_loop` reads `Workspace.CurrentCamera` to find/remove Camera, but Workspace has no properties in the DOM. + +## Constraint + +`SerializationService:SerializeInstancesAsync` throws if services are in the top-level list. Children-only rbxm stays; service metadata must be encoded separately. + +## Changes + +### 1. Plugin -- New `encodeService` helper + +Create `[plugin/src/ChangeBatcher/encodeService.lua](plugin/src/ChangeBatcher/encodeService.lua)`. Returns a full `ServiceChunk` table: + +```lua +-- Returns: +-- { +-- className = "Lighting", +-- data = buffer, -- rbxm of children (empty buffer if no children) +-- properties = {}, -- non-Ref encoded properties (via RbxDom) +-- attributes = {}, -- encoded attributes map +-- tags = {}, -- tags array +-- refs = {}, -- Ref properties: { PropName = { name, className } } +-- } +``` + +Implementation reuses existing `encodeProperty`, `RbxDom.findClassDescriptor`, `RbxDom.findCanonicalPropertyDescriptor`, and the `UNENCODABLE_DATA_TYPES` filter from `[plugin/src/ChangeBatcher/propertyFilter.lua](plugin/src/ChangeBatcher/propertyFilter.lua)`. + +**Property encoding** -- same loop as `[encodeInstance.lua](plugin/src/ChangeBatcher/encodeInstance.lua)` lines 200-249 but without script-specific skips: + +```lua +for propertyName, propertyMeta in classDescriptor.properties do + -- skip Parent, Name, Archivable, Attributes, Tags + if isReadable and doesSerialize then + if descriptor.dataType == "Ref" then + -- Read ref target; if it's a direct child, record name+className + local ok, target = descriptor:read(service) + if ok and target and target.Parent == service then + refs[propertyName] = { name = target.Name, className = target.ClassName } + end + elseif not UNENCODABLE_DATA_TYPES[descriptor.dataType] then + local ok, encoded = encodeProperty(service, propertyName, descriptor) + if ok and encoded ~= nil then + properties[propertyName] = encoded + end + end + end +end +``` + +**Attributes** -- encoded via the Attributes property descriptor (same as `encodeAttributes` in `encodeInstance.lua` lines 121-137), but returned as a separate field. + +**Tags** -- read via `service:GetTags()`, returned as a plain array. + +**Refs** -- Ref properties where the target is a direct child of the service. Encoded as `{ name = target.Name, className = target.ClassName }` so the server can resolve them by finding the matching child after cloning from rbxm. + +**Children rbxm** -- `SerializationService:SerializeInstancesAsync(service:GetChildren())` if children exist, otherwise empty buffer. + +### 2. Plugin -- Update `performSyncback` in `[plugin/src/App/init.lua](plugin/src/App/init.lua)` (lines 641-651) + +```lua +local encodeService = require(Plugin.ChangeBatcher.encodeService) + +local services = {} +for _, className in SYNCBACK_SERVICES do + local ok, service = pcall(game.FindService, game, className) + if ok and service then + table.insert(services, encodeService(service)) + end +end +``` + +No children guard. Every found service is encoded. + +### 3. Server -- Extend `ServiceChunk` in `[src/web/interface.rs](src/web/interface.rs)` (lines 30-37) + +```rust +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ServiceChunk { + pub class_name: String, + #[serde(with = "serde_bytes")] + pub data: Vec, + #[serde(default)] + pub properties: HashMap, + #[serde(default)] + pub attributes: HashMap, + #[serde(default)] + pub tags: Vec, + #[serde(default)] + pub refs: HashMap, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ServiceRef { + pub name: String, + pub class_name: String, +} +``` + +All new fields use `#[serde(default)]` for backwards compatibility. + +### 4. Server -- Update `build_dom_from_chunks` in `[src/cli/serve.rs](src/cli/serve.rs)` (lines 140-179) + +After creating the service instance, apply properties/attributes/tags: + +```rust +for chunk in &payload.services { + let mut builder = InstanceBuilder::new(&chunk.class_name); + + // Apply encoded properties + for (key, value) in &chunk.properties { + builder = builder.with_property(key.as_str(), value.clone()); + } + + // Apply attributes + if !chunk.attributes.is_empty() { + let attrs = rbx_dom_weak::types::Attributes::new(); + // ... convert HashMap to Attributes ... + builder = builder.with_property("Attributes", Variant::Attributes(attrs)); + } + + // Apply tags + if !chunk.tags.is_empty() { + builder = builder.with_property("Tags", Variant::Tags(chunk.tags.clone().into())); + } + + let service_ref = dom.insert(root_ref, builder); + created_services.insert(chunk.class_name.clone()); + + // Clone children from rbxm + if !chunk.data.is_empty() { + let chunk_dom = rbx_binary::from_reader(Cursor::new(&chunk.data))?; + for &child_ref in chunk_dom.root().children() { + deep_clone_into(&chunk_dom, &mut dom, child_ref, service_ref, &mut global_ref_map); + } + } + + // Resolve Ref properties by finding matching children + if !chunk.refs.is_empty() { + let children: Vec = dom.get_by_ref(service_ref).unwrap().children().to_vec(); + for (prop_name, target) in &chunk.refs { + let found = children.iter().find(|&&child_ref| { + let child = dom.get_by_ref(child_ref).unwrap(); + child.name.as_str() == target.name && child.class.as_str() == target.class_name + }); + if let Some(&child_ref) = found { + let service = dom.get_by_ref_mut(service_ref).unwrap(); + service.properties.insert(prop_name.as_str().into(), Variant::Ref(child_ref)); + } + } + } +} +``` + +This handles `Workspace.CurrentCamera` naturally -- the Camera is cloned from the rbxm, then the Ref is resolved by matching `name="Camera", className="Camera"` among children. `syncback_loop` can then find it and destroy it when `syncCurrentCamera: false`. + +### 5. Test helpers -- `[tests/rojo_test/serve_util.rs](tests/rojo_test/serve_util.rs)` + +Update `make_service_chunk` to accept optional properties/attributes/tags/refs. Add companion: + +```rust +pub fn make_service_chunk_full( + class_name: &str, + properties: Vec<(&str, Variant)>, + attributes: Vec<(&str, Variant)>, + tags: Vec, + refs: Vec<(&str, &str, &str)>, // (prop_name, target_name, target_class) + children: Vec, +) -> ServiceChunk +``` + +Update `make_rbxl_from_chunks` to apply properties/attributes/tags/refs to the service in the DOM (mirrors server logic). + +Update `roundtrip_build_syncback_rebuild` to extract service properties from the built rbxl and populate `ServiceChunk` fields. + +### 6. Fixture -- `[rojo-test/serve-tests/live_syncback/default.project.json5](rojo-test/serve-tests/live_syncback/default.project.json5)` + +Add service entries with `$path` for Lighting, SoundService, StarterPlayer, TextChatService, VoiceChatService. Create empty `src/` subdirectories. + +### 7. New tests -- `[tests/tests/live_syncback.rs](tests/tests/live_syncback.rs)` + +**Service property parity tests** (use `assert_live_matches_cli`): + +- `**parity_lighting_properties`** -- Lighting with `Ambient`, `ClockTime`, `Brightness` + child +- `**parity_soundservice_properties`** -- SoundService with `AmbientReverb`, `DistanceFactor` +- `**parity_starterplayer_properties`** -- StarterPlayer with `CameraMaxZoomDistance` +- `**parity_textchatservice_properties**` -- TextChatService with `ChatVersion` +- `**parity_childless_service_with_properties**` -- Service with properties but zero children +- `**parity_service_properties_with_children**` -- Service with both properties AND children +- `**parity_multiple_services_with_properties**` -- Multiple services in one request + +**Camera removal test:** + +- `**parity_camera_not_synced`** -- Workspace with Camera child + `CurrentCamera` ref. Verify Camera does NOT appear on disk (default `syncCurrentCamera: false`). + +**Round-trip test:** + +- `**roundtrip_service_properties`** -- Build -> live syncback -> rebuild. Compare service properties survive the cycle. + diff --git a/plugin/src/App/init.lua b/plugin/src/App/init.lua index 047e6415..06a390e7 100644 --- a/plugin/src/App/init.lua +++ b/plugin/src/App/init.lua @@ -1,6 +1,5 @@ local ChangeHistoryService = game:GetService("ChangeHistoryService") local Players = game:GetService("Players") -local SerializationService = game:GetService("SerializationService") local ServerStorage = game:GetService("ServerStorage") local RunService = game:GetService("RunService") @@ -47,6 +46,8 @@ local Theme = require(script.Theme) local Http = require(Packages.Http) +local encodeService = require(Plugin.ChangeBatcher.encodeService) + local Page = require(script.Page) local Notifications = require(script.Components.Notifications) local TextButton = require(script.Components.TextButton) @@ -641,12 +642,8 @@ function App:performSyncback() local services = {} for _, className in SYNCBACK_SERVICES do local ok, service = pcall(game.FindService, game, className) - if ok and service and #service:GetChildren() > 0 then - local buf = SerializationService:SerializeInstancesAsync(service:GetChildren()) - table.insert(services, { - className = className, - data = buf, - }) + if ok and service then + table.insert(services, encodeService(service)) end end diff --git a/plugin/src/ChangeBatcher/encodeService.lua b/plugin/src/ChangeBatcher/encodeService.lua new file mode 100644 index 00000000..981269f2 --- /dev/null +++ b/plugin/src/ChangeBatcher/encodeService.lua @@ -0,0 +1,101 @@ +local SerializationService = game:GetService("SerializationService") + +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) +local RbxDom = require(Packages.RbxDom) + +local encodeProperty = require(script.Parent.encodeProperty) +local UNENCODABLE_DATA_TYPES = require(script.Parent.propertyFilter) + +local SKIP_PROPERTIES = { + Parent = true, + Name = true, + Archivable = true, +} + +return function(service: Instance) + local properties = {} + local attributes = {} + local tags = {} + local refs = {} + + local classDescriptor = RbxDom.findClassDescriptor(service.ClassName) + if classDescriptor then + for propertyName, propertyMeta in pairs(classDescriptor.properties) do + if SKIP_PROPERTIES[propertyName] then + continue + end + if propertyName == "Attributes" or propertyName == "Tags" then + continue + end + + local isReadable = propertyMeta.scriptability == "ReadWrite" or propertyMeta.scriptability == "Read" + local doesSerialize = propertyMeta.serialization ~= "DoesNotSerialize" + + if isReadable and doesSerialize then + local descriptor = RbxDom.findCanonicalPropertyDescriptor(service.ClassName, propertyName) + if descriptor then + if UNENCODABLE_DATA_TYPES[descriptor.dataType] then + continue + end + + if descriptor.dataType == "Ref" then + local readOk, target = descriptor:read(service) + if readOk and target and target.Parent == service then + refs[propertyName] = { + name = target.Name, + className = target.ClassName, + } + end + continue + end + + local encodeOk, encoded = encodeProperty(service, propertyName, descriptor) + if encodeOk and encoded ~= nil then + properties[propertyName] = encoded + end + end + end + end + end + + local attrOk, attrMap = pcall(function() + return service:GetAttributes() + end) + if attrOk and attrMap and next(attrMap) then + local descriptor = RbxDom.findCanonicalPropertyDescriptor(service.ClassName, "Attributes") + if descriptor then + local encodeOk, encoded = encodeProperty(service, "Attributes", descriptor) + if encodeOk and encoded ~= nil then + attributes = encoded + end + end + end + + local tagOk, tagList = pcall(function() + return service:GetTags() + end) + if tagOk and tagList and #tagList > 0 then + tags = tagList + end + + local children = service:GetChildren() + local data = buffer.create(0) + if #children > 0 then + local serializeOk, result = pcall(SerializationService.SerializeInstancesAsync, SerializationService, children) + if serializeOk then + data = result + else + Log.warn("Failed to serialize children of {}: {}", service.ClassName, tostring(result)) + end + end + + return { + className = service.ClassName, + data = data, + properties = properties, + attributes = attributes, + tags = tags, + refs = refs, + } +end diff --git a/rojo-test/serve-tests/live_syncback/default.project.json5 b/rojo-test/serve-tests/live_syncback/default.project.json5 index b96dcdeb..b9497483 100644 --- a/rojo-test/serve-tests/live_syncback/default.project.json5 +++ b/rojo-test/serve-tests/live_syncback/default.project.json5 @@ -13,6 +13,26 @@ "Workspace": { "$className": "Workspace", "$path": "src/workspace" + }, + "Lighting": { + "$className": "Lighting", + "$path": "src/lighting" + }, + "SoundService": { + "$className": "SoundService", + "$path": "src/soundservice" + }, + "StarterPlayer": { + "$className": "StarterPlayer", + "$path": "src/starterplayer" + }, + "TextChatService": { + "$className": "TextChatService", + "$path": "src/textchatservice" + }, + "VoiceChatService": { + "$className": "VoiceChatService", + "$path": "src/voicechatservice" } } } diff --git a/rojo-test/serve-tests/live_syncback/src/lighting/.gitkeep b/rojo-test/serve-tests/live_syncback/src/lighting/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/rojo-test/serve-tests/live_syncback/src/soundservice/.gitkeep b/rojo-test/serve-tests/live_syncback/src/soundservice/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/rojo-test/serve-tests/live_syncback/src/starterplayer/.gitkeep b/rojo-test/serve-tests/live_syncback/src/starterplayer/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/rojo-test/serve-tests/live_syncback/src/textchatservice/.gitkeep b/rojo-test/serve-tests/live_syncback/src/textchatservice/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/rojo-test/serve-tests/live_syncback/src/voicechatservice/.gitkeep b/rojo-test/serve-tests/live_syncback/src/voicechatservice/.gitkeep new file mode 100644 index 00000000..e69de29b diff --git a/src/cli/serve.rs b/src/cli/serve.rs index 01a6d365..b03e986d 100644 --- a/src/cli/serve.rs +++ b/src/cli/serve.rs @@ -139,6 +139,7 @@ fn run_live_syncback(project_path: &Path, payload: SyncbackPayload) -> anyhow::R fn build_dom_from_chunks(payload: SyncbackPayload) -> anyhow::Result { use crate::syncback::VISIBLE_SERVICES; + use rbx_dom_weak::types::{Attributes, Tags}; let mut dom = WeakDom::new(InstanceBuilder::new("DataModel")); let root_ref = dom.root_ref(); @@ -147,7 +148,26 @@ fn build_dom_from_chunks(payload: SyncbackPayload) -> anyhow::Result { let mut created_services: std::collections::HashSet = std::collections::HashSet::new(); for chunk in &payload.services { - let service_ref = dom.insert(root_ref, InstanceBuilder::new(&chunk.class_name)); + let mut builder = InstanceBuilder::new(&chunk.class_name); + + for (key, value) in &chunk.properties { + builder = builder.with_property(key.as_str(), value.clone()); + } + + if !chunk.attributes.is_empty() { + let mut attrs = Attributes::new(); + for (key, value) in &chunk.attributes { + attrs.insert(key.clone(), value.clone()); + } + builder = builder.with_property("Attributes", Variant::Attributes(attrs)); + } + + if !chunk.tags.is_empty() { + let tags: Tags = chunk.tags.iter().map(|s| s.as_str()).collect(); + builder = builder.with_property("Tags", Variant::Tags(tags)); + } + + let service_ref = dom.insert(root_ref, builder); created_services.insert(chunk.class_name.clone()); if !chunk.data.is_empty() { @@ -166,6 +186,22 @@ fn build_dom_from_chunks(payload: SyncbackPayload) -> anyhow::Result { ); } } + + if !chunk.refs.is_empty() { + let children: Vec = dom.get_by_ref(service_ref).unwrap().children().to_vec(); + for (prop_name, target) in &chunk.refs { + let found = children.iter().find(|&&child_ref| { + let child = dom.get_by_ref(child_ref).unwrap(); + child.name.as_str() == target.name && child.class.as_str() == target.class_name + }); + if let Some(&child_ref) = found { + let service = dom.get_by_ref_mut(service_ref).unwrap(); + service + .properties + .insert(prop_name.as_str().into(), Variant::Ref(child_ref)); + } + } + } } for &service_name in VISIBLE_SERVICES { diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs index 001c4f5b..7d41cc3a 100644 --- a/src/snapshot/matching.rs +++ b/src/snapshot/matching.rs @@ -14,6 +14,7 @@ use std::cell::RefCell; use std::collections::HashMap; use rbx_dom_weak::types::Ref; +use rbx_dom_weak::Ustr; use crate::variant_eq::variant_eq; @@ -55,9 +56,6 @@ pub struct ForwardMatchResult { pub unmatched_snapshot: Vec, /// Tree children with no match in the snapshot (to be removed). pub unmatched_tree: Vec, - /// Total cost of the matching (sum of all matched pair costs + - /// unmatched penalties). Zero means no changes across anything. - pub total_cost: u32, } /// Match snapshot children to tree children, minimizing total changes. @@ -72,34 +70,35 @@ pub fn match_forward( matched: Vec::new(), unmatched_snapshot: Vec::new(), unmatched_tree: Vec::new(), - total_cost: 0, }; } let snap_available: Vec> = snapshot_children.into_iter().map(Some).collect(); - let mut snap_matched: Vec = vec![false; snap_available.len()]; - let mut tree_available: Vec = vec![true; tree_children.len()]; - let mut matched: Vec<(usize, usize)> = Vec::new(); + let snap_len = snap_available.len(); + let tree_len = tree_children.len(); + let mut snap_matched: Vec = vec![false; snap_len]; + let mut tree_available: Vec = vec![true; tree_len]; + let mut matched: Vec<(usize, usize)> = Vec::with_capacity(snap_len.min(tree_len)); // ================================================================ // Fast-path: Group by (Name, ClassName) -- 1:1 groups instant-match // ================================================================ - let mut snap_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut snap_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(snap_len); for (i, snap_opt) in snap_available.iter().enumerate() { if let Some(snap) = snap_opt { snap_by_key - .entry((snap.name.to_string(), snap.class_name.to_string())) + .entry((snap.name.to_string(), snap.class_name)) .or_default() .push(i); } } - let mut tree_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut tree_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(tree_len); for (i, &child_ref) in tree_children.iter().enumerate() { if let Some(inst) = tree.get_instance(child_ref) { tree_by_key - .entry((inst.name().to_string(), inst.class_name().to_string())) + .entry((inst.name().to_string(), inst.class_name())) .or_default() .push(i); } @@ -122,14 +121,7 @@ pub fn match_forward( let snap_remaining = snap_matched.iter().filter(|&&m| !m).count(); let tree_remaining = tree_available.iter().filter(|&&a| a).count(); if snap_remaining == 0 || tree_remaining == 0 { - return build_result( - snap_available, - tree_children, - &tree_available, - matched, - tree, - session, - ); + return build_result(snap_available, tree_children, &tree_available, matched); } // ================================================================ @@ -168,7 +160,8 @@ pub fn match_forward( } // Score all (A, B) pairs using recursive change-count scoring - let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); + let mut pairs: Vec<(u32, usize, usize)> = + Vec::with_capacity(avail_snap.len() * avail_tree.len()); let mut best_so_far = u32::MAX; for &si in &avail_snap { let snap = match &snap_available[si] { @@ -199,38 +192,19 @@ pub fn match_forward( } } - build_result( - snap_available, - tree_children, - &tree_available, - matched, - tree, - session, - ) + build_result(snap_available, tree_children, &tree_available, matched) } /// Build the final result, consuming the snapshot children. -/// Computes `total_cost` for all matched pairs (using session cache). fn build_result( mut snap_available: Vec>, tree_children: &[Ref], tree_available: &[bool], matched_indices: Vec<(usize, usize)>, - tree: &RojoTree, - session: &MatchingSession, ) -> ForwardMatchResult { let mut matched = Vec::with_capacity(matched_indices.len()); - let mut total_cost: u32 = 0; for (si, ti) in matched_indices { if let Some(snap) = snap_available[si].take() { - total_cost = total_cost.saturating_add(compute_change_count( - &snap, - tree_children[ti], - tree, - u32::MAX, - 0, - session, - )); matched.push((snap, tree_children[ti])); } } @@ -244,15 +218,10 @@ fn build_result( .map(|(_, r)| *r) .collect(); - total_cost = total_cost.saturating_add( - (unmatched_snapshot.len() + unmatched_tree.len()) as u32 * UNMATCHED_PENALTY, - ); - ForwardMatchResult { matched, unmatched_snapshot, unmatched_tree, - total_cost, } } @@ -275,32 +244,34 @@ fn match_children_for_scoring( depth: u32, session: &MatchingSession, ) -> ScoringMatchResult { - let mut snap_matched = vec![false; snap_children.len()]; - let mut tree_matched = vec![false; tree_children.len()]; - let mut matched = Vec::new(); - if snap_children.is_empty() && tree_children.is_empty() { return ScoringMatchResult { - matched, + matched: Vec::new(), unmatched_snap: 0, unmatched_tree: 0, }; } + let snap_len = snap_children.len(); + let tree_len = tree_children.len(); + let mut snap_matched = vec![false; snap_len]; + let mut tree_matched = vec![false; tree_len]; + let mut matched: Vec<(usize, usize)> = Vec::with_capacity(snap_len.min(tree_len)); + // Group by (Name, ClassName) - let mut snap_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut snap_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(snap_len); for (i, snap) in snap_children.iter().enumerate() { snap_by_key - .entry((snap.name.to_string(), snap.class_name.to_string())) + .entry((snap.name.to_string(), snap.class_name)) .or_default() .push(i); } - let mut tree_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut tree_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(tree_len); for (i, &child_ref) in tree_children.iter().enumerate() { if let Some(inst) = tree.get_instance(child_ref) { tree_by_key - .entry((inst.name().to_string(), inst.class_name().to_string())) + .entry((inst.name().to_string(), inst.class_name())) .or_default() .push(i); } @@ -348,7 +319,8 @@ fn match_children_for_scoring( continue; } - let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); + let mut pairs: Vec<(u32, usize, usize)> = + Vec::with_capacity(avail_snap.len() * avail_tree.len()); let mut best_so_far = u32::MAX; for &si in &avail_snap { for &ti in &avail_tree { @@ -476,6 +448,7 @@ fn compute_change_count( /// Studio via two-way sync) has at their class defaults. To avoid inflating /// match costs with phantom diffs, properties present on only one side are /// skipped when their value matches the class default. +#[inline] fn count_own_diffs(snap: &InstanceSnapshot, inst: &InstanceWithMeta) -> u32 { let mut cost: u32 = 0; @@ -1309,88 +1282,6 @@ mod tests { ); } - #[test] - fn total_cost_zero_for_identical() { - let snaps = vec![ - make_snapshot("Alpha", "Folder"), - make_snapshot("Beta", "Folder"), - ]; - let (tree, children) = make_tree_with_children(&[("Alpha", "Folder"), ("Beta", "Folder")]); - let result = match_forward(snaps, &children, &tree, &MatchingSession::new()); - assert_eq!(result.matched.len(), 2); - assert_eq!( - result.total_cost, 0, - "Identical instances should have zero cost" - ); - } - - #[test] - fn total_cost_nonzero_for_different() { - use rbx_dom_weak::types::Variant; - - let snaps = vec![make_snapshot_with_props( - "Part", - "Part", - vec![("Transparency", Variant::Float32(0.5))], - )]; - let (tree, children) = make_tree_from_snapshots(vec![make_snapshot_with_props( - "Part", - "Part", - vec![("Transparency", Variant::Float32(0.0))], - )]); - let result = match_forward(snaps, &children, &tree, &MatchingSession::new()); - assert_eq!(result.matched.len(), 1); - assert!( - result.total_cost > 0, - "Different properties should produce nonzero cost, got {}", - result.total_cost - ); - } - - #[test] - fn total_cost_includes_unmatched() { - let snaps = vec![ - make_snapshot("A", "Folder"), - make_snapshot("B", "Folder"), - make_snapshot("C", "Folder"), - ]; - let (tree, children) = make_tree_with_children(&[("A", "Folder"), ("B", "Folder")]); - let result = match_forward(snaps, &children, &tree, &MatchingSession::new()); - assert_eq!(result.matched.len(), 2); - assert_eq!(result.unmatched_snapshot.len(), 1); - assert!( - result.total_cost >= UNMATCHED_PENALTY, - "Should include penalty for unmatched snapshot, got {}", - result.total_cost - ); - } - - #[test] - fn total_cost_sums_correctly() { - use rbx_dom_weak::types::Variant; - - let snaps = vec![ - make_snapshot("A", "Folder"), - make_snapshot_with_props("B", "Part", vec![("Transparency", Variant::Float32(0.5))]), - make_snapshot("C", "Folder"), - ]; - let tree_children_snaps = vec![ - make_snapshot("A", "Folder"), - make_snapshot_with_props("B", "Part", vec![("Transparency", Variant::Float32(0.0))]), - ]; - let (tree, children) = make_tree_from_snapshots(tree_children_snaps); - let result = match_forward(snaps, &children, &tree, &MatchingSession::new()); - - assert_eq!(result.matched.len(), 2); - assert_eq!(result.unmatched_snapshot.len(), 1); - // total = matched costs (A=0 + B>=1) + unmatched (C=10000) - assert!( - result.total_cost >= UNMATCHED_PENALTY + 1, - "Should be at least penalty + 1 diff, got {}", - result.total_cost - ); - } - #[test] fn session_cache_consistent() { let snaps1 = vec![make_snapshot("X", "Folder"), make_snapshot("Y", "Folder")]; @@ -1402,36 +1293,6 @@ mod tests { let r2 = match_forward(snaps2, &children, &tree, &session); assert_eq!(r1.matched.len(), r2.matched.len()); - assert_eq!(r1.total_cost, r2.total_cost); assert_eq!(r1.unmatched_snapshot.len(), r2.unmatched_snapshot.len()); } - - #[test] - fn session_cache_does_not_corrupt() { - use rbx_dom_weak::types::Variant; - - let session = MatchingSession::new(); - - // Group A: identical folders - let snaps_a = vec![make_snapshot("A", "Folder")]; - let (tree_a, children_a) = make_tree_with_children(&[("A", "Folder")]); - let ra = match_forward(snaps_a, &children_a, &tree_a, &session); - assert_eq!(ra.total_cost, 0); - - // Group B: parts with property diff - let snaps_b = vec![make_snapshot_with_props( - "B", - "Part", - vec![("Transparency", Variant::Float32(1.0))], - )]; - let (tree_b, children_b) = make_tree_from_snapshots(vec![make_snapshot_with_props( - "B", - "Part", - vec![("Transparency", Variant::Float32(0.0))], - )]); - let rb = match_forward(snaps_b, &children_b, &tree_b, &session); - - assert_eq!(ra.total_cost, 0, "Group A should still be zero"); - assert!(rb.total_cost > 0, "Group B should have nonzero cost"); - } } diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs index 5841550d..2bdee937 100644 --- a/src/syncback/matching.rs +++ b/src/syncback/matching.rs @@ -12,10 +12,11 @@ //! to turn instance A into instance B. use std::cell::RefCell; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use blake3::Hash; -use rbx_dom_weak::{types::Ref, WeakDom}; +use rbx_dom_weak::types::Ref; +use rbx_dom_weak::{Ustr, WeakDom}; use crate::variant_eq::variant_eq; @@ -50,7 +51,6 @@ pub struct MatchResult { pub matched: Vec<(Ref, Ref)>, pub unmatched_new: Vec, pub unmatched_old: Vec, - pub total_cost: u32, } /// Match new children to old children, minimizing total changes. @@ -68,106 +68,80 @@ pub fn match_children( matched: Vec::new(), unmatched_new: Vec::new(), unmatched_old: Vec::new(), - total_cost: 0, }; } - let mut matched: Vec<(Ref, Ref)> = Vec::new(); - let mut remaining_new: Vec = new_children.to_vec(); - let mut remaining_old: Vec = old_children.to_vec(); + let new_len = new_children.len(); + let old_len = old_children.len(); + let mut new_matched = vec![false; new_len]; + let mut old_matched = vec![false; old_len]; + let mut matched: Vec<(usize, usize)> = Vec::with_capacity(new_len.min(old_len)); // ================================================================ // Fast-path: Group by (Name, ClassName) -- 1:1 groups instant-match // ================================================================ - let mut new_by_key: HashMap<(String, String), Vec> = HashMap::new(); - for &r in &remaining_new { + let mut new_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(new_len); + for (i, &r) in new_children.iter().enumerate() { if let Some(inst) = new_dom.get_by_ref(r) { new_by_key - .entry((inst.name.clone(), inst.class.to_string())) + .entry((inst.name.clone(), inst.class)) .or_default() - .push(r); + .push(i); } } - let mut old_by_key: HashMap<(String, String), Vec> = HashMap::new(); - for &r in &remaining_old { + let mut old_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(old_len); + for (i, &r) in old_children.iter().enumerate() { if let Some(inst) = old_dom.get_by_ref(r) { old_by_key - .entry((inst.name.clone(), inst.class.to_string())) + .entry((inst.name.clone(), inst.class)) .or_default() - .push(r); + .push(i); } } - let mut matched_new: HashSet = HashSet::new(); - let mut matched_old: HashSet = HashSet::new(); - // 1:1 groups: instant match - // Sort keys for deterministic iteration - let mut sorted_keys: Vec<_> = new_by_key.keys().cloned().collect(); - sorted_keys.sort(); - - for key in &sorted_keys { - let Some(new_refs) = new_by_key.get(key) else { - continue; - }; - let Some(old_refs) = old_by_key.get(key) else { - continue; - }; - if new_refs.len() == 1 && old_refs.len() == 1 { - matched.push((new_refs[0], old_refs[0])); - matched_new.insert(new_refs[0]); - matched_old.insert(old_refs[0]); + for (key, new_indices) in &new_by_key { + if let Some(old_indices) = old_by_key.get(key) { + if new_indices.len() == 1 && old_indices.len() == 1 { + let ni = new_indices[0]; + let oi = old_indices[0]; + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; + } } } - remaining_new.retain(|r| !matched_new.contains(r)); - remaining_old.retain(|r| !matched_old.contains(r)); + let new_remaining = new_matched.iter().filter(|&&m| !m).count(); + let old_remaining = old_matched.iter().filter(|&&m| !m).count(); - if remaining_new.is_empty() || remaining_old.is_empty() { - let mut total_cost: u32 = 0; - for &(new_ref, old_ref) in &matched { - total_cost = total_cost.saturating_add(compute_change_count( - new_ref, - old_ref, - new_dom, - old_dom, - new_hashes, - old_hashes, - u32::MAX, - 0, - session, - )); - } - total_cost = total_cost - .saturating_add((remaining_new.len() + remaining_old.len()) as u32 * UNMATCHED_PENALTY); - return MatchResult { + if new_remaining == 0 || old_remaining == 0 { + return build_result( + new_children, + old_children, + &new_matched, + &old_matched, matched, - unmatched_new: remaining_new, - unmatched_old: remaining_old, - total_cost, - }; + ); } // ================================================================ // Ambiguous groups: change-count scoring + greedy assignment // ================================================================ - for key in &sorted_keys { - let Some(new_refs) = new_by_key.get(key) else { - continue; - }; - let Some(old_refs) = old_by_key.get(key) else { + for (key, new_indices) in &new_by_key { + let Some(old_indices) = old_by_key.get(key) else { continue; }; - let avail_new: Vec = new_refs + let avail_new: Vec = new_indices .iter() - .filter(|r| !matched_new.contains(r)) + .filter(|&&ni| !new_matched[ni]) .copied() .collect(); - let avail_old: Vec = old_refs + let avail_old: Vec = old_indices .iter() - .filter(|r| !matched_old.contains(r)) + .filter(|&&oi| !old_matched[oi]) .copied() .collect(); @@ -175,24 +149,25 @@ pub fn match_children( continue; } - // Handle remaining 1:1 (from groups that had multiple but some were - // already matched, leaving 1 on each side) if avail_new.len() == 1 && avail_old.len() == 1 { - matched.push((avail_new[0], avail_old[0])); - matched_new.insert(avail_new[0]); - matched_old.insert(avail_old[0]); + let ni = avail_new[0]; + let oi = avail_old[0]; + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; continue; } // Score all (A, B) pairs using recursive change-count scoring - let mut pairs: Vec<(u32, Ref, Ref)> = Vec::new(); + let mut pairs: Vec<(u32, usize, usize)> = + Vec::with_capacity(avail_new.len() * avail_old.len()); let mut best_so_far = u32::MAX; - for &new_ref in &avail_new { - for &old_ref in &avail_old { + for &ni in &avail_new { + for &oi in &avail_old { let cost = compute_change_count( - new_ref, - old_ref, + new_children[ni], + old_children[oi], new_dom, old_dom, new_hashes, @@ -201,7 +176,7 @@ pub fn match_children( 0, session, ); - pairs.push((cost, new_ref, old_ref)); + pairs.push((cost, ni, oi)); if cost < best_so_far { best_so_far = cost; } @@ -212,41 +187,56 @@ pub fn match_children( pairs.sort_by_key(|&(cost, _, _)| cost); // Greedy assign - for &(_, new_ref, old_ref) in &pairs { - if matched_new.contains(&new_ref) || matched_old.contains(&old_ref) { + for &(_, ni, oi) in &pairs { + if new_matched[ni] || old_matched[oi] { continue; } - matched.push((new_ref, old_ref)); - matched_new.insert(new_ref); - matched_old.insert(old_ref); + matched.push((ni, oi)); + new_matched[ni] = true; + old_matched[oi] = true; } } - remaining_new.retain(|r| !matched_new.contains(r)); - remaining_old.retain(|r| !matched_old.contains(r)); + build_result( + new_children, + old_children, + &new_matched, + &old_matched, + matched, + ) +} - let mut total_cost: u32 = 0; - for &(new_ref, old_ref) in &matched { - total_cost = total_cost.saturating_add(compute_change_count( - new_ref, - old_ref, - new_dom, - old_dom, - new_hashes, - old_hashes, - u32::MAX, - 0, - session, - )); - } - total_cost = total_cost - .saturating_add((remaining_new.len() + remaining_old.len()) as u32 * UNMATCHED_PENALTY); +/// Convert index-based matching results back to Ref-based MatchResult. +fn build_result( + new_children: &[Ref], + old_children: &[Ref], + new_matched: &[bool], + old_matched: &[bool], + matched_indices: Vec<(usize, usize)>, +) -> MatchResult { + let matched: Vec<(Ref, Ref)> = matched_indices + .into_iter() + .map(|(ni, oi)| (new_children[ni], old_children[oi])) + .collect(); + + let unmatched_new: Vec = new_matched + .iter() + .enumerate() + .filter(|(_, &m)| !m) + .map(|(i, _)| new_children[i]) + .collect(); + + let unmatched_old: Vec = old_matched + .iter() + .enumerate() + .filter(|(_, &m)| !m) + .map(|(i, _)| old_children[i]) + .collect(); MatchResult { matched, - unmatched_new: remaining_new, - unmatched_old: remaining_old, - total_cost, + unmatched_new, + unmatched_old, } } @@ -270,34 +260,36 @@ fn match_children_for_scoring( depth: u32, session: &MatchingSession, ) -> ScoringMatchResult { - let mut new_matched = vec![false; new_children.len()]; - let mut old_matched = vec![false; old_children.len()]; - let mut matched = Vec::new(); - if new_children.is_empty() && old_children.is_empty() { return ScoringMatchResult { - matched, + matched: Vec::new(), unmatched_new: 0, unmatched_old: 0, }; } + let new_len = new_children.len(); + let old_len = old_children.len(); + let mut new_matched = vec![false; new_len]; + let mut old_matched = vec![false; old_len]; + let mut matched: Vec<(usize, usize)> = Vec::with_capacity(new_len.min(old_len)); + // Group by (Name, ClassName) - let mut new_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut new_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(new_len); for (i, &r) in new_children.iter().enumerate() { if let Some(inst) = new_dom.get_by_ref(r) { new_by_key - .entry((inst.name.clone(), inst.class.to_string())) + .entry((inst.name.clone(), inst.class)) .or_default() .push(i); } } - let mut old_by_key: HashMap<(String, String), Vec> = HashMap::new(); + let mut old_by_key: HashMap<(String, Ustr), Vec> = HashMap::with_capacity(old_len); for (i, &r) in old_children.iter().enumerate() { if let Some(inst) = old_dom.get_by_ref(r) { old_by_key - .entry((inst.name.clone(), inst.class.to_string())) + .entry((inst.name.clone(), inst.class)) .or_default() .push(i); } @@ -345,7 +337,8 @@ fn match_children_for_scoring( continue; } - let mut pairs: Vec<(u32, usize, usize)> = Vec::new(); + let mut pairs: Vec<(u32, usize, usize)> = + Vec::with_capacity(avail_new.len() * avail_old.len()); let mut best_so_far = u32::MAX; for &ni in &avail_new { for &oi in &avail_old { @@ -502,6 +495,7 @@ fn compute_change_count( /// /// Properties present on only one side are skipped when their value matches /// the class default (see `src/snapshot/matching.rs` for full rationale). +#[inline] fn count_own_diffs(new_ref: Ref, old_ref: Ref, new_dom: &WeakDom, old_dom: &WeakDom) -> u32 { let new_inst = match new_dom.get_by_ref(new_ref) { Some(i) => i, @@ -759,106 +753,6 @@ mod tests { } } - #[test] - fn total_cost_zero_identical() { - let (new_dom, new_root, _, _) = build_test_dom(); - let (old_dom, old_root, _, _) = build_test_dom(); - - let new_children: Vec = new_dom.get_by_ref(new_root).unwrap().children().to_vec(); - let old_children: Vec = old_dom.get_by_ref(old_root).unwrap().children().to_vec(); - - let result = match_children( - &new_children, - &old_children, - &new_dom, - &old_dom, - None, - None, - &MatchingSession::new(), - ); - assert_eq!(result.matched.len(), 2); - assert_eq!( - result.total_cost, 0, - "Identical instances should have zero cost" - ); - } - - #[test] - fn total_cost_nonzero_different() { - use rbx_dom_weak::types::Variant; - - let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); - let new_root = new_dom.root_ref(); - new_dom.insert( - new_root, - InstanceBuilder::new("Part") - .with_name("P") - .with_property("Transparency", Variant::Float32(0.5)), - ); - - let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); - let old_root = old_dom.root_ref(); - old_dom.insert( - old_root, - InstanceBuilder::new("Part") - .with_name("P") - .with_property("Transparency", Variant::Float32(0.0)), - ); - - let new_children: Vec = new_dom.get_by_ref(new_root).unwrap().children().to_vec(); - let old_children: Vec = old_dom.get_by_ref(old_root).unwrap().children().to_vec(); - - let result = match_children( - &new_children, - &old_children, - &new_dom, - &old_dom, - None, - None, - &MatchingSession::new(), - ); - assert_eq!(result.matched.len(), 1); - assert!( - result.total_cost > 0, - "Different properties should produce nonzero cost, got {}", - result.total_cost - ); - } - - #[test] - fn total_cost_includes_unmatched() { - let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); - let new_root = new_dom.root_ref(); - new_dom.insert(new_root, InstanceBuilder::new("Folder").with_name("A")); - new_dom.insert(new_root, InstanceBuilder::new("Folder").with_name("B")); - new_dom.insert(new_root, InstanceBuilder::new("Folder").with_name("C")); - - let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); - let old_root = old_dom.root_ref(); - old_dom.insert(old_root, InstanceBuilder::new("Folder").with_name("A")); - old_dom.insert(old_root, InstanceBuilder::new("Folder").with_name("B")); - - let new_children: Vec = new_dom.get_by_ref(new_root).unwrap().children().to_vec(); - let old_children: Vec = old_dom.get_by_ref(old_root).unwrap().children().to_vec(); - - let result = match_children( - &new_children, - &old_children, - &new_dom, - &old_dom, - None, - None, - &MatchingSession::new(), - ); - assert_eq!(result.matched.len(), 2); - assert_eq!(result.unmatched_new.len(), 1); - assert!( - result.total_cost >= UNMATCHED_PENALTY, - "Should include penalty for unmatched, got {}", - result.total_cost - ); - } - #[test] fn session_cache_consistent_syncback() { let (new_dom, new_root, _, _) = build_test_dom(); @@ -887,6 +781,5 @@ mod tests { &session, ); assert_eq!(r1.matched.len(), r2.matched.len()); - assert_eq!(r1.total_cost, r2.total_cost); } } diff --git a/src/variant_eq.rs b/src/variant_eq.rs index ae3ca20d..9a9c68d5 100644 --- a/src/variant_eq.rs +++ b/src/variant_eq.rs @@ -12,6 +12,7 @@ macro_rules! approx_eq { /// Compares two variants to determine if they're equal. This correctly takes /// float comparisons into account. +#[inline] pub fn variant_eq(variant_a: &Variant, variant_b: &Variant) -> bool { if variant_a.ty() != variant_b.ty() { return false; diff --git a/src/web/interface.rs b/src/web/interface.rs index 2f0a27fb..fe103a65 100644 --- a/src/web/interface.rs +++ b/src/web/interface.rs @@ -27,13 +27,31 @@ pub(crate) const SERVER_VERSION: &str = env!("CARGO_PKG_VERSION"); /// Current protocol version, which is required to match. pub const PROTOCOL_VERSION: u64 = 6; -/// A single service's children serialized as rbxm binary data. +/// A single service's children serialized as rbxm binary data, plus the +/// service's own properties, attributes, tags, and Ref property hints. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ServiceChunk { pub class_name: String, #[serde(with = "serde_bytes")] pub data: Vec, + #[serde(default)] + pub properties: HashMap, + #[serde(default)] + pub attributes: HashMap, + #[serde(default)] + pub tags: Vec, + #[serde(default)] + pub refs: HashMap, +} + +/// Describes a Ref property target by name and class so the server can resolve +/// it among the service's cloned children. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ServiceRef { + pub name: String, + pub class_name: String, } /// Incoming request from the plugin for live syncback. diff --git a/tests/rojo_test/serve_util.rs b/tests/rojo_test/serve_util.rs index 8ac90eeb..d67cfcfb 100644 --- a/tests/rojo_test/serve_util.rs +++ b/tests/rojo_test/serve_util.rs @@ -527,26 +527,69 @@ fn get_port_number() -> usize { pub fn make_service_chunk( class_name: &str, children: Vec, +) -> librojo::web_api::ServiceChunk { + make_service_chunk_full(class_name, vec![], vec![], vec![], vec![], children) +} + +/// Build a ServiceChunk with explicit service-level properties, attributes, +/// tags, and ref hints alongside the serialized children. +pub fn make_service_chunk_full( + class_name: &str, + properties: Vec<(&str, rbx_dom_weak::types::Variant)>, + attributes: Vec<(&str, rbx_dom_weak::types::Variant)>, + tags: Vec, + refs: Vec<(&str, &str, &str)>, + children: Vec, ) -> librojo::web_api::ServiceChunk { use rbx_dom_weak::WeakDom; + use std::collections::HashMap; let mut dom = WeakDom::new(rbx_dom_weak::InstanceBuilder::new("DataModel")); let root = dom.root_ref(); for child in children { dom.insert(root, child); } - let refs: Vec = dom.root().children().to_vec(); + let child_refs: Vec = dom.root().children().to_vec(); let mut buf = Vec::new(); - rbx_binary::to_writer(&mut buf, &dom, &refs).unwrap(); + if !child_refs.is_empty() { + rbx_binary::to_writer(&mut buf, &dom, &child_refs).unwrap(); + } + + let props_map: HashMap = properties + .into_iter() + .map(|(k, v)| (k.to_string(), v)) + .collect(); + let attrs_map: HashMap = attributes + .into_iter() + .map(|(k, v)| (k.to_string(), v)) + .collect(); + let refs_map: HashMap = refs + .into_iter() + .map(|(prop, name, class)| { + ( + prop.to_string(), + librojo::web_api::ServiceRef { + name: name.to_string(), + class_name: class.to_string(), + }, + ) + }) + .collect(); + librojo::web_api::ServiceChunk { class_name: class_name.to_string(), data: buf, + properties: props_map, + attributes: attrs_map, + tags, + refs: refs_map, } } /// Build a complete rbxl file from service chunks, mirroring the same data /// that live syncback receives. Used for CLI parity testing. pub fn make_rbxl_from_chunks(chunks: &[librojo::web_api::ServiceChunk]) -> Vec { + use rbx_dom_weak::types::{Attributes, Tags, Variant}; use rbx_dom_weak::WeakDom; use std::collections::HashMap; use std::io::Cursor; @@ -555,63 +598,80 @@ pub fn make_rbxl_from_chunks(chunks: &[librojo::web_api::ServiceChunk]) -> Vec = HashMap::new(); - for &child_ref in chunk_dom.root().children() { - deep_clone_into_test(&chunk_dom, &mut dom, child_ref, service_ref, &mut ref_map); + for (key, value) in &chunk.properties { + builder = builder.with_property(key.as_str(), value.clone()); } - let all_refs: Vec = ref_map.values().copied().collect(); - for inst_ref in all_refs { - let props_to_fix: Vec<(String, Ref)> = { - let inst = dom.get_by_ref(inst_ref).unwrap(); - inst.properties - .iter() - .filter_map(|(key, value)| { - if let rbx_dom_weak::types::Variant::Ref(r) = value { - ref_map.get(r).map(|&mapped| (key.to_string(), mapped)) - } else { - None - } - }) - .collect() - }; - if !props_to_fix.is_empty() { - let inst = dom.get_by_ref_mut(inst_ref).unwrap(); - for (key, new_ref) in props_to_fix { + if !chunk.attributes.is_empty() { + let mut attrs = Attributes::new(); + for (key, value) in &chunk.attributes { + attrs.insert(key.clone(), value.clone()); + } + builder = builder.with_property("Attributes", Variant::Attributes(attrs)); + } + + if !chunk.tags.is_empty() { + let tags: Tags = chunk.tags.iter().map(|s| s.as_str()).collect(); + builder = builder.with_property("Tags", Variant::Tags(tags)); + } + + let service_ref = dom.insert(root_ref, builder); + + if !chunk.data.is_empty() { + let chunk_dom = rbx_binary::from_reader(Cursor::new(&chunk.data)) + .unwrap_or_else(|e| panic!("Failed to parse rbxm for {}: {e}", chunk.class_name)); + + let mut ref_map: HashMap = HashMap::new(); + for &child_ref in chunk_dom.root().children() { + deep_clone_into_test(&chunk_dom, &mut dom, child_ref, service_ref, &mut ref_map); + } + + let all_refs: Vec = ref_map.values().copied().collect(); + for inst_ref in all_refs { + let props_to_fix: Vec<(String, Ref)> = { + let inst = dom.get_by_ref(inst_ref).unwrap(); inst.properties - .insert(key.into(), rbx_dom_weak::types::Variant::Ref(new_ref)); + .iter() + .filter_map(|(key, value)| { + if let Variant::Ref(r) = value { + ref_map.get(r).map(|&mapped| (key.to_string(), mapped)) + } else { + None + } + }) + .collect() + }; + if !props_to_fix.is_empty() { + let inst = dom.get_by_ref_mut(inst_ref).unwrap(); + for (key, new_ref) in props_to_fix { + inst.properties.insert(key.into(), Variant::Ref(new_ref)); + } + } + } + } + + if !chunk.refs.is_empty() { + let children: Vec = dom.get_by_ref(service_ref).unwrap().children().to_vec(); + for (prop_name, target) in &chunk.refs { + let found = children.iter().find(|&&child_ref| { + let child = dom.get_by_ref(child_ref).unwrap(); + child.name.as_str() == target.name && child.class.as_str() == target.class_name + }); + if let Some(&child_ref) = found { + let service = dom.get_by_ref_mut(service_ref).unwrap(); + service + .properties + .insert(prop_name.as_str().into(), Variant::Ref(child_ref)); } } } } - let visible_services = [ - "Lighting", - "MaterialService", - "ReplicatedFirst", - "ReplicatedStorage", - "ServerScriptService", - "ServerStorage", - "SoundService", - "StarterGui", - "StarterPack", - "StarterPlayer", - "Teams", - "TextChatService", - "VoiceChatService", - "Workspace", - ]; let existing: std::collections::HashSet = chunks.iter().map(|c| c.class_name.clone()).collect(); - for &svc in &visible_services { + for &svc in librojo::syncback::VISIBLE_SERVICES { if !existing.contains(svc) { dom.insert(root_ref, rbx_dom_weak::InstanceBuilder::new(svc)); } diff --git a/tests/tests/live_syncback.rs b/tests/tests/live_syncback.rs index 5752138b..c8e34afc 100644 --- a/tests/tests/live_syncback.rs +++ b/tests/tests/live_syncback.rs @@ -4,7 +4,9 @@ use rbx_dom_weak::InstanceBuilder; use crate::rojo_test::{ roundtrip_util::assert_dirs_equal, - serve_util::{make_service_chunk, run_cli_syncback_on_chunks, run_serve_test}, + serve_util::{ + make_service_chunk, make_service_chunk_full, run_cli_syncback_on_chunks, run_serve_test, + }, }; use librojo::web_api::{ServiceChunk, SocketPacketType, PROTOCOL_VERSION}; @@ -182,6 +184,189 @@ fn parity_empty_services_mixed() { assert_live_matches_cli("live_syncback", &chunks, None); } +// ── Service property parity tests ──────────────────────────────── + +#[test] +fn parity_lighting_properties() { + use rbx_dom_weak::types::{Color3, Variant}; + + let chunks = vec![make_service_chunk_full( + "Lighting", + vec![ + ("Ambient", Variant::Color3(Color3::new(0.5, 0.5, 0.5))), + ("Brightness", Variant::Float32(2.0)), + ("ClockTime", Variant::Float32(14.5)), + ], + vec![], + vec![], + vec![], + vec![InstanceBuilder::new("PointLight").with_name("TestLight")], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_soundservice_properties() { + use rbx_dom_weak::types::Variant; + + let chunks = vec![make_service_chunk_full( + "SoundService", + vec![ + ( + "AmbientReverb", + Variant::Enum(rbx_dom_weak::types::Enum::from_u32(2)), + ), + ("DistanceFactor", Variant::Float32(5.0)), + ], + vec![], + vec![], + vec![], + vec![], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_starterplayer_properties() { + use rbx_dom_weak::types::Variant; + + let chunks = vec![make_service_chunk_full( + "StarterPlayer", + vec![("CameraMaxZoomDistance", Variant::Float32(200.0))], + vec![], + vec![], + vec![], + vec![], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_textchatservice_properties() { + use rbx_dom_weak::types::Variant; + + let chunks = vec![make_service_chunk_full( + "TextChatService", + vec![( + "ChatVersion", + Variant::Enum(rbx_dom_weak::types::Enum::from_u32(2)), + )], + vec![], + vec![], + vec![], + vec![], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_childless_service_with_properties() { + use rbx_dom_weak::types::Variant; + + let chunks = vec![make_service_chunk_full( + "VoiceChatService", + vec![( + "UseAudioApi", + Variant::Enum(rbx_dom_weak::types::Enum::from_u32(2)), + )], + vec![], + vec![], + vec![], + vec![], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_service_properties_with_children() { + use rbx_dom_weak::types::{Color3, Variant}; + + let chunks = vec![make_service_chunk_full( + "Lighting", + vec![ + ("Ambient", Variant::Color3(Color3::new(0.3, 0.3, 0.3))), + ("Brightness", Variant::Float32(3.0)), + ], + vec![], + vec![], + vec![], + vec![ + InstanceBuilder::new("PointLight").with_name("LightA"), + InstanceBuilder::new("ModuleScript") + .with_name("LightingConfig") + .with_property("Source", Variant::String("return {}".into())), + ], + )]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_multiple_services_with_properties() { + use rbx_dom_weak::types::{Color3, Variant}; + + let chunks = vec![ + make_service_chunk_full( + "Lighting", + vec![("Ambient", Variant::Color3(Color3::new(0.2, 0.2, 0.2)))], + vec![], + vec![], + vec![], + vec![], + ), + make_service_chunk_full( + "SoundService", + vec![("DistanceFactor", Variant::Float32(10.0))], + vec![], + vec![], + vec![], + vec![], + ), + make_service_chunk( + "ReplicatedStorage", + vec![InstanceBuilder::new("Folder").with_name("Assets")], + ), + ]; + assert_live_matches_cli("live_syncback", &chunks, None); +} + +#[test] +fn parity_camera_not_synced() { + let camera_child = InstanceBuilder::new("Camera").with_name("Camera"); + let chunks = vec![make_service_chunk_full( + "Workspace", + vec![], + vec![], + vec![], + vec![("CurrentCamera", "Camera", "Camera")], + vec![ + camera_child, + InstanceBuilder::new("Part") + .with_name("Floor") + .with_property("Anchored", rbx_dom_weak::types::Variant::Bool(true)), + ], + )]; + + run_serve_test("live_syncback", |session, _| { + session.post_api_syncback(None, chunks.to_vec()); + session.wait_to_come_back_online(); + + let workspace_dir = session.path().join("src/workspace"); + let has_camera = workspace_dir.join("Camera").exists() + || workspace_dir.join("Camera.rbxm").exists() + || workspace_dir.join("Camera.model.json5").exists(); + + assert!( + !has_camera, + "Camera should not appear on disk (syncCurrentCamera defaults to false)" + ); + + assert!( + workspace_dir.join("Floor.model.json5").exists(), + "Non-camera children should still be written" + ); + }); +} + // ── Validation / rejection tests ───────────────────────────────── #[test] @@ -461,14 +646,59 @@ fn roundtrip_build_syncback_rebuild() { for &service_ref in dom_a.root().children() { let service = dom_a.get_by_ref(service_ref).unwrap(); let child_refs: Vec = service.children().to_vec(); - if child_refs.is_empty() { - continue; + + let mut data = Vec::new(); + if !child_refs.is_empty() { + rbx_binary::to_writer(&mut data, &dom_a, &child_refs).unwrap(); } - let mut buf = Vec::new(); - rbx_binary::to_writer(&mut buf, &dom_a, &child_refs).unwrap(); + + let mut properties = std::collections::HashMap::new(); + let mut refs_map = std::collections::HashMap::new(); + for (key, value) in &service.properties { + match value { + rbx_dom_weak::types::Variant::Ref(target) => { + if let Some(target_inst) = dom_a.get_by_ref(*target) { + refs_map.insert( + key.to_string(), + librojo::web_api::ServiceRef { + name: target_inst.name.to_string(), + class_name: target_inst.class.to_string(), + }, + ); + } + } + rbx_dom_weak::types::Variant::Attributes(_) + | rbx_dom_weak::types::Variant::Tags(_) => {} + _ => { + properties.insert(key.to_string(), value.clone()); + } + } + } + + let mut attributes_map = std::collections::HashMap::new(); + if let Some(rbx_dom_weak::types::Variant::Attributes(attrs)) = + service.properties.get(&rbx_dom_weak::ustr("Attributes")) + { + for (k, v) in attrs.iter() { + attributes_map.insert(k.to_string(), v.clone()); + } + } + + let tags_vec = if let Some(rbx_dom_weak::types::Variant::Tags(tags)) = + service.properties.get(&rbx_dom_weak::ustr("Tags")) + { + tags.iter().map(|s| s.to_string()).collect() + } else { + vec![] + }; + chunks.push(ServiceChunk { class_name: service.class.to_string(), - data: buf, + data, + properties, + attributes: attributes_map, + tags: tags_vec, + refs: refs_map, }); }