mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Implement live syncback fixes addressing multiple issues
- Added explicit encoding for Workspace attributes and tags in `encodeService.lua` to prevent data loss during syncback. - Filtered non-serializable properties, including `ClockTime`, in `build_dom_from_chunks` to ensure accurate data handling. - Resolved a leak of `Rojo_Ref_CurrentCamera` by filtering deferred referents after camera destruction in `syncback_loop`. - Truncated float values to f32 precision in `UnresolvedValue::from_variant` to match rbxl storage format, improving data consistency. These changes enhance the reliability and accuracy of the live syncback process, addressing critical issues identified in previous implementations.
This commit is contained in:
parent
a75a1443c0
commit
7dc4f52793
6 changed files with 180 additions and 5 deletions
65
.cursor/plans/live_syncback_float_precision_1be79350.plan.md
Normal file
65
.cursor/plans/live_syncback_float_precision_1be79350.plan.md
Normal file
|
|
@ -0,0 +1,65 @@
|
|||
---
|
||||
name: Live syncback fixes
|
||||
overview: "Fix five issues: lost Workspace attributes, spurious ClockTime, Rojo_Ref_CurrentCamera leak, and float precision noise via f32 truncation."
|
||||
todos:
|
||||
- id: fix-attributes-encoding
|
||||
content: Add explicit Attributes/Tags encoding to encodeService.lua (mirroring encodeInstance.lua)
|
||||
status: completed
|
||||
- id: fix-clocktime
|
||||
content: Filter non-serializable properties server-side in build_dom_from_chunks using the Rust reflection DB
|
||||
status: completed
|
||||
- id: fix-currentcamera-ref
|
||||
content: "Stop Rojo_Ref_CurrentCamera from leaking: filter deferred_referents after camera destruction in syncback_loop"
|
||||
status: completed
|
||||
- id: fix-float-rounding
|
||||
content: Truncate all float values to f32 in UnresolvedValue::from_variant for rbxl parity
|
||||
status: completed
|
||||
isProject: false
|
||||
---
|
||||
|
||||
# Live Syncback Fixes
|
||||
|
||||
## Issue 1: Workspace Attributes/Properties Lost
|
||||
|
||||
**Root cause:** `encodeService.lua` iterates `classDescriptor.properties` but the Luau RbxDom database may not list `Attributes` and `Tags` as enumerable properties. `encodeInstance.lua` has explicit `encodeAttributes()` / `encodeTags()` helpers using `instance:GetAttributes()` / `instance:GetTags()` directly.
|
||||
|
||||
**Fix:** Add explicit Attributes and Tags encoding to `[plugin/src/ChangeBatcher/encodeService.lua](plugin/src/ChangeBatcher/encodeService.lua)` after the property loop. Add `if propertyName == "Attributes" or propertyName == "Tags" then continue end` to the main loop to avoid double-encoding.
|
||||
|
||||
## Issue 2: Lighting.ClockTime Appearing
|
||||
|
||||
**Root cause:** Luau DB marks ClockTime as serializable; Rust DB marks it as DoesNotSerialize. Plugin sends it, CLI syncback filters it.
|
||||
|
||||
**Fix:** In `[src/cli/serve.rs](src/cli/serve.rs)` `build_dom_from_chunks`, filter each plugin-sent property through `should_property_serialize` from the Rust reflection DB before applying it to the builder.
|
||||
|
||||
## Issue 3: Rojo_Ref_CurrentCamera Leaking (pre-existing bug)
|
||||
|
||||
**Root cause:** `collect_referents` captures `Workspace.CurrentCamera` -> Camera ref before camera is destroyed. `link_referents` writes `Rojo_Ref_CurrentCamera` without checking target still exists.
|
||||
|
||||
**Fix:** In `[src/syncback/mod.rs](src/syncback/mod.rs)`, after camera destruction (line 222), filter `deferred_referents.path_links` to remove entries whose target ref no longer exists.
|
||||
|
||||
## Issue 4: Float Precision
|
||||
|
||||
**Root cause:** Live syncback sends Float64 from Lua; rbxl stores Float32. Tiny diffs cause properties to fail default comparison.
|
||||
|
||||
**Fix:** In `[src/resolution.rs](src/resolution.rs)` `UnresolvedValue::from_variant`, truncate ALL float values to f32 precision. This is the single chokepoint for json5 writes:
|
||||
|
||||
- **Float32(n)** (line 67): already f32, `n as f64` is fine (no change needed)
|
||||
- **Float64(n)** (line 68): change to `(n as f32) as f64` to truncate
|
||||
- **Vector2** (line 82): `(vector.x as f32) as f64` etc
|
||||
- **Vector3** (line 85): same
|
||||
- **CFrame** (lines 90-103): all 12 components
|
||||
- **Color3** (line 88): already f32, fine
|
||||
- **UDim/UDim2**: fall through to `FullyQualified` -- add a `round_variant` helper that truncates float components to f32 before wrapping
|
||||
|
||||
Helper:
|
||||
|
||||
```rust
|
||||
fn truncate_to_f32(v: f64) -> f64 {
|
||||
(v as f32) as f64
|
||||
}
|
||||
```
|
||||
|
||||
## Deferred: Matching Parity
|
||||
|
||||
Matching differences between live and CLI syncback are likely downstream of the above fixes (extra properties inflating scores, missing attributes, float diffs). Revisit after these four fixes land. If matching is still off, consider reusing the previously-served tree instead of creating a fresh `ServeSession::new_oneshot` in `run_live_syncback`.
|
||||
|
||||
|
|
@ -21,6 +21,9 @@ return function(service: Instance)
|
|||
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"
|
||||
|
|
@ -53,6 +56,32 @@ return function(service: Instance)
|
|||
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
|
||||
properties.Attributes = encoded
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
local tagOk, tagList = pcall(function()
|
||||
return service:GetTags()
|
||||
end)
|
||||
if tagOk and tagList and #tagList > 0 then
|
||||
local descriptor = RbxDom.findCanonicalPropertyDescriptor(service.ClassName, "Tags")
|
||||
if descriptor then
|
||||
local encodeOk, encoded = encodeProperty(service, "Tags", descriptor)
|
||||
if encodeOk and encoded ~= nil then
|
||||
properties.Tags = encoded
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
local chunk = {
|
||||
className = service.ClassName,
|
||||
childCount = #service:GetChildren(),
|
||||
|
|
|
|||
|
|
@ -164,6 +164,9 @@ fn build_dom_from_chunks(payload: SyncbackPayload) -> anyhow::Result<WeakDom> {
|
|||
for chunk in &payload.services {
|
||||
let mut builder = InstanceBuilder::new(&chunk.class_name);
|
||||
for (key, value) in &chunk.properties {
|
||||
if !crate::syncback::should_property_serialize(&chunk.class_name, key) {
|
||||
continue;
|
||||
}
|
||||
builder = builder.with_property(key.as_str(), value.clone());
|
||||
}
|
||||
let service_ref = dom.insert(root_ref, builder);
|
||||
|
|
|
|||
|
|
@ -10,6 +10,68 @@ use serde::{Deserialize, Serialize};
|
|||
|
||||
use crate::REF_PATH_ATTRIBUTE_PREFIX;
|
||||
|
||||
/// Truncates float components of Variant types to f32 precision, matching the
|
||||
/// rbxl binary format which stores most float properties as Float32.
|
||||
fn truncate_variant(v: Variant) -> Variant {
|
||||
#[inline]
|
||||
fn t(v: f32) -> f32 {
|
||||
v as f64 as f32
|
||||
}
|
||||
|
||||
match v {
|
||||
Variant::CFrame(cf) => Variant::CFrame(CFrame {
|
||||
position: Vector3::new(t(cf.position.x), t(cf.position.y), t(cf.position.z)),
|
||||
orientation: Matrix3::new(
|
||||
Vector3::new(
|
||||
t(cf.orientation.x.x),
|
||||
t(cf.orientation.x.y),
|
||||
t(cf.orientation.x.z),
|
||||
),
|
||||
Vector3::new(
|
||||
t(cf.orientation.y.x),
|
||||
t(cf.orientation.y.y),
|
||||
t(cf.orientation.y.z),
|
||||
),
|
||||
Vector3::new(
|
||||
t(cf.orientation.z.x),
|
||||
t(cf.orientation.z.y),
|
||||
t(cf.orientation.z.z),
|
||||
),
|
||||
),
|
||||
}),
|
||||
Variant::OptionalCFrame(Some(cf)) => Variant::OptionalCFrame(Some(CFrame {
|
||||
position: Vector3::new(t(cf.position.x), t(cf.position.y), t(cf.position.z)),
|
||||
orientation: Matrix3::new(
|
||||
Vector3::new(
|
||||
t(cf.orientation.x.x),
|
||||
t(cf.orientation.x.y),
|
||||
t(cf.orientation.x.z),
|
||||
),
|
||||
Vector3::new(
|
||||
t(cf.orientation.y.x),
|
||||
t(cf.orientation.y.y),
|
||||
t(cf.orientation.y.z),
|
||||
),
|
||||
Vector3::new(
|
||||
t(cf.orientation.z.x),
|
||||
t(cf.orientation.z.y),
|
||||
t(cf.orientation.z.z),
|
||||
),
|
||||
),
|
||||
})),
|
||||
Variant::Vector3(v) => Variant::Vector3(Vector3::new(t(v.x), t(v.y), t(v.z))),
|
||||
Variant::Vector2(v) => Variant::Vector2(Vector2::new(t(v.x), t(v.y))),
|
||||
Variant::UDim(u) => Variant::UDim(rbx_dom_weak::types::UDim::new(t(u.scale), u.offset)),
|
||||
Variant::UDim2(u) => Variant::UDim2(rbx_dom_weak::types::UDim2::new(
|
||||
rbx_dom_weak::types::UDim::new(t(u.x.scale), u.x.offset),
|
||||
rbx_dom_weak::types::UDim::new(t(u.y.scale), u.y.offset),
|
||||
)),
|
||||
Variant::Float32(n) => Variant::Float32(n as f64 as f32),
|
||||
Variant::Float64(n) => Variant::Float64(n as f32 as f64),
|
||||
other => other,
|
||||
}
|
||||
}
|
||||
|
||||
/// A user-friendly version of `Variant` that supports specifying ambiguous
|
||||
/// values. Ambiguous values need a reflection database to be resolved to a
|
||||
/// usable value.
|
||||
|
|
@ -41,6 +103,7 @@ impl UnresolvedValue {
|
|||
/// Creates an `UnresolvedValue` from a variant, using a class and property
|
||||
/// name to potentially allow for ambiguous Enum variants.
|
||||
pub fn from_variant(variant: Variant, class_name: &str, prop_name: &str) -> Self {
|
||||
let variant = truncate_variant(variant);
|
||||
let descriptor = find_descriptor(class_name, prop_name);
|
||||
if descriptor.is_some() {
|
||||
// We can only use an ambiguous syntax if the property is known
|
||||
|
|
@ -116,6 +179,7 @@ impl UnresolvedValue {
|
|||
/// Creates an `UnresolvedValue` from a variant, only returning ambiguous
|
||||
/// values if they're able to be resolved in a context-free environment.
|
||||
pub fn from_variant_unambiguous(variant: Variant) -> Self {
|
||||
let variant = truncate_variant(variant);
|
||||
match variant {
|
||||
Variant::String(str) => Self::Ambiguous(AmbiguousValue::String(str)),
|
||||
Variant::Float64(number) => Self::Ambiguous(AmbiguousValue::Number(number)),
|
||||
|
|
|
|||
|
|
@ -207,18 +207,21 @@ pub fn syncback_loop_with_stats(
|
|||
.unwrap_or(false);
|
||||
if !sync_current_camera {
|
||||
log::debug!("Removing CurrentCamera from new DOM");
|
||||
let mut camera_ref = None;
|
||||
let mut workspace_ref = None;
|
||||
let mut camera_target = None;
|
||||
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(&ustr("CurrentCamera"));
|
||||
workspace_ref = Some(*child_ref);
|
||||
camera_target = inst.properties.get(&ustr("CurrentCamera")).cloned();
|
||||
break;
|
||||
}
|
||||
}
|
||||
if let Some(Variant::Ref(camera_ref)) = camera_ref {
|
||||
if new_tree.get_by_ref(*camera_ref).is_some() {
|
||||
new_tree.destroy(*camera_ref);
|
||||
if let (Some(ws_ref), Some(Variant::Ref(cam_ref))) = (workspace_ref, camera_target) {
|
||||
if new_tree.get_by_ref(cam_ref).is_some() {
|
||||
new_tree.destroy(cam_ref);
|
||||
}
|
||||
deferred_referents.remove_ref(ws_ref, "CurrentCamera");
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -149,6 +149,17 @@ pub struct RefLinks {
|
|||
pub placeholder_to_source_and_target: HashMap<String, (Ref, Ref)>,
|
||||
}
|
||||
|
||||
impl RefLinks {
|
||||
pub fn remove_ref(&mut self, source: Ref, prop_name: &str) {
|
||||
if let Some(links) = self.path_links.get_mut(&source) {
|
||||
links.retain(|link| link.name.as_str() != prop_name);
|
||||
}
|
||||
if let Some(links) = self.id_links.get_mut(&source) {
|
||||
links.retain(|link| link.name.as_str() != prop_name);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(PartialEq, Eq)]
|
||||
struct PathRefLink {
|
||||
name: Ustr,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue