fix: apply TriggerArea collisionMask and mesh updates after creation - #9809
fix: apply TriggerArea collisionMask and mesh updates after creation#9809alejandro-jimenez-dcl wants to merge 1 commit into
Conversation
## Problem A scene that mutates a live `TriggerArea` component — changing `collisionMask` or `mesh` after creation — sees no effect: trigger events keep firing against the original mask/mesh until the component is removed and re-added. The `CL_MAIN_PLAYER → wider mask` case is triply stale (component mask, mono `TargetTransform` filter, `gameObject.layer` routing). GitHub #6484 (stale-bot autoclosed; defect verified present at the pin). ## Root cause `TriggerAreaHandlerSystem.SetupTriggerArea` reads `PBTriggerArea` exactly once and bakes mask/mesh/targetOnlyMainPlayer into the component; `UpdateTriggerArea` never consumed the PB component's `IsDirty` — later PUTs from the scene were silently dropped. Both sibling area systems (`AvatarModifierAreaHandlerSystem`, `CameraModeAreaHandlerSystem`) already consume their dirty flags via the identical pattern; this system was the odd one out. ## Fix - `SDKEntityTriggerAreaComponent`: new `UpdateMaskAndMeshType(...)` (mirrors `UpdateAreaSize`); `TryAssignArea` rebinds/clears the mono's `TargetTransform` on every call (also closes a latent pooled-mono stale-filter leak). - `TriggerAreaHandlerSystem.UpdateTriggerArea`: consumes `pbTriggerArea.IsDirty` exactly like the siblings; on an actual mask change, emits synthetic `TaetExit`/`TaetEnter` for current insiders whose match status flips (Unity does not synthesize enter/exit on live filter swaps), skipping colliders still in the pending-ENTER queue. - `SDKEntityTriggerArea` (mono): `SetTargetTransform` evicts non-target insiders from all tracking sets when a filter binds (the synthetic EXITs already on the wire make the sets truthful), and `OnTriggerExit` only queues exits for colliders it actually tracked — no phantom ENTERs and no unbalanced EXITs across mask cycles. Residual documented edges only ever MISS an event (same-frame race class shared with the siblings); no path fabricates one. ## Test New EditMode `TriggerAreaLayerUpdateShould` (5 tests, existing harness): dirty-PB mask/mesh consumption, synthetic EXIT and ENTER emission, TargetTransform rebind on `CL_MAIN_PLAYER → CL_PLAYER`, and the mask-cycle eviction regression from the adversarial review. All 14 pre-existing `TriggerAreaHandlerSystemShould` tests stay green. ## Validation Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 5/5 as intended (no dirty consumption, no synthetic events, filter stays bound at pin) / GREEN PASS 5/5, sibling regression suite 14/14. Fixes #6484 Related: #6483 (CL_POINTER support — out of scope, unchanged), #3728 (same read-PB-once defect class in GLTFContainer, closed) Includes inspection-warning cleanup in all touched files.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9809: fix: apply TriggerArea collisionMask and mesh updates after creation
STEP 1 — Context & scope
Changed files (5):
TriggerAreaHandlerSystem.cs— dirty-flag consumption, synthetic event emission,ColliderMatchesMaskpredicate,(uint)cast fixesSDKEntityTriggerAreaComponent.cs—UpdateMaskAndMeshType,IsEnterPending,TryAssignArearefactor to rebindSetTargetTransformon every callSDKEntityTriggerArea.cs—SetTargetTransformwith eviction,OnTriggerExittracking guard,IsEnterPending,TargetTransformencapsulationTriggerAreaLayerUpdateShould.cs— 5 new EditMode testsTriggerAreaLayerUpdateShould.cs.meta— Unity meta
Surrounding context read: full TriggerAreaHandlerSystem.cs (base + head), full SDKEntityTriggerArea.cs (base + head), full SDKEntityTriggerAreaComponent.cs (base + head). Sibling systems confirmed: AvatarModifierAreaHandlerSystem and CameraModeAreaHandlerSystem both consume IsDirty on their PB components via the identical pattern this PR adopts.
STEP 2 — Root-cause check
Problem: UpdateTriggerArea never consumed PBTriggerArea.IsDirty, so scene-side PUTs that changed collisionMask or mesh after creation were silently dropped. Both sibling area systems already consume their dirty flags — this system was the odd one out.
Fix: Consumes IsDirty in UpdateTriggerArea, updates the component via UpdateMaskAndMeshType, and emits synthetic ENTER/EXIT events for insiders whose match status flips (Unity does not synthesize enter/exit on live filter swaps).
RESULT: PASS ✅ — Fixes the root cause, not a symptom. The dirty-flag consumption pattern is identical to the siblings.
STEP 3 — Design & integration
No new long-lived units introduced. All logic is placed within existing owners:
- Dirty-flag consumption →
TriggerAreaHandlerSystem.UpdateTriggerArea(the natural home, matching sibling systems) UpdateMaskAndMeshType→SDKEntityTriggerAreaComponent(mirrors existingUpdateAreaSize)SetTargetTransformwith eviction →SDKEntityTriggerAreamono (the existing owner ofTargetTransformand the tracking sets)
Owner search: The lifecycle for TriggerArea components is owned by TriggerAreaHandlerSystem (setup via SetupTriggerArea, destruction via HandleEntityDestruction/HandleComponentRemoval, finalize via FinalizeComponents). The mono is managed by SDKEntityTriggerAreaComponent.TryAssignArea (creation) and TryRelease (pooling). The dirty-flag consumption fits within the existing UpdateTriggerArea query — no new system, no new lifecycle manager.
Teardown/consumption trace:
isNotTargetEntity(Predicate<Collider>?) — cached delegate on theMonoBehaviourinstance, captured via??=; lives with the mono, no separate teardown needed.- No new subscriptions, event hookups,
+=, or connections introduced.
RESULT: PASS ✅
STEP 4 — Member audit
| New member | Consumers | Verdict |
|---|---|---|
SDKEntityTriggerAreaComponent.UpdateMaskAndMeshType(ColliderLayer, SDKEntityTriggerAreaMeshType) |
1 (UpdateTriggerArea) |
Named state-mutation method mirroring UpdateAreaSize — legitimate encapsulation, not a derived predicate. ✅ |
SDKEntityTriggerAreaComponent.IsEnterPending(Collider) |
1 (ReEvaluateEntitiesInside) |
Thin forwarding accessor centralizing access to the mono's pending-enter set. ✅ |
SDKEntityTriggerArea.IsEnterPending(Collider) |
1 (SDKEntityTriggerAreaComponent.IsEnterPending) |
Direct HashSet.Contains check. ✅ |
SDKEntityTriggerArea.SetTargetTransform(Transform?) |
1 (TryAssignArea) |
Replaces the direct public field assignment with a method that also performs eviction — legitimate encapsulation lift. ✅ |
SDKEntityTriggerArea.TargetTransform (field → property) |
Multiple (existing OnTriggerEnter/OnTriggerExit, new SetTargetTransform) |
Good encapsulation — private set prevents external mutation. ✅ |
RESULT: PASS ✅
STEP 5 — Line-level review
Pass A — Blocking-issue categories
Query attribute change ([All(typeof(PBTriggerArea), typeof(TriggerAreaComponent))] → [All(typeof(TriggerAreaComponent))] + in PBTriggerArea parameter): In Arch source-gen queries, component types in the method signature are automatically included in the query filter. Removing PBTriggerArea from [All] while adding it as a parameter is functionally equivalent and avoids redundancy. ✅
pbTriggerArea.IsDirty = false on an in parameter: PBTriggerArea is a Protobuf-generated class (reference type); in makes the reference read-only but the object mutable. The sibling systems use the identical pattern. ✅
ReEvaluateEntitiesInside iteration safety: Iterates CurrentEntitiesInside (the mono's HashSet<Collider>). The loop body calls only ColliderMatchesMask (pure predicate) and PropagateResultComponent (CRDT write) — neither modifies the iterated set. No concurrent modification. ✅
ColliderMatchesMask vs PropagateResultComponent consistency: ColliderMatchesMask mirrors the layer-mask gate of PropagateResultComponent without calling TryGetAvatarEntity. This is intentional — the worst case is an unnecessary PropagateResultComponent call that returns early (no event fabricated). Falls under the documented same-frame race class. ✅
(uint) casts in PropagateResultComponent: Explicit unsigned bitwise AND for ColliderLayer enum values, consistent with the new ColliderMatchesMask. Prevents potential signed-arithmetic surprises on high-bit flags. Correct cleanup. ✅
OnTriggerExit tracking guard (if (!currentEntitiesInside.Remove(other)) return;): Only queues an exit for colliders that were actually tracked. Prevents unbalanced EXITs from colliders admitted by physics while the target filter was bound (they bypassed OnTriggerEnter and were never in currentEntitiesInside). ✅
SetTargetTransform eviction: When binding a filter, RemoveWhere on all three sets evicts colliders that the filter would stop tracking. The isNotTargetEntity predicate reads TargetTransform dynamically (not at capture time), so it always uses the latest value despite being cached via ??=. Correct. ✅
TryAssignArea refactor (!hasMonoBehaviour → monoBehaviour is not { } area): Pattern-match captures the mono as area, eliminating repeated null-forgiving monoBehaviour!. accesses. SetTargetTransform moved outside the creation branch so it runs on every TryAssignArea call — this is the key fix that ensures mask changes rebind/clear the filter. ✅
targetOnlyMainPlayer mutability (readonly → mutable): Required for UpdateMaskAndMeshType to update the fast-path flag. The struct is always accessed via ref in ECS queries, so mutations are written back correctly. ✅
Struct mutation via ref: UpdateTriggerArea takes ref SDKEntityTriggerAreaComponent, so UpdateMaskAndMeshType mutations persist in ECS. IncrementalTick (post-increment property) correctly advances for each synthetic event on the ref. ✅
= null! on serialized properties (BoxCollider, SphereCollider): Standard Unity practice for [SerializeField] properties — tells the nullable analyzer these are assigned by Unity serialization. Follows CLAUDE.md convention. ✅
Removed using SceneRunner.Scene;: Correct — no longer referenced after the diff. ✅
Pass B — Design, encapsulation & resource smells
No issues found. The isNotTargetEntity predicate is a single-allocation cached delegate (no per-call closures). No magic numbers introduced. Naming is clear and matches existing conventions.
No P0, P1, or P2 issues found.
STEP 6 — Complexity assessment
COMPLEX — Modifies an ECS system query signature, adds component mutation via a struct method, alters physics callback tracking logic (OnTriggerExit), and introduces synthetic event emission that bridges the ECS and physics layers.
STEP 7 — QA assessment
QA_REQUIRED: YES — Runtime behavior change affecting trigger area event emission visible to scenes. Scenes that mutate a live TriggerArea component will now see updated collision behavior.
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed files.
Security review
No security issues found. The diff is purely internal ECS game-engine logic — no external input parsing, no network calls, no auth logic, no secrets, no new dependencies.
Test coverage
5 new EditMode tests in TriggerAreaLayerUpdateShould:
UpdateMaskAndMeshTypeFromDirtyPBTriggerArea— dirty-flag consumption, mask/mesh update, component re-flagged dirtyEmitSyntheticExitWhenMaskChangeExcludesInsider— narrowing mask emits synthetic EXITEmitSyntheticEnterWhenMaskChangeIncludesInsider— widening mask emits synthetic ENTERRebindTargetTransformWhenMaskChangesFromMainPlayerOnly— CL_MAIN_PLAYER → CL_PLAYER clears the fast-path filterEvictStaleInsiderWhenMaskCyclesThroughMainPlayerOnly— full mask cycle validates eviction + no fabricated events
All 14 pre-existing TriggerAreaHandlerSystemShould tests reported green. Coverage is thorough for the fix scope.
Summary
This is a well-crafted bug fix that correctly addresses the root cause (missing IsDirty consumption), follows the established sibling-system pattern, handles edge cases carefully (pending enters, destroyed colliders, mask cycles, target-filter eviction), and ships with comprehensive test coverage. The synthetic ENTER/EXIT emission logic is sound — ColliderMatchesMask correctly mirrors PropagateResultComponent's gate, and SetTargetTransform eviction prevents unbalanced events across mask cycles. The OnTriggerExit tracking guard closes the phantom-exit vector. No blocking issues found.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies ECS system query, component struct mutation, physics callback tracking, and introduces synthetic event emission bridging ECS and Unity physics layers.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 12724 => 13126 — remove at least 403 warnings to merge. Warnings/errors in files changed by this PR (4)All Unity tests passed ✅
|
|
PR #9809, run #32266550901 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
3 participants
Problem
A scene that mutates a live
TriggerAreacomponent - changingcollisionMaskormeshafter creation - sees no effect: trigger events keep firing against the original mask/mesh
until the component is removed and re-added. The
CL_MAIN_PLAYER → wider maskcase istriply stale (component mask, mono
TargetTransformfilter,gameObject.layerrouting).GitHub #6484 (stale-bot autoclosed; defect verified present at the pin).
Root cause
TriggerAreaHandlerSystem.SetupTriggerAreareadsPBTriggerAreaexactly once and bakesmask/mesh/targetOnlyMainPlayer into the component;
UpdateTriggerAreanever consumed thePB component's
IsDirty- later PUTs from the scene were silently dropped. Both siblingarea systems (
AvatarModifierAreaHandlerSystem,CameraModeAreaHandlerSystem) alreadyconsume their dirty flags via the identical pattern; this system was the odd one out.
Fix
SDKEntityTriggerAreaComponent: newUpdateMaskAndMeshType(...)(mirrorsUpdateAreaSize);TryAssignArearebinds/clears the mono'sTargetTransformon everycall (also closes a latent pooled-mono stale-filter leak).
TriggerAreaHandlerSystem.UpdateTriggerArea: consumespbTriggerArea.IsDirtyexactlylike the siblings; on an actual mask change, emits synthetic
TaetExit/TaetEnterforcurrent insiders whose match status flips (Unity does not synthesize enter/exit on live
filter swaps), skipping colliders still in the pending-ENTER queue.
SDKEntityTriggerArea(mono):SetTargetTransformevicts non-target insiders from alltracking sets when a filter binds (the synthetic EXITs already on the wire make the sets
truthful), and
OnTriggerExitonly queues exits for colliders it actually tracked - nophantom ENTERs and no unbalanced EXITs across mask cycles.
Residual documented edges only ever MISS an event (same-frame race class shared with the
siblings); no path fabricates one.
Test
New EditMode
TriggerAreaLayerUpdateShould(5 tests, existing harness): dirty-PBmask/mesh consumption, synthetic EXIT and ENTER emission, TargetTransform rebind on
CL_MAIN_PLAYER → CL_PLAYER, and the mask-cycle eviction regression from the adversarialreview. All 14 pre-existing
TriggerAreaHandlerSystemShouldtests stay green.Validation
Windows Unity 6000.4 EditMode lane at the pin: RED FAIL 5/5 as intended (no dirty
consumption, no synthetic events, filter stays bound at pin) / GREEN PASS 5/5, sibling
regression suite 14/14.
Fixes #6484
Related: #6483 (CL_POINTER support - out of scope, unchanged), #3728 (same
read-PB-once defect class in GLTFContainer, closed)
Includes inspection-warning cleanup in all touched files.
Fixes #6484