feat: forced wearables debug tool - #10160
Conversation
Renders wearables on the own avatar without owning them, so an unpublished or locally converted wearable can be checked in-world. ForcedWearables holds the session's set: SelfProfile applies it to every profile it hands out (what the player entity, and therefore the avatar, is built from) and strips it from anything about to be deployed. Seeded the same way forced emotes are - --self-force-wearables or the DebugSettings array - plus a "Forced Wearables" debug widget that edits the set at runtime, with the avatar rebuilt on change. The strip covers everything forced this session, not just the current set: un-forcing does not un-equip, and the backpack copies the player entity's wearables into IEquippedWearables, from where they would otherwise be deployed. It sits in UpdateProfileAsync, the only path that reaches IProfileRepository.SetAsync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🚦 CI StatusWindows and Mac built successfully in Unity Cloud.
Warnings count reduced: 11870 => 11858 Warnings/errors in files changed by this PR (24)Lint run · full InspectCode report · took 26m 14s All Unity tests passed ✅
Tests time sums the test cases; Job time is the job's wall clock including checkout, licensing and asset import. Slowest tests
Full report: run summary · results + editor logs: editmode · playmode 🏁 Bare-metal benchmark finished — run #35874600659. Full reportPR #10160, run #35874600659 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
On demand — comment |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: forced wearables debug tool
Step 2 — Root-cause check
New feature (debug tool for wearing wearables without owning them). Not a fix — no root-cause concern. PASS
Step 3 — Design & integration
Owner search. ForcedWearables manages a session-scoped set of URNs applied to the self-profile. Searched:
SelfProfile— owns profile fetch/deploy. Cannot host the data: the debug widget also callsAdd/Remove/Clearindependently.ProfileContainer.Create— composition root that wires bothSelfProfileand the widget. Right place for construction.- No existing manager covers "session-only faked wearables" —
IOwnedNftFilter,SELF_PREVIEW_WEARABLES, andIEquippedWearablesare unrelated concerns.
ForcedWearables earns its existence: two independent callers (SelfProfile and the debug widget), a real invariant (everForced ⊇ wearables), and a Changed event. Inlining would scatter the state, not simplify it.
Teardown trace:
forcedWearables.Changed += RebuildAvatarWithForcedWearables(SelfProfile.cs:66) — subscribed in constructorforcedWearables.Changed -= RebuildAvatarWithForcedWearables(SelfProfile.cs:73) — unsubscribed in DisposeProfileContainer.Dispose()→SelfProfile.Dispose()— symmetric ✅
ProfilePropagated deliberately skipped. RebuildAvatarWithForcedWearables calls UpdateAvatarInWorld but not ProfilePropagated. Correct — ProfilePropagated feeds MultiplayerContainer.OnSelfProfilePropagated, which propagates to other players. Forced wearables are local-only by design.
Security invariant verified. ProfileBuilder.Build() copies URNs element-by-element into a fresh Avatar.wearables set, so ApplyTo/RemoveFrom mutating the profile cannot cross-contaminate other cached/pooled profiles. UpdateProfileAsync strips before every SetAsync, and TryGetBackingSet gates both apply and strip — if the cast fails, neither can execute, so the invariant holds: nothing applied → nothing to strip. The debug widget is excluded from non-debug builds by DebugUtilitiesContainer's allowedCategories gate.
Design: PASS
Step 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
ForcedWearables.Wearables (line 29) |
0 | Unused — see inline comment |
ForcedWearables.Add/Remove/Clear |
Debug widget (ProfileContainer.cs:159-161) | OK — single consumer, but exposed for the widget |
ForcedWearables.ApplyTo/RemoveFrom |
SelfProfile (4 call sites) | OK |
ForcedWearables.Changed |
SelfProfile (subscribe/unsubscribe) | OK — symmetric |
RebuildAvatarWithForcedWearables |
Changed event only |
OK — private |
Step 5 — Line-level findings
See inline comments below. Summary:
| # | Sev | Rule | File | Finding |
|---|---|---|---|---|
| 1 | P1 | R22 | (repo-wide) | No tests for ForcedWearables |
| 2 | P2 | R13 | ProfileContainer.cs | Parsing helpers duplicate the emotes ones |
| 3 | P2 | R15 | ForcedWearables.cs:29 | Wearables property has zero consumers |
| 4 | P2 | R23 | SelfProfile.cs:135 | Comment narrates backpack behavior |
| 5 | P2 | R7 | ForcedWearables.cs:100 | null! in TryGetBackingSet out-param — justified (Try-pattern) |
Categories checked clean: R1 perf-alloc (all paths are boot-only or user-triggered, not per-frame), R2 LINQ (mirrors existing emotes pattern at boot, not hot path), R4 ECS lifecycle (no ECS systems introduced), R5 entity by-ref (no entity manipulation), R6 teardown (symmetric), R7 nullability (sound), R8 root-cause (n/a — new feature), R9 logging (ReportHub.LogError with ReportCategory.PROFILE — correct), R10 catch-scope (no new catch blocks), R11 async (no new async code), R12 single-impl (no new interfaces), R14 contract honesty (OK), R16-R19 naming/hygiene (OK), R20 idiom (no Concurrent*, readonly on fields — correct), R21 MVC (n/a), R24 scope (all files relate to the stated change).
Step 6 — Complexity
COMPLEX — new class with state management, touches profile lifecycle (SelfProfile constructor + UpdateProfileAsync), modifies the profile deploy path, adds debug widget integration.
Step 7 — QA assessment
QA_REQUIRED: YES — runtime code under Explorer/, affects avatar rendering and profile deploy paths.
Step 8 — Non-blocking warnings
No Main.unity modifications. No warnings.
Merge gates
R25 — Outstanding comments: This is the first review. 5 inline findings posted below (1× P1, 4× P2).
R26 — QA sign-off: Awaiting QA. PR does not carry no QA needed. Test instructions are present and followable.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: New stateful class touching profile lifecycle, deploy path, and debug widget integration
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
- Add ForcedWearablesShould: apply/strip round-trip, the everForced retention that keeps an un-forced wearable strippable, URN shortening, empty-URN rejection, Changed firing only on real mutations, and a pin on Avatar.Wearables' backing type so the cast cannot regress silently (R22). - Fold the wearable and emote URN parsing into one pair of helpers (R13). - Drop the unused ForcedWearables.Wearables property (R15). - Trim comments that narrated backpack and debug-widget behaviour (R23). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Launch arguments and the editor debug settings cover the use case, so the widget and its debug category go. Without a runtime caller the mutation API had no consumers: Add/Remove/Clear and the Changed event are gone, and with them SelfProfile's subscription and avatar rebuild. The set is fixed at construction, so tracking what was forced earlier in the session no longer says anything the current set does not - everForced collapses into it. ForcedWearables stays a type rather than a plain IReadOnlyList<URN> for the test seam: the strip is the safety property worth covering, and reaching it through SelfProfile would need the repository, identity cache and world. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Heads-up on a design change since the review: the debug widget is gone (9bcf63d). Launch arguments and the editor debug settings cover the use case, so the panel entry and its That invalidates part of the Step 3 design note, which justified
PR description and test steps updated to match. |
Drop 6 redundant usings, 4 redundant null-forgiving operators, declare the lazily-created ScreenRecorderTester.recorder nullable, and initialize DebugSettings.appParameters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #10160: feat: forced wearables debug tool
STEP 2 — Root-cause check: PASS
The PR solves a real problem: there is no way to check an unowned wearable (e.g. a locally converted asset bundle) on your own avatar. The approach is direct — inject into the profile on read, strip before deploy — rather than a workaround. Not a symptom fix.
STEP 3 — Design & integration
MANDATORY OWNER SEARCH — ForcedWearables:
- It manages the lifecycle of a session-scoped set of wearable URNs applied to and stripped from profiles.
- The existing profile lifecycle is owned by
SelfProfile(construction viaProfileContainer.Create, disposal viaProfileContainer.Dispose). The profile fetch path (ProfileAsync) and deploy path (UpdateProfileAsync → IProfileRepository.SetAsync) are both inSelfProfile. ForcedWearablescould be inlined intoSelfProfile, but it earns its existence through the test seam:ForcedWearablesShouldtests apply/strip logic in isolation without constructing the fullSelfProfiledependency tree. TheTryGetBackingSetcast + error path is non-trivial and worth covering independently.
Teardown trace:
SelfProfilesubscribes toweb3IdentityCache.OnIdentityClearedandOnIdentityChangedin constructor (lines 64-65), unsubscribes inDispose(lines 70-71). ✓ForcedWearableshas no subscriptions, no disposables, no teardown needed. ✓- No new subscriptions, events, or pools in the diff.
R13 consolidation: The diff refactors ParseDebugForcedEmotes/ParseParamsForcedEmotes into generic ParseDebugUrns/ParseParamsUrns. The old ParseParamsForcedEmotes had a dead ?. on string.Split() and a ?? ArraySegment<URN>.Empty fallback (Split never returns null). The refactored version correctly drops both. Behavior-preserving.
Assembly boundary note: Avatar.wearables is internal to DCL.SharedAPI. ForcedWearables reaches the backing HashSet<URN> through a cast on the public IReadOnlyCollection<URN> view. This is fragile by nature, but the PR defends it well: TryGetBackingSet reports an error rather than silently no-oping, and the KeepAvatarWearablesBackedByAHashSet test pins the backing type in CI.
STEP 4 — Member audit
New public members in ForcedWearables:
ApplyTo(Profile)— called in 3 places inSelfProfile(ProfileAsync, and both success paths of UpdateProfileAsync). Correct.RemoveFrom(Profile)— called in 1 place inSelfProfile(UpdateProfileAsync before SetAsync). Correct.- Constructor — called in 3 places:
ProfileContainer.Create,ScreenRecorderTester.CreateProfile,SelfProfilePlayground. All correct.
No single-use derived predicates, no redundant guards.
STEP 5 — Line-level findings
See inline comments below.
STEP 6 — Complexity: COMPLEX
Touches profile fetch/deploy pipeline, adds a new type with safety-critical strip-before-deploy logic, modifies SelfProfile constructor and both deploy paths, refactors shared parse helpers.
STEP 7 — QA: YES
Runtime code change affecting avatar rendering and profile deployment. Testable on both platforms.
STEP 8 — Non-blocking warnings
No main scene modification. ✓
Review-bar rule coverage
| Category | Status |
|---|---|
| R1/R2 perf-alloc | Clean — LINQ in ParseDebugUrns/ParseParamsUrns is startup-only |
| R3 class-in-struct | N/A |
| R4 ECS lifecycle | Clean — no per-frame work, no new systems |
| R5 Entity by-ref | N/A |
| R6 Acquire/release | Clean — no new subscriptions or disposables |
| R7 Nullability | P2 on null! out param (see inline) |
| R8 Root cause | PASS |
| R9 Logging | Clean — ReportHub.LogError with correct ReportCategory.PROFILE |
| R10 Catch-scope | N/A — no new catches |
| R11 Async/CT | N/A — no new async |
| R12 Abstraction | PASS — ForcedWearables earns its test seam |
| R13 Reuse | PASS — parse helpers consolidated |
| R14 Contract honesty | Clean |
| R15 Dead weight | Clean — unused imports removed |
| R16/R17 Naming | Clean |
| R18 Magic numbers | Clean |
| R19 File hygiene | Clean |
| R20 Idiom cluster | Clean — readonly on fields ✓ |
| R21 MVC | N/A |
| R22 Tests | P0 — 3 tests fail in CI |
| R23 Comments | P2 — see inline |
| R24 Scope | Clean — all files relate to forced wearables |
Merge gates
- R25 — Outstanding comments: 5 prior review comments from
decentraland-bot, all addressed by the author in subsequent commits. 0 unresolved threads. - R26 — QA sign-off: Awaiting QA. No
no QA neededlabel. PR description includes detailed test instructions.
REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies profile fetch/deploy pipeline in SelfProfile, adds safety-critical strip-before-deploy logic, refactors shared URN parse helpers.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
URN converts implicitly to string, so Does.Contain(new URN(...)) bound to the string overload and compared URN elements against a string. The positive assertions always failed and the negative ones passed vacuously. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Trim the class summary and the strip comment to what the annotated code itself does, and declare TryGetBackingSet's out parameter as nullable with NotNullWhen(true) instead of assigning null!. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: forced wearables debug tool (Round 3)
STEP 2 — Root-cause check: PASS
New feature: adds a debug mechanism for rendering wearables on the own avatar without owning them (e.g. verifying a locally converted asset bundle). Not a fix — no root-cause concern.
STEP 3 — Design & integration: PASS
Owner search. ForcedWearables manages a session-scoped set of wearable URNs to inject/strip on the self profile.
- Profile creation/deployment owner:
SelfProfile— ownsProfileAsync(read) andUpdateProfileAsync(deploy). Both are the natural home for apply/strip. Confirmed:ForcedWearables.ApplyTois called inProfileAsync(line 87) and at both post-save sites (lines 166, 194).ForcedWearables.RemoveFromis called inUpdateProfileAsync(line 134), immediately before the deploy path.IProfileRepository.SetAsyncis called only fromSelfProfile— no unguarded deploy path exists. - Construction owner:
ProfileContainer.Create(composition root) — constructsForcedWearablesfrom app args and debug settings, injects it intoSelfProfile. Correct placement. - Lifecycle: Fixed at construction, read-only thereafter. No subscriptions, no event hookups, no disposables. No teardown needed.
Should ForcedWearables be a separate class? Yes. One consumer (SelfProfile), but it encapsulates the safety-critical strip-before-deploy invariant and provides the test seam through which ForcedWearablesShould validates apply/strip round-trips, extended URN shortening, and the backing-type pin — all without needing the full SelfProfile dependency graph (repository, identity cache, world). R12 is satisfied by the test seam justification.
Asymmetry with forced emotes: Forced emotes are applied inline in ProfileAsync (slot replacement) with no strip before deploy — emotes don't require the same safety gate because CreateNewProfileForUpdate rebuilds emotes from IEquippedEmotes, not from the profile's emote array. Forced wearables follow a different path (Backpack copies URNs into IEquippedWearables, from where CreateNewProfileForUpdate would carry them into the deployed profile), so the strip is necessary and justifies the dedicated type.
TryGetBackingSet cast: Avatar.wearables is internal readonly HashSet<URN> (Avatar.cs:13), exposed as public IReadOnlyCollection<URN> Wearables (Avatar.cs:21). The internal field is scoped to the SharedAPI assembly and not reachable from the Profiles assembly. The cast through IReadOnlyCollection is pragmatic given the assembly boundary; the KeepAvatarWearablesBackedByAHashSet test (line 114) pins it in CI, and ReportHub.LogError surfaces a backing-type change at runtime. The symmetric failure property (both ApplyTo and RemoveFrom use the same cast) means a backing-type change disables both injection and stripping — forced wearables cannot enter a profile that can't be stripped.
STEP 4 — Member audit
| Member | Consumers | Verdict |
|---|---|---|
ForcedWearables.ApplyTo(Profile) |
3 (SelfProfile: lines 87, 166, 194) | Justified — injects at read time and after both save paths |
ForcedWearables.RemoveFrom(Profile) |
1 (SelfProfile: line 134) | Justified — the deploy-time strip is the safety gate |
TryGetBackingSet (private) |
2 (ApplyTo, RemoveFrom) | Justified — shared cast + error reporting |
DebugSettings.WearablesToAddToUserProfile |
1 (ProfileContainer.Create) | Mirrors EmotesToAddToUserProfile |
AppArgsFlags.FORCED_WEARABLES |
1 (ProfileContainer.Create) | Mirrors FORCED_EMOTES |
No single-use-then-merge candidates. No absent-vs-false confusion. No redundant guards.
STEP 5 — Line-level findings
See inline comments below. Two P2 findings.
STEP 5b — Review bar (R1–R26)
Lint pre-flight: scripts/lint/custom-rules.sh not present on this branch — rules checked by hand.
| Category | Result |
|---|---|
| R1 perf-alloc | Clean — no hot-path code added. LINQ in ParseDebugUrns/ParseParamsUrns runs once at startup |
| R2 no-LINQ | Clean — LINQ is in container construction only |
| R3 class-in-struct | Not applicable |
| R4 ECS discipline | Clean — ForcedWearables is a plain service, no systems or queries |
| R5 entity by-ref | Not applicable |
| R6 acquire/release | Clean — no subscriptions, connections, or disposables added |
| R7 nullability | One P2 finding (see inline) |
| R8 root cause | PASS (new feature) |
| R9 logging | Clean — ReportHub.LogError with ReportCategory.PROFILE |
| R10 catch-scope | Not applicable (no new catches) |
| R11 async/CT | Clean — no new async flows introduced |
| R12 abstraction | ForcedWearables justified by test seam (see Step 3) |
| R13 reuse | Clean — ParseParamsUrns/ParseDebugUrns generalization removes duplication |
| R14 contract honesty | The IReadOnlyCollection → HashSet cast is technically a contract violation, but pragmatically justified by the assembly boundary and pinned by CI test |
| R15 dead weight | Clean — unused imports removed across 3 files, no dead members remain |
| R16/R17 naming | Clean — PascalCase, CQRS verbs (ApplyTo/RemoveFrom/TryGetBackingSet) |
| R18 magic numbers | None |
| R19 file hygiene | Clean — one class per file, correct placement |
| R20 idiom cluster | Clean — readonly fields, no concurrent collections, no Debug.Log |
| R21 MVC | Not applicable |
| R22 tests | 8 tests covering apply, strip, cross-path strip, URN shortening, empty URN filtering, no-op, and backing-type pin. One minor gap noted inline. |
| R23 comments | One P2 finding (see inline) |
| R24 scope | Clean — all files relate to the forced wearables feature or the shared URN parsing generalization |
STEP 6 — Complexity: COMPLEX
Modifies profile infrastructure (SelfProfile, ProfileContainer), introduces a new dependency (ForcedWearables), and touches the async profile deployment path.
STEP 7 — QA assessment: YES
Runtime code that affects avatar rendering and profile deployment.
STEP 8 — Non-blocking warnings
No warnings. Main scene not modified.
Merge gates
- R25 — Open threads: All 11 prior bot comments have author replies with fixes (commits d972016, f121641, 26c15be). 0 unresolved prior threads. This review adds 2 new P2 inline comments.
- R26 — QA: No
no QA neededlabel. PR carries detailed, followable test instructions with a concrete wearable URN and step-by-step verification. Testable on both platforms in the same build. Awaiting QA.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async profile deployment paths (SelfProfile.UpdateProfileAsync), profile container wiring, and introduces cross-assembly type dependency (Avatar.Wearables backing set)
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
A session with forced wearables (or emotes) no longer deploys a profile at all, replacing the strip-before-deploy path. Every deploy route builds its profile from one ProfileAsync handed out, so suppressing the deploy covers all of them at once, mirroring how --self-preview-wearables already suppresses backpack publishing. Drops ForcedWearables.RemoveFrom and both post-deploy re-applies. Also closes the pre-existing forced-emote leak, which had no strip counterpart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both Mac and Windows test steps work as expected using the latest build ✅ Note: The name change takes effect, but somehow it takes a while longer to appear in the nametag; the name change can be seen in the profile.
Mac PR.10160.name.change.mp4PR.10160.name.change.mp4Windows 23.09.2026_16.46.25_REC.name.change.+.no.args.windows.mp4 |
4 participants
Pull Request Description
What does this PR change?
Renders a wearable on your own avatar without owning it, so an unpublished or locally re-converted wearable can be checked in-world. Came out of verifying an abgen asset-bundle fix on a wearable the tester doesn't own.
ForcedWearablesholds the session's faked set.SelfProfile.ProfileAsyncapplies it to every profile it hands out — which is what the player entity, and therefore the avatar, is built from.While the set is non-empty, the session does not deploy a profile at all. Backpack saves, name changes and passport edits apply locally and are discarded on relaunch. Same approach
--self-preview-wearablesalready takes for backpack publishing. This also covers forced emotes, which leak into deployed profiles ondevtoday.Two ways in, mirroring the existing forced-emotes flags:
--self-force-wearables <csv>--self-force-emotesDebugSettings.wearablesToAddToUserProfileemotesToAddToUserProfileFixed at construction — no runtime mutation, no debug widget, no mid-session avatar rebuild.
Docs:
self-force-wearablesentry indocs/app-arguments.md.Test Instructions
Steps (standard run):
Expected result: without the flag, nothing changes — every apply is an early return and profiles deploy normally.
Prerequisites
Strawberry rabbit lolita dress (
upper_body, unmistakable at a glance):Test Steps
--self-force-wearables <urn above>. The avatar wears the dress from boot.Additional Testing Notes
Profile deploy skipped: forced wearables or emotes are activefires on every suppressed save — worth tailing so a silent no-op isn't mistaken for a bug.upper_bodyalready worn. Expected.…:0:105) is shortened on the way in — behaviour identical.SelfProfileconstructor;ScreenRecorderTesterandSelfProfilePlaygroundupdated.Quality Checklist
🤖 Generated with Claude Code