### Summary
When two or more sibling instances share the same `Name` and
`ClassName`, Rojo's reconciler previously paired them with their
server-side counterparts purely by child order (first-unvisited match in
`GetChildren()` order). If the workspace child order ever diverged from
the server's, the wrong instance got paired so each duplicate could
inherit a sibling's properties.
This is the root of the #1257 bug: the welded parts would oscillate
between positions on each connect/disconnect because hydration kept
mis-pairing them. (#1265 stopped the sync fallback from scrambling child
order in the first place but this PR makes hydration robust even when
order *does* diverge.)
This PR makes `hydrate` break ties by comparing properties: when several
existing children match on `Name`+`ClassName`, it scores each candidate
by how many of the virtual instance's properties match the candidate's
live values, and picks the best. Order remains the tiebreak when scores
are equal, so behavior is unchanged for uniquely-named instances and for
indistinguishable siblings.
### Changes
- `trueEquals.lua`: extracted verbatim from `diff.lua` (the fuzzy
value-equality helper) so it can be shared. No behavior change;
`diff.lua` now requires it.
- `countMatchingProperties.lua`: added
`countMatchingProperties(instance, virtualInstance, instanceMap) ->
number`. Skips `Ref` properties (the instanceMap isn't fully built
mid-hydrate, so refs can't be decoded reliably, and they're a poor
disambiguator anyway) and any property that can't be read or decoded.
- `hydrate.lua`: See details below.
### Hydrate Changes
This touches `hydrate`, which runs over the whole tree on every
connect/resync, so I want state clearly that **the common path is faster
than before, not slower** even for parents with thousands of children!
The old algorithm was a nested scan: for each of `V` virtual children,
scan existing children until the first unvisited `Name`+`ClassName`
match. Two costs stand out:
- A `pcall` (to guard DataModel permission errors) ran on every
comparison (up to `V*E` `pcall`s per parent).
- Even for in-order trees the re-scanning of the visited prefix made it
`O(V^2)`.
The new algorithm does a single bucketing pass, then `O(1)` lookups:
1. One `O(E)` pass groups existing children into nested
`buckets[name][className]` tables. This runs exactly `E` `pcall`s total
(one per child), down from the `V*E` worst case.
2. Each virtual child does an `O(1)` bucket lookup to find its
candidates.
3. A per-bucket cursor skips already-paired children, so order-based
matching is amortized `O(1)` per child instead of rescanning.
| Scenario | Old | New |
| ------------------------------------------------ |
--------------------------------------- |
--------------------------------------------- |
| Unique-named children (typical, incl. thousands) | `O(V^2)`, plus up
to `V*E` pcalls | `O(V + E)`, plus exactly `E` pcalls |
| `C <= 32` candidates | `O(C^2)` | `O(P * C^2)` scoring |
| `C > 32` candidates | `O(C^2)` | `O(C)` |
Property scoring (`getProperty`/`decodeValue`/`trueEquals`) is the only
new expense, and it's gated two ways:
- It runs only when a `Name`+`ClassName` group has >=2 candidates (i.e.
never for uniquely-named instances).
- A cap, `MAX_CANDIDATES_TO_SCORE = 32`, means scoring only kicks in
once a group has <=32 unvisited candidates. A folder of thousands of
identically-named parts therefore does not trigger scoring; it falls
back to the original order-based pairing. The worst-case added scoring
work is bounded to roughly `32^2` property comparisons per group,
independent of group size.
So overall this is faster when you have unique names or many children.
It is slower but more robust when you have small groups of duplicate
names. Memory usage is increased as it creates the candidate buckets.
* Add user confirmation to initial sync
* Use "Accept" instead of "Confirm"
* Draw tree alphabetically for determinism
* Add diff table dropdown
* Add diff table to newly added objects
* Unblock keybind workflow
* Only show reject button when two way is enabled
* Try to patch back to the files when changes are rejected
* Improve text spacing of the prop diff table
* Skip user confirmation of perfect syncs
* Give instances names for debugging UI
* Optimize tree building
* Efficiency: dynamic virtual scrolling & lazy rendering
* Simplify virtual scroller logic and avoid wasteful rerenders
* Remove debug print
* Consistent naming
* Move new patch applied callback into accept
* Pcall archivable
* Keybinds open popup diff window
* Theme rows in diff
* Remove relic of prototype
* Color value visuals and better component name
* changeBatcher is not needed when no sync is active
* Simplify popup roact entrypoint
* Alphabetical prop lists and refactor
* Add a stroke to color blot for contrast
* Make color blots animate transparency with the rest of the page
* StyLua formatting on newly added files
* Remove wasteful table
* Fix diffing custom properties
* Display tables more meaningfully
* Allow children in the button components
* Create a rough tooltip component
* Add tooltips to buttons
* Use provider+trigger schema to avoid tooltip ZIndex issues
* Add triangle point to tooltip
* Tooltip underneath instead of covering
* Cancel hovers when unmounting
* Allow multiple canvases from one provider
* Display above or below depending on available space
* Move patch equality to PatchSet.isEqual
* Use Container
* Remove old submodules
* Reduce false positives in diff
* Add debug log
* Fuzzy equals CFrame in diffs to avoid floating point in
* Fix decodeValue usage
* Support the .changedName patches
* Fix content overlapping border
* Fix tooltip tail alignment
* Fix tooltip text fit
* Whoops, fix it properly
* Move PatchVisualizer to Components
* Provide Connected info with full patch data
* Avoid implicit nil return
* Add patch visualizer to connected page
* Make Current column invisible when visualizing applied patches
* Avoid floating point diffs in a numbers and vectors
* Switch git submodules to Wally packages
* Update build snapshot
* Add wally to foreman and use latest versions
* Install packages in CI runners
* Fix indents
* Install packages in the correct directory
* Install packages in correct dir of release action too
* Remove submodules from ci checkout
* Remove submodules from release checkout
* Update selene with latest fix
* Fix whitespace
Co-authored-by: Lucien Greathouse <me@lpghatguy.com>
* Start splitting apart reconciler, with tests
* Reify children in reify
* Baseline hydrate implementation
* Remove debug print
* Scaffold out diff implementation, just supporting name changes
* invariant -> error in decodeValue
* Flesh out diff and add getProperty
* Clear out top-level reconciler interface, start updating code that touches it
* Address review feedback
* Add (experimental) Selene configuration
* Add emptiness checks to PatchSet, remove unimplement invert method
* Improve descendant destruction behavior in InstanceMap
* Track instanceId on all reify errors
* Base implementation of applyPatch, returning partial patches on failure
* Change reify to accept InstanceMap and insert instances into it
* Start testing applyPatch for removals
* Add test for applyPatch adding instances successfully and not
* Add , which is just error with formatting
* Correctly use new diff and applyPatch APIs
* Improve applyPatch logging and fix field name typo
* Better debug output when reify fails
* Print out unapplied patch in debug mode
* Don't write properties if their values are not different.
This was exposed trying to sync the Rojo plugin, which
has a gigantic ModuleScript in it with the reflection
database. This workaround was present in some form in
many versions of Rojo, and I guess we still need it.
This time, I actually documented why it's here so that
I don't forget for the umpteenth time...
* Add placeholder test that needs to happen still
* Introduce easier plugin testing, write applyPatch properties test
* Delete legacy get/setCanonicalProperty files
* Fix trying to remove numbers instead of instances
* Change applyPatch to return partial patches instead of binary success
* Work towards being able to decode and apply refs
* Add helpers for PatchSet assertions
* Apply refs in reify, test all cases
* Improve diagnostics when patches fail to apply
* Stop logging when destroying untracked instances, it's ok
* Remove read before setting property in applyPatch
* Fix diff thinking all properties are changed
