fix: scene's gliding restriction stayed active after the player left that scene - #9518
Conversation
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
Warnings count reduced: 14075 => 14060 |
popuz
changed the title
popuz
changed the title
popuz
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — fix: scene's gliding restriction stayed active after the player left that scene
STEP 2 — Root-cause check ✅
Problem: A scene that disabled only gliding (or only double jump) via InputModifier leaked that restriction into every subsequent scene until a full client restart.
Root cause correctly identified and fixed. The old SendBusMessage check manually enumerated 6 flags (DisableAll, DisableWalk, DisableJog, DisableRun, DisableJump, DisableEmote) but excluded DisableGliding and DisableDoubleJump. This meant lastBusMessageAction never flipped to Applied for gliding-only scenes, so the OnSceneIsCurrentChanged(false) reset gate (lastBusMessageAction == Applied) was never satisfied, and the global DisableGliding flag persisted across scene transitions.
The fix replaces the manual enumeration with inputModifier.EverythingEnabled (which checks disabledMask == ModifierId.None), correctly covering all current and future modifier flags. This is a cause fix, not a symptom fix.
STEP 3 — Design & integration ✅
No new long-lived units introduced. The change is a single-line fix within the existing InputModifierHandlerSystem. No new systems, managers, or persistent state holders are added.
Architecture assessment: The system holds one SceneRestrictionsAction lastBusMessageAction field (a single enum scalar). This is comparable to a bool hasInitialized field and does not violate the "systems must not hold persistent collections" rule. Multiple per-scene system instances sharing one global InputModifierComponent is safe because exactly one scene is current at a time — OnSceneIsCurrentChanged(false) fires on the departing scene before (true) on the arriving one.
Semantic coupling note (non-blocking): lastBusMessageAction now serves double duty as both the UI indicator gate and the cleanup gate. If a future modifier should trigger cleanup but not the UI indicator (or vice versa), this coupling would need to be revisited. Under the current design this is fine — all modifiers should consistently trigger both.
Teardown/consumption trace: No new subscriptions, event hookups, connections, or buffers are introduced.
STEP 4 — Member audit ✅
No new public properties or accessors are added in production code. EverythingEnabled is a pre-existing property on InputModifierComponent (checks disabledMask == ModifierId.None). Its single consumer in the production diff is the SendBusMessage method — this is appropriate since EverythingEnabled is a canonical bitmask check, not a single-use derived predicate.
STEP 5 — Line-level review
Production code (1 line changed): Clean, correct, more maintainable than the manual enumeration it replaces. No bugs, no performance regressions, no resource leaks.
Behavioral change — PR description inaccuracy [P2]: The PR description states "SendBusMessage / the UI indicator behavior is unchanged" and mentions a "dedicated sceneAssertedModifiers flag" — but neither is true in the actual implementation. The fix does change the UI indicator: the minimap AvatarMovementsBlocked indicator will now appear for gliding-only and double-jump-only restrictions (previously it did not). The tests explicitly validate this new behavior (PushMovementBlockedBus_WhenOnlyGlidingDisabled, PushMovementBlockedBus_WhenOnlyDoubleJumpDisabled). The description appears to have been written for an earlier iteration of the fix. This is arguably a better, more consistent behavior — but the description should be updated to accurately reflect what changed, so QA testers know to verify the indicator behavior.
Tests (426 new lines): Well-structured, comprehensive, follow AAA pattern with NUnit + NSubstitute as required. Three test suites cover:
InputModifierComponentShould— bitmask primitives (EverythingEnabled, per-flag independence,DisableAlldominance,RemoveAllModifiers)InputModifierHandlerSystemShould— new cases for gliding-only/double-jump-only reset, bus messages, reapply-on-return, gate-clears-when-scene-emptiesInputModifierHandlerSystemCrossSceneShould— integration suite reproducing the exact playtest bug with two per-scene systems sharing one global player
No issues found in test code. Test helper methods (Set/Get switch in InputModifierComponentShould) are appropriate for the parameterized [TestCase] pattern.
STEP 3 (Security) ✅
No security issues found. The change is a pure logic refactor in a game input system. No auth, network, file I/O, sensitive data, or injection surfaces are involved.
STEP 6 — Complexity
SIMPLE — 1 production file changed (1 line), 3 test files added. The change modifies how a condition is evaluated in an existing ECS system method, using an existing property.
STEP 7 — QA assessment
YES — This changes runtime behavior affecting player input (glider/double-jump restrictions) and the minimap UI indicator. Manual verification of the teleport scenario is required.
STEP 8 — Non-blocking warnings
None (Main scene not modified).
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: Single-line fix in InputModifierHandlerSystem.SendBusMessage — replaces manual flag enumeration with existing EverythingEnabled bitmask check
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tests: 24492 passed, 0 failed ✅ |
DafGreco
left a comment
DafGreco
left a comment
There was a problem hiding this comment.
✔️ PR reviewed and approved by QA on both platforms following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [✔️ ] Backpack and wearables in world
- [✔️ ] Emotes in world and in backpack
- [✔️ ] Teleport with map/coordinates/Jump In
- [ ✔️] Chat and multiplayer
- [✔️ ] Profile card
- [ ✔️] Camera
- [✔️ ] Settings
Evidence on the following thread
3 participants
Pull Request Description
What does this PR change?
Fixes a cross-scene bug where a scene's gliding restriction stayed active after the player left that scene, disabling the glider everywhere until a full client restart.
Symptom (from playtest)
A scene (
shroomzoom.dcl.eth) disabled the glider via theInputModifierSDK component. After teleporting to a different World that imposes no restriction, the glider remained disabled./reloaddid not help — only a full client restart did.Root cause
The gliding restriction is stored in the shared
InputModifierComponenton the global player entity, and each scene'sInputModifierHandlerSystemresets it when the scene stops being current. That reset is gated onlastBusMessageAction == Applied, which is driven bySendBusMessage.SendBusMessage's "is anything restricted?" check only looked atwalk/jog/run/jump/emote/all. That check was written before gliding and double jump existed (introduced in #2437), and when those flags were added (#7312) they were added toApplyModifiers/ResetModifiersbut never added to this check.Two consequences of that omission:
lastBusMessageActiontoApplied, so on scene exit the reset was skipped and the globalDisableGlidingflag leaked into every subsequent scene./reloadreloads the scene but not the global player entity, so it couldn't clear the stale flag; only a restart (which recreates the global world) did.AvatarMovementsBlockedminimap indicator never appeared for gliding-only / double-jump-only restrictions, even though it correctly appeared for every other input modifier (including emote).Fix
Generalize the check to
inputModifier.EverythingEnabledso it accounts for all modifiers. This restores the originally intended semantics ("is any modifier active?") in a way that can't drift again as new flags are added. No extra state was introduced.Because the check now includes gliding and double jump, the
AvatarMovementsBlockedminimap indicator ("• Avatar movement disabled") now appears for scenes that disable only gliding or only double jump. Previously it did not appear for those cases. This is the intended, consistent behavior (it already appeared for jump/emote/etc.), but it is a user-visible change.Tests added
InputModifierComponentShould— new suite covering the bit-mask primitives the fix relies on (EverythingEnabled, per-flag independence,DisableAlldominance,RemoveAllModifiers).InputModifierHandlerSystemShould— new cases: reset-on-leave for gliding-only and double-jump-only scenes;PushMovementBlockedBus_WhenOnlyGlidingDisabled/...WhenOnlyDoubleJumpDisabled(pins that the indicator now firesAppliedon entry andRemovedon leave); reapply-on-return; gate-clears-when-scene-empties.InputModifierHandlerSystemCrossSceneShould— integration suite with two per-scene systems sharing one global player, reproducing the teleport handoff (including the exact playtest repro) and proving scenes don't clobber each other's restrictions.Test Instructions
Run this PR:
Run with a fresh account:
Prerequisites
FeatureId.Gliding). Confirm you can glide in a normal parcel before testing the restriction.Test Steps (QA — the bug repro)
shroomzoom.dcl.eth(this World disables the glider viaInputModifier). Try to glide — the glider must NOT open here. This confirms the restriction is applied.shroomzoom.dcl.ethand confirm the glider is disabled again, then leave once more and confirm it re-enables — the restriction should toggle cleanly on every entry/exit.Verify the minimap indicator (behavioral change)
shroomzoom.dcl.eth, check the minimap scene-restrictions icon/toast — it should now list "• Avatar movement disabled" (this did not appear before this PR for a gliding-only restriction).Additional Testing Notes
/reloadwhile insideshroomzoom.dcl.ethshould not be required to recover gliding after leaving; verify recovery happens purely on teleport out.Automated tests
Run the InputModifier EditMode suites in the Unity Test Runner (filter by
InputModifier):InputModifierComponentShouldInputModifierHandlerSystemShouldInputModifierHandlerSystemCrossSceneShouldQuality Checklist
SendBusMessagecondition simplification)shroomzoom.dcl.ethreproduces the restriction)Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.