fix: spring bone root detection#56
Conversation
There was a problem hiding this comment.
Pull request overview
Refines spring/extra bone reparenting so wearable “extra” bones reliably follow the live avatar skeleton during emotes by using name-based matching rather than object identity.
Changes:
- Skip reparenting for transforms that correspond to standard avatar bones by name.
- Reparent extra bone chain roots under the live avatar bone by matching the parent transform’s name in the avatar bone map.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Vercel Preview is ready!
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
PR: #56 — fix: spring bone root detection
Branch: fix/spring-bones-issue → main
Files changed: 1 (+7 -12)
CI: ✅ All checks passing
Verdict: ✅ Approve
This is a clean, correct fix. The old identity-based checks (HashSet<Transform>.Contains) were fundamentally broken because wearable bone transforms are different instances from the live avatar bones — the identity checks would never match, so spring bones would never get reparented. Switching to name-based matching via the existing avatarBoneMap dictionary is the right approach.
The code is simpler (5 net lines removed, one allocation eliminated, one concept removed) and the guard-clause consolidation reads clearly.
Findings
P3 — Nice-to-Have
-
[Performance]
Transform.nameallocates a string per access in Unity — The oldHashSet<Transform>used identity-based lookups (zero string alloc). The new code accessestransform.nameandtransform.parent.nameper iteration, marshaling strings from native to managed memory. For typical wearable rigs (50–150 transforms) at equip time, this is negligible. If this ever becomes a hot path, you could cache the name in a local or build aHashSet<string>fromavatarBoneMap.Keysup front. -
[Robustness] Name collision edge case —
avatarBoneMap.ContainsKey(transform.name)will skip an extra bone that happens to share a name with a standard avatar bone. In practice this is unlikely for humanoid rigs, but if you want a tighter check you could verify identity too:if (avatarBoneMap.TryGetValue(transform.name, out var mapped) && mapped == transform) continue;
This skips only transforms that are actually the live avatar bone, not just name-matches.
Neither finding blocks merge. The before/after videos clearly demonstrate the fix working correctly. Nice work!
Requested by Rocío Corral Mena via Slack
Summary
Related PR: #55
This pull request refines the logic for reparenting extra bones (such as spring bones) in the
ReparentExtraBonesUnderAvatarSkeletonmethod. The update ensures that only transforms not corresponding to standard avatar bones are considered for reparenting, and that extra bones are more reliably reparented under the correct live avatar bones by matching names rather than relying on object identity.Bone reparenting improvements:
Screenshots
BEFORE:
Screen.Recording.2026-04-09.at.12.58.42.PM.mp4
AFTER:
Screen.Recording.2026-04-09.at.1.02.04.PM.mp4