hakowork/rojo
3
0
Fork 0
mirror of https://github.com/rojo-rbx/rojo.git synced 2026-10-23 05:51:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Remove float-cmp dependency and implement custom float comparison logic

Browse source 
- Eliminated the `float-cmp` crate from the project, replacing its functionality with custom float comparison functions that match Lua's epsilon-based equality checks.
- Updated the `variant_eq` function to utilize the new fuzzy equality methods for `Float32` and `Float64` variants, ensuring consistent behavior across Rust and Lua implementations.
- Enhanced the matching algorithms in both forward sync and syncback processes to account for refined scoring of `Ref`, `Tags`, and `Attributes`, improving scoring accuracy and parity between implementations.
- Introduced a new plan document detailing the changes made to the matching algorithms, including the rationale for the updates and the expected impact on performance and correctness.

These changes aim to improve the reliability and maintainability of the matching algorithms while ensuring parity with Lua's behavior.
This commit is contained in:
Joe 2026-02-18 23:57:12 -06:00
parent a283fd1dc9
commit 7e0538b9f5
10 changed files with 1276 additions and 87 deletions
Download patch file Download diff file Expand all files Collapse all files

275
.cursor/plans/matching_algorithm_parity_01524d4c.plan.md Normal file
View file

@ -0,0 +1,275 @@
---
name: matching algorithm parity
overview: Fix scoring parity between the 3 matching algorithm implementations (Rust forward sync, Rust syncback, Lua plugin) and add parity/round-trip tests.
todos:
- id: fix-1-ref-scoring
content: "Fix 1: Add Ref-target-aware scoring to Rust countOwnDiffs in both forward sync and syncback matching"
status: completed
- id: fix-2-tags-attrs
content: "Fix 2: Add granular Tags/Attributes scoring to Rust countOwnDiffs in both implementations"
status: completed
- id: fix-3-epsilon
content: "Fix 3: Replace float_cmp::approx_eq! with custom fuzzy_eq matching Lua's 0.0001 epsilon in variant_eq.rs"
status: completed
- id: fix-4-sort
content: "Fix 4: Add matched_indices sort to forward sync build_result; remove TODO from syncback"
status: completed
- id: fix-5-vcache
content: "Fix 5: Cache VCache at session level in Lua matching.lua to avoid redundant decoding"
status: completed
- id: fix-6-tests
content: "Fix 6: Add parity tests (Rust forward vs syncback), round-trip matching test, and Lua parity spec test"
status: completed
- id: run-ci
content: Run /ci after all fixes to verify CI passes
status: completed
isProject: false
---
# Matching Algorithm Parity Audit Fixes
> This plan was generated by `/audit` from a Plan-mode session.
> Refer to `.cursor/rules/atlas.mdc` for full project standards.
## Standards
These standards govern every fix in this plan. An implementer MUST read `.cursor/rules/atlas.mdc` before starting.
### Round-Trip Identity (from `atlas.mdc` Quality Standard)
Syncback (or two-way sync) writes a directory tree. Building an rbxl from that directory tree and forward-syncing it back must produce a **bit-identical instance tree** -- same names, same classes, same properties, same hierarchy, same ref targets. Any deviation is a bug.
### CLI Syncback Parity (from `atlas.mdc` Two-Way Sync Strategy)
Plugin-based sync must produce **exactly the same filesystem output** that `atlas syncback` would give for the same input. Byte-for-byte identical files, identical directory structures, identical naming. Any divergence is a bug until proven otherwise.
### Code Quality (from `atlas.mdc` Code Quality Standard)
DRY code that is easy to reason about and hard to get wrong. The same slugify/dedup/meta-update pattern should not be copy-pasted across files. Small refactors (2-3 call sites) are included as fixes. Major rewrites (10+ call sites) are deferred.
## Context
The matching algorithm was audited across all 3 parallel implementations:
- `src/snapshot/matching.rs` (Rust forward sync)
- `src/syncback/matching.rs` (Rust syncback)
- `plugin/src/Reconciler/matching.lua` (Lua plugin)
The audit found **scoring parity issues** that could cause different implementations to produce different pairings for the same logical input. The most critical is Ref property scoring (UNMATCHED_PENALTY in Lua vs +1 in Rust), followed by Tags/Attributes granularity and float epsilon differences.
## Fixes
### Fix 1: Add Ref-Target-Aware Scoring to Rust `countOwnDiffs`
- **Status:** Approved
- **Finding:** Step 11l: Matching Algorithm Parity -- Ref property scoring
- **Files:** `src/snapshot/matching.rs`, `src/syncback/matching.rs`
- **Problem:** Lua `countOwnDiffs` (matching.lua lines 309-325) scores Ref property mismatches at `UNMATCHED_PENALTY = 10000` by comparing target Name+ClassName. Rust `count_own_diffs` in both files treats Ref properties as regular Variant properties scoring +1 per mismatch. In syncback matching, Ref IDs are always different between WeakDom instances, so ALL Ref properties score +1 regardless of logical equivalence.
- **Solution:**
- In **forward sync** `count_own_diffs` (matching.rs line 452-496): When comparing properties, detect `Variant::Ref` values. For each Ref property:
- If both sides have a Ref value, resolve both Refs to their target instances (via `tree.get_instance(ref)` for tree side, look up snapshot children by snapshot_id for snap side). Compare target Name+ClassName. If different, add `UNMATCHED_PENALTY` instead of 1.
- If one side has Ref and other doesn't (or Ref::none vs valid ref), add `UNMATCHED_PENALTY`.
- Skip `Variant::Ref` from regular property comparison to avoid double-counting.
- In **syncback** `count_own_diffs` (matching.rs line 501-548): Same approach using `new_dom.get_by_ref(ref)` and `old_dom.get_by_ref(ref)` to resolve targets.
- Both functions need access to the DOM/tree for Ref resolution -- add the necessary parameters.
- **Risk:** Adding DOM access to `count_own_diffs` increases its parameter count. The function is `#[inline]`, so ensure the extra parameter doesn't cause performance regression. The Ref resolution is a simple hash lookup (O(1)).
- **Verify round-trip:** Create 3 Models named "Weapon" with different PrimaryPart targets. Syncback, then forward sync. Verify all 3 match to their correct counterparts.
- **Verify syncback parity:** Compare CLI syncback output with two-way sync output for a tree with Ref-differentiated instances.
- **Tests required:**
- Rust unit test: `ref_scoring_forward_sync` -- 2 Models with different PrimaryPart targets, verify correct pairing
- Rust unit test: `ref_scoring_syncback` -- same scenario for syncback matching
- Lua spec test: already has ref scoring test in matching.spec.lua, verify it passes
### Fix 2: Add Granular Tags/Attributes Scoring to Rust `countOwnDiffs`
- **Status:** Approved
- **Finding:** Step 11l: Matching Algorithm Parity -- Tags/Attributes granularity
- **Files:** `src/snapshot/matching.rs`, `src/syncback/matching.rs`
- **Problem:** Lua `countOwnDiffs` counts per-tag (+1 each in symmetric difference) and per-attribute (+1 each diff). Rust counts Tags as one Variant property (+1 total) and Attributes as one Variant property (+1 total). An instance with 5 extra tags scores +5 in Lua but +1 in Rust.
- **Solution:**
- In both Rust `count_own_diffs` functions:
- When encountering `Variant::Tags(tags)`, extract and compare granularly: +1 per tag in symmetric difference. Skip from regular property comparison.
- When encountering `Variant::Attributes(attrs)`, extract and compare granularly: +1 per attribute whose value differs (using `variant_eq`), +1 per attribute present only on one side. Skip from regular property comparison.
- The key check is `key == "Tags"` (for `Variant::Tags`) and `key == "Attributes"` (for `Variant::Attributes`).
- **Risk:** Low. Only changes the cost value for Tags/Attributes, making it more discriminating. Existing tests should still pass since they don't depend on exact cost values, only on correct pairings.
- **Verify round-trip:** Instances differing by tags should be paired correctly.
- **Verify syncback parity:** N/A -- Tags/Attributes rarely distinguish instances in typical syncback scenarios.
- **Tests required:**
- Rust unit test: `tags_granular_scoring` -- 2 Parts with different tag sets, verify scoring matches Lua behavior
- Rust unit test: `attributes_granular_scoring` -- 2 Parts with different attribute maps, verify scoring
### Fix 3: Loosen Rust Float Epsilon to Match Lua
- **Status:** Approved (Modified: loosen Rust to match Lua)
- **Finding:** Step 11l: Matching Algorithm Parity -- Float epsilon
- **Files:** `src/variant_eq.rs`
- **Problem:** Lua `trueEquals` uses epsilon=0.0001 with absolute+relative check. Rust `variant_eq` uses `float_cmp::approx_eq!` defaults (epsilon~1.19e-7, ulps=4), which is ~1000x tighter. This means Rust considers values different that Lua considers equal, potentially causing different match scores and different pairings.
- **Solution:**
- Replace the `approx_eq!` macro wrapper in `variant_eq.rs` with a custom `fuzzy_eq` function that matches Lua's dual epsilon approach:
```rust
const EPSILON: f32 = 0.0001;
fn fuzzy_eq_f32(a: f32, b: f32) -> bool {
if a.is_nan() && b.is_nan() { return true; }
let diff = (a - b).abs();
let max_val = a.abs().max(b.abs()).max(1.0);
diff < EPSILON || diff < max_val * EPSILON
}
```
- Apply similar `fuzzy_eq_f64` for f64 comparisons.
- Update all float comparison call sites in `variant_eq` (Color3, CFrame, Vector3, UDim, etc.) to use the new function instead of `approx_eq!`.
- Remove the `float-cmp` dependency from `Cargo.toml` if no other code uses it.
- **Risk:** Medium. Loosening epsilon means values that were previously considered different are now equal. This affects:
- `variant_eq` used in matching (intended -- this fix)
- `variant_eq` used in `patch_compute.rs` for property diff detection (side effect -- could suppress legitimate property patches for near-equal values)
- Verify that the 0.0001 epsilon doesn't suppress meaningful Studio property changes.
- **Verify round-trip:** Float properties should round-trip without phantom diffs (the looser epsilon should REDUCE phantom diffs, not create them).
- **Verify syncback parity:** N/A
- **Tests required:**
- Rust unit test: update existing `variant_eq` float tests to verify the new epsilon behavior
- Rust unit test: `fuzzy_eq_matches_lua` -- verify same boundary values as `trueEquals.spec.lua` (0.0001 threshold, relative scaling, NaN handling)
### Fix 4: Add Sort to Forward Sync `build_result` for Consistency
- **Status:** Approved
- **Finding:** Code quality -- inconsistent sort between implementations
- **Files:** `src/snapshot/matching.rs`, `src/syncback/matching.rs`
- **Problem:** Syncback `build_result` (line 217) sorts `matched_indices` by new_ref index. Forward sync `build_result` (line 206) does NOT sort. The Lua `matchChildren` returns matched pairs in insertion order (which is group iteration order, then sorted within groups).
- **Solution:**
- Add `matched_indices.sort_by_key(|&(si, _)| si);` to the forward sync `build_result` function (line 203, before the matched Vec construction).
- Remove the TODO comment from the syncback version (line 217): the sort is intentional for consistent downstream iteration order.
- **Risk:** Very low. Only changes the ORDER of matched pairs in the result, not WHICH pairs are matched. Downstream code iterates matched pairs for patch computation, and order should not affect correctness.
- **Verify round-trip:** N/A -- order of matched pairs doesn't affect round-trip identity.
- **Verify syncback parity:** N/A
- **Tests required:**
- No new tests needed. Existing tests verify correct pairings, not ordering.
### Fix 5: Cache VCache at Session Level in Lua Matching
- **Status:** Approved
- **Finding:** Performance -- redundant `cacheVirtual` calls
- **Files:** `plugin/src/Reconciler/matching.lua`
- **Problem:** `cacheVirtual` is called during ambiguous group scoring (line 492) AND again inside `computePairCost` (line 362). The same virtual instance's properties are decoded multiple times per session.
- **Solution:**
- Add a `vCacheStore: { [string]: VCache }` field to `MatchingSession`.
- In `cacheVirtual`, check `session.vCacheStore[virtualId]` first. If present, return cached. Otherwise, compute and store.
- Update `computePairCost` to use the session-level cache instead of re-calling `cacheVirtual`.
- The `classKeys` parameter varies by group, but for a given virtualId the classKeys is always the same (determined by ClassName). So caching by virtualId is safe.
- **Risk:** Very low. Pure performance optimization. The VCache content is deterministic for a given virtualId.
- **Verify round-trip:** N/A
- **Verify syncback parity:** N/A
- **Tests required:**
- No new tests needed. Existing matching.spec.lua tests verify correctness. Could add a timing test but not required.
### Fix 6: Add Parity Tests Between All 3 Implementations
- **Status:** Approved
- **Finding:** Steps 5-6: Missing parity and round-trip tests
- **Files:** `tests/tests/matching_fixtures.rs` (extend), `plugin/src/Reconciler/matching.spec.lua` (extend)
- **Problem:** No tests verify that the 3 implementations produce identical pairings for the same input. Given Fixes 1-3 close the scoring gaps, parity tests are needed to prevent future divergence.
- **Solution:**
- **Rust integration test** (`tests/tests/matching_fixtures.rs`): Add a test `parity_forward_vs_syncback` that:
1. Creates a WeakDom with an ambiguous group (e.g., 4 Parts named "Line" with different Transparency values, in reversed order)
2. Runs syncback matching on `(new_children, old_children)`
3. Builds a RojoTree from `new_children` snapshots, runs forward sync matching on `(old_children_as_snapshots, tree_children)`
4. Verifies both produce the same pairings (by comparing matched Name+Property values)
- **Lua spec test** (`matching.spec.lua`): Add a test `parity_with_rust_fixture` that:
1. Uses the same input data as the Rust test (4 Parts with specific Transparency values)
2. Runs Lua matching
3. Verifies the same pairings as the Rust tests (by property comparison)
- **Round-trip matching test** (`tests/tests/matching_fixtures.rs`): Add `round_trip_matching_identity`:
1. Uses the UFOWave fixture (12 Textures with default-omitted Face)
2. Runs syncback to produce filesystem
3. Runs forward sync on the filesystem output
4. Verifies all 12 Textures are matched to their correct counterparts (by Face value)
- **Risk:** None. Tests only.
- **Verify round-trip:** This IS the round-trip test.
- **Verify syncback parity:** N/A (these are matching-level tests, not filesystem-level)
- **Tests required:** (this fix IS the test)
## Skipped Fixes
None. All identified fixes were approved.
## Accepted Limitations
### Greedy Matching Not Globally Optimal
- **Limitation:** The greedy assignment algorithm is not globally optimal. Edge cases exist where a locally-best pick forces a globally-worse assignment.
- **Scenario:** Large ambiguous groups (10+ instances) with specific cost patterns where the Hungarian algorithm would produce different (better) pairings.
- **User decision:** Accepted for all releases (documented in `atlas-dedup.mdc`).
- **Mitigation:** Ambiguous groups are typically small (2-10 instances). The depth limit (MAX_SCORING_DEPTH=3) bounds worst-case performance. Monitoring via test coverage for large groups.
## Deferred Refactors
- **Shared matching core:** The 3 implementations share the same algorithm but differ in input types (InstanceSnapshot vs WeakDom vs Studio Instance). A generic/trait-based approach could consolidate the duplicated logic (~2700 total lines), but the type differences are fundamental to each context. Estimated scope: 10+ files, significant API redesign. Deferred.
- **Duplicated grouping pattern:** All 3 implementations repeat the same HashMap-based `(Name, ClassName) -> Vec<index>` grouping. Could be factored into a shared helper, but types differ. Minor DRY violation. Deferred.
## Test Plan
### Rust Unit Tests (`#[cfg(test)]` blocks)
- `ref_scoring_forward_sync` -- 2 Models with different PrimaryPart targets, verify correct pairing (Fix 1)
- `ref_scoring_syncback` -- same scenario for syncback matching (Fix 1)
- `tags_granular_scoring` -- 2 Parts with different tag sets, verify scoring (Fix 2)
- `attributes_granular_scoring` -- 2 Parts with different attribute maps, verify scoring (Fix 2)
- Update existing `variant_eq` float tests for new epsilon behavior (Fix 3)
- `fuzzy_eq_matches_lua` -- verify same boundary values as `trueEquals.spec.lua` (Fix 3)
### Rust Integration Tests (`tests/tests/`)
- `parity_forward_vs_syncback` -- same ambiguous group through both Rust matchers, verify identical pairings (Fix 6)
- `round_trip_matching_identity` -- UFOWave fixture through syncback then forward sync, verify correct pairings (Fix 6)
### Lua Spec Tests (`.spec.lua` files)
- `parity_with_rust_fixture` -- same input as Rust parity test, verify identical pairings (Fix 6)
### Snapshot Tests (`insta` crate)
- None needed for matching fixes.
### Failing Tests for Known Limitations
- None. The greedy limitation is accepted and no test is needed to demonstrate it.
## Test Rules
The implementer MUST follow these rules when writing tests for this plan:
### For bugs found and fixed
Every bug fix MUST have a test that:
1. Would have **FAILED** before the fix
2. **PASSES** after the fix
3. Prevents the bug from regressing
### For missing coverage identified
For each "missing test coverage" gap:
1. Write the test
2. Verify it passes against the current implementation
3. If it fails, investigate -- it may have found another bug
### For known limitations (Accepted Limitations section)
No known-limitation tests needed for this plan.
### Scope: test every affected layer
For each finding, determine which test layers are affected and write tests in ALL of them:
- Does the bug manifest at the plugin level? -> Lua spec test
- Does it manifest at the server level? -> Rust unit test or integration test
- Does it affect the round-trip? -> Integration test with `/api/write` + filesystem verification
- Does it affect forward-sync? -> Serve test with `recv_socket_packet`
- Could it regress from a different code path? -> Test that code path too
## Final Step: Run CI
After ALL fixes and tests are implemented, run the `/ci` command (see `.cursor/commands/ci.md`) to execute the full CI pipeline. Every fix must pass CI before the plan is considered complete. Do not skip this step.