Skip to content

fix: glider response to external forces#9317

Merged
popuz merged 14 commits into
devfrom
fix/week-external-force-for-glider
Jul 8, 2026
Merged

fix: glider response to external forces#9317
popuz merged 14 commits into
devfrom
fix/week-external-force-for-glider

Conversation

@popuz

@popuz popuz commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Pull Request Description

What does this PR change?

When the player is pushed by scene physics (e.g. a wind turbine using avatarPhysicsSettings / applyForce) while gliding, the push was very subtle and its direction was distorted compared to being pushed without the glider — while one would expect an open glider to be more responsive to wind, not less.

Cause: ApplyGliding clamped the magnitude of the whole GravityVelocity vector to GlideMaxGravity (1 m/s) every physics tick. The clamp predates SDK external forces (#7044), which route the vertical part of a force into GravityVelocity via effective gravity — so while gliding the clamp silently erased all wind lift. The horizontal part of the force goes through ExternalVelocity and passed untouched, which is why the push direction bent towards the horizontal.

Changes:

  • ApplyGliding: clamp only the fall speed (GravityVelocity.y = Max(y, -GlideMaxGravity)) instead of the whole vector magnitude — upward velocity from external forces and jump momentum is preserved.
  • ApplyExternalForce: while gliding, the acceleration computed from ExternalForce is multiplied by the new GlideWindResponse setting (default 1.5, tunable in CharacterControllerSettings) — an open glider catches the airflow with a larger effective area. One-shot impulses (applyImpulse) are intentionally unaffected.
  • Unit tests: ApplyGlidingShould (fall speed clamped, upward velocity preserved), ApplyExternalForceShould (multiplier applied only while gliding).

Test Instructions

Steps (standard run):

metaforge explorer run 9317

Expected result: in a scene with a continuous-force zone (wind turbine / fan pushing at an angle), flying into it while gliding pushes the player stronger than without the glider (1.5× by default) and along the force direction, with a clear vertical lift instead of a mostly horizontal drift.

Test Steps

  1. Go to flutterecho.dcl.eth
  2. Walk/jump into the force zone without the glider — note the push strength and direction.
  3. Double jump and hold jump to open the glider, fly into the same zone.
  4. Expected: the push is now stronger than in step 2 and follows the force direction (noticeable lift for angled/vertical forces).
  5. Verify jump pads and impulse-based pushes (applyImpulse) behave exactly as before, with and without the glider.
  6. Verify regular gliding is unchanged: descent still capped, opening/closing/cooldown feel the same.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

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.

🤖 Generated with Claude Code

  - ApplyGliding: clamp only fall speed (GravityVelocity.y) instead of the whole vector magnitude
  - ApplyExternalForce: multiply external force acceleration by GlideWindResponse while gliding
  - add GlideWindResponse setting (default 1.5) to character controller settings
  - add ApplyGlidingShould and ApplyExternalForceShould tests
@popuz popuz self-assigned this Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

badge

New build in progress, come back later!

@popuz
popuz marked this pull request as ready for review July 7, 2026 10:28
@popuz
popuz requested review from a team as code owners July 7, 2026 10:28
@github-actions
github-actions Bot requested a review from anicalbano July 7, 2026 10:29
@claude

This comment has been minimized.

Comment thread Explorer/Assets/DCL/Character/CharacterMotion/Velocity/ApplyExternalForce.cs Outdated
Comment thread Explorer/DCL.Character.csproj.DotSettings Outdated
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Warnings count reduced: 26870 => 26830

@claude

This comment has been minimized.

Comment thread Explorer/Assets/DCL/Character/CharacterMotion/Velocity/ApplyExternalForce.cs Outdated
Comment thread Explorer/DCL.Character.csproj.DotSettings Outdated
@claude

This comment has been minimized.

Comment thread Explorer/Assets/DCL/Character/CharacterMotion/Velocity/ApplyExternalForce.cs Outdated
Comment thread Explorer/DCL.Character.csproj.DotSettings Outdated
Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

This comment has been minimized.

@claude

This comment has been minimized.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@popuz
popuz marked this pull request as draft July 7, 2026 16:40
@popuz
popuz marked this pull request as ready for review July 7, 2026 16:40
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Todo List

  • Step 1: Load context (CLAUDE.md, docs/README.md, subsystem docs)
  • Step 2: Root-cause check
  • Step 3: Design & integration
  • Step 4: Member audit
  • Step 5: Line-level review
  • Steps 6–9: Complexity, QA, warnings, verdict

View job run

@claude

This comment has been minimized.

Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
…ableCharacterControllerSettings.cs

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Signed-off-by: Vitaly Popuzin <35366872+popuz@users.noreply.github.com>
@claude

This comment has been minimized.

Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
@balducciv

Copy link
Copy Markdown

PR reviewed and approved by QA on Mac following the PR test instructions. ✅
Smoke test performed on macOS to verify the normal flow is working as expected. ✅

Build: v0.154.0-alpha-fix/week-external-force-for-glider-d3a80a6 (PR #9317, commit d3a80a6)
OS: macOS 26.5.2 — Apple M3 Pro

Test results:

  • Push into force zone without glider — baseline behavior confirmed (step 2)
  • Push into force zone with glider open — stronger push, follows force direction with clear vertical lift (step 3-4)
  • Jump pads / impulse-based pushes (applyImpulse) unaffected, with and without glider (step 5)
  • Regular gliding unchanged — descent cap, open/close/cooldown feel the same (step 6)
  • Windows — not tested yet

Unrelated errors noted (do not affect verdict):

  • ObjectDisposedException / DynamicWorldContainer disposal errors — occur during MainSceneLoader.Shutdown() at session teardown, not runtime regressions
  • Curl error 60: Cert verify failed. Certificate has expired. (UnityTls error code 7) — unrelated to this fix

Note: Player.log does not capture character-motion/physics behavior directly in this alpha build (relevant log categories are suppressed), so log absence of errors is supporting evidence only — the in-game smoke test is the primary confirmation of the fix.

Verdict: PENDING ⏳ (Mac PASS ✅ — Windows coverage pending)

08.07.2026_10.03.33_REC.physics.mp4

Player-prev.log
Player.log

@DafGreco DafGreco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ PR reviewed and approved by QA on windows 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
  • [ ✔️] Skybox

Evidence:

20260708-0811-27.2542213.mp4

@claude

This comment has been minimized.

Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
@claude

This comment has been minimized.

Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
Comment thread Explorer/DCL.Character.csproj.DotSettings

@pravusjif pravusjif left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@popuz
popuz enabled auto-merge (squash) July 8, 2026 21:10
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @popuz's task in 3m 14s —— View job


Todo List

  • Step 1: Load context (CLAUDE.md, docs/README.md, diff)
  • Step 2: Root-cause check
  • Step 3: Design & integration
  • Step 4: Member audit
  • Step 5: Line-level review
  • Steps 6–9: Complexity, QA, warnings, verdict

Step 2 — Root-cause check

PASS. ApplyGliding was calling Vector3.ClampMagnitude(rigidTransform.GravityVelocity, settings.GlideMaxGravity), which clamped the full vector magnitude and silently erased upward velocity contributed by external forces. The replacement Mathf.Max(rigidTransform.GravityVelocity.y, -settings.GlideMaxGravity) floors only the fall component, leaving upward velocity untouched. Root cause correctly addressed.


Step 3 — Design & integration

PASS. Both ApplyGliding and ApplyExternalForce are static stateless utility classes — no new lifecycle-owning units introduced, so MANDATORY OWNER SEARCH is not applicable. GlideWindResponse follows the established pattern: CharacterControllerSettings (ScriptableObject) → ICharacterControllerSettingsOverridableCharacterControllerSettings. Call order in CalculateCharacterVelocitySystem.ResolveAvatarVelocity is correct — ApplyExternalForce runs before ApplyGravity/ApplyGliding so ExternalAcceleration.y is available when needed. Stray files from the prior security review are confirmed absent from this branch.

The latest commit (defbcce7) correctly removes the comment from OverridableCharacterControllerSettings.cs per pravusjif's request.


Step 4 — Member audit

PASS. GlideWindResponse has exactly one consumer (ApplyExternalForce.Execute). As a named, tunable constant on a ScriptableObject settings asset it is the correct pattern — an inline 1.5f literal would be non-discoverable and inspector-untweakable. GlideWindResponse is get-only in the interface and private set in the concrete class.


Step 5 — Line-level review

One blocking issue remains.

Explorer/DCL.PlayMode.Tests.csproj.DotSettings line 5 — file is still missing a trailing newline (\ No newline at end of file). This has been flagged in every prior review round. A suggestion has been posted inline — please accept it. All other .DotSettings files in the repo end with a newline; this one does not.

No other blocking issues found.


Step 6 — Complexity

COMPLEX. Modifies the character locomotion physics pipeline — ApplyGliding fall-speed clamping, ApplyExternalForce glide-state branching, and CalculateCharacterVelocitySystem integration.

Step 7 — QA assessment

QA_REQUIRED: YES. ✅ Already completed — Mac (balducciv) and Windows (DafGreco) both approved via in-game smoke test per the PR test instructions.

Step 8 — Non-blocking warnings

None.


REVIEW_RESULT: FAIL ❌
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the character locomotion physics pipeline — ApplyGliding fall-speed clamping, ApplyExternalForce glide-state branching, and CalculateCharacterVelocitySystem integration.
QA_REQUIRED: YES


💰 Review cost: claude-sonnet-4-6 · re-review on synchronize · branch fix/week-external-force-for-glider


💰 Review cost: $0.67 · claude-sonnet-4-6 · 24 turns

Comment thread Explorer/DCL.PlayMode.Tests.csproj.DotSettings
@popuz
popuz merged commit 990aff4 into dev Jul 8, 2026
26 of 27 checks passed
@popuz
popuz deleted the fix/week-external-force-for-glider branch July 8, 2026 21:42
popuz added a commit that referenced this pull request Jul 14, 2026
# Pull Request Description

## What does this PR change?

Fixes SDK **continuous external forces** (`PBPhysicsCombinedForce` — wind tunnels, wind zones) dying the moment any **Portable Experience** is loaded (a global PX or a worn smart wearable), plus a second bug of the same class: unloading any PX/scene killed an active force owned by another scene.

**Root cause.** `SDKExternalPhysicsSystems` runs in every scene world. Each instance whose scene is "current" zeroed the shared global `CharacterRigidTransform.ExternalForce` every frame and re-asserted it only from its own world's components. A Portable Experience world is *permanently* current (`PortableExperienceSceneFacade` sets `IsCurrent = true` forever), so the PX instance erased the parcel scene's continuous force every frame and never re-asserted anything (a PX world has no force components of its own). Impulses (jump pads) survived because they accumulate via `+=`.

**Fix.** The shared-field write is replaced with **per-scene-world contribution slots** — `CharacterRigidTransform.ExternalForceContributions` (`Dictionary<World, Vector3>`):
- each scene world writes/removes **only its own slot**; no world can erase another world's force anymore;
- the effective `ExternalForce` is derived every tick in `ApplyExternalForce.Execute` as the **vector sum** of all slots — forces from several current worlds stack (scene wind + PX jetpack is intended physics);
- a world unloading (`FinalizeComponents`) or becoming non-current removes only its own contribution;
- `/reload` clears the whole map;
- the **impulse pipeline is untouched**.

Covered by new red-first EditMode regression tests (`SDKExternalPhysicsSystemsShould`, 7 tests — the main repro test was verified failing on the old code) + adapted `ApplyExternalForceShould`.

## Test Instructions

### ⚠️ Read first: reproduce the bug BEFORE validating the fix

The bug only manifests while a Portable Experience world is running — with no PX loaded every case below trivially "passes" and validates nothing. So the QA pass has two phases:

1. **Phase 1 — repro on a build WITHOUT this fix** (any other recent PR / dev build): confirm a PX is actually running, then confirm the force dies.
2. **Phase 2 — the same steps on THIS PR build**: confirm the force keeps working.

**How PX loading works in builds:** The only lever is the environment: the PX list comes from the `alfa-global-portable-experiences` feature flag of the current environment, so launch with `--dclenv org` (production) to get the global PX list (feature flags are environment-specific).

### Prerequisites
- [ ] A Portable Experience running: the startup-loaded global PX (launch with `--dclenv org`), a **worn smart wearable** (ideal — a PX world runs on you the whole session), or `/loadpx <name>` / `/killpx <name>` chat commands.
- [ ] Test scene with a **continuous external force** (wind tunnel) and a **jump pad** (impulse): launch with `--realm flutterecho.dcl.eth`, or `/goto flutterecho.dcl.eth` from chat.

**Steps (standard run)** — verified launch line:
```bash
metaforge explorer run 9343
# or launch the downloaded build directly:
Decentraland.exe --realm flutterecho.dcl.eth --dclenv org
```
- `--realm flutterecho.dcl.eth` — loads the test world directly (no `/goto` needed)
- `--dclenv org` — production environment, whose `alfa-global-portable-experiences` flag carries the global PX list

### Test Steps

**Case 0 — repro on an OLD build (phase 1, mandatory):**
1. On any build without this fix (launched with the same args), enter the wind tunnel — the character is pushed.
2. Load any PX (`/loadpx <name>`) or equip a smart wearable.
3. Confirm the push **dies** the moment the PX starts. This is the bug. Now switch to this PR build for all cases below.

**Case 1 — continuous force survives a loaded PX (the main fix):**
1. Enter the wind tunnel, confirm the push works.
2. While inside, load a PX (`/loadpx`) — and separately, arrive already wearing a smart wearable.
3. Expected: the push keeps working, no interruption or stutter in either variant.

**Case 2 — unloading a PX must not kill a foreign force:**
1. Stand in the wind tunnel with a PX loaded (force working).
2. `/killpx <name>` (or unequip the smart wearable) while standing in the airflow.
3. Expected: the force continues uninterrupted.

**Case 3 — jump pads (impulses) unchanged:**
1. Use a jump pad with no PX loaded, then again with a PX loaded and while wearing a smart wearable.
2. Expected: behavior identical to production — same launch strength, no double or missing impulse.

**Case 4 — forces from several worlds stack:**
1. Stand in the wind tunnel and activate a PX that applies its own force (e.g. a jetpack smart wearable), if one is available.
2. Expected: the forces combine (vector sum), one does not replace the other; killing the PX removes only its own share, the tunnel keeps pushing.

**Case 5 — leaving the scene stops its force:**
1. With a PX still loaded, walk out of the force-emitting parcel to a neighboring parcel.
2. Expected: the scene's push stops on leaving; re-entering restores it.

**Case 6 — `/reload`:**
1. Stand in the wind tunnel (force active, PX loaded).
2. Type `/reload`.
3. Expected: the force stops instantly with the reload; after the scene reloads, re-entering the tunnel applies the force again.

### Additional Testing Notes
- Run the cases with the PX present in **both** forms: a global PX loaded via `/loadpx` and a **worn smart wearable** — the bug is the same but the world lifecycle differs.
- Regression watch: glider response to wind (#9317) must behave as on dev — open the glider inside the tunnel airflow.
- When reproducing on the old build the force dies the *same frame* the PX world starts — it is very visible, no need to wait.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants