fix: reset double jump when impulse up is applied

[popuz]

Pull Request Description

What does this PR change?

Fixes the double jump sometimes not resetting in SDK scenes with jump pads (platforms that push the player up when touched).

Root cause — a race between the scene tick and our physics tick. JumpState.JumpCount is reset in exactly one place (ApplyJump), and only when a FixedUpdate tick observes CharacterRigidTransform.IsGrounded == true. But IsGrounded is produced later, in InterpolateCharacterSystem (Presentation loop), from CharacterController.Move collision flags. A scene detects the touch on its own tick and sends an upward impulse (PBPhysicsCombinedImpulse → SDKExternalPhysicsSystems → ApplyExternalImpulse), which can launch the character before any fixed tick ever sees a grounded state — the physical contact never registers, the reset never fires, and the airborne clamp keeps JumpCount ≥ 1. The player then has to wait for the next real landing to get the double jump back.

Fix. ApplyExternalImpulse.Execute now receives ref JumpState: an upward impulse consumed while IsGrounded or within 1 m of the ground (GroundDistance, already computed every frame by GroundDistanceSystem) is treated as a landing and resets JumpCount and any pending AirJumpDelay. An upward impulse taken clear of the ground is not a landing and does not restore air jumps; horizontal and downward impulses are unaffected.

Covered by unit tests: ApplyExternalImpulseShould.

Test Instructions

Play shroomzoom.dcl..eth and verify that each mushroom resets your double jump.
BE AWARE:

• some mushrooms desn't have colliders - it is a scene issue.
• if you pushed by impulse up, but there were no ground (collider) under you, then double jump wont reset. We consider it as well a scene issue and it should be fixed by creator on the scene side. For example, you can be pushed up when you land on the edge of the violet mushroom but double jump reset wouldn't work. This is because trigger area on the mushroom probably bigger than its collider.

Do basic smoke test on jumping and impulse combinations.

Additional Testing Notes

• The fix only activates on upward external impulses taken on/near the ground; regular jumping paths are untouched, so the main risk area is jump-pad-like scenes.

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

[github-actions]

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name	Link
Commit	48addad
Logs	https://github.com/decentraland/unity-explorer/actions/runs/30546637609
Download Windows	https://github.com/decentraland/unity-explorer/suites/82819928505/artifacts/8761834347
Download Windows S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/movement/double-jump-reset-by-impule/pr-24358-48addad/Decentraland_windows64.zip
Download Mac	https://github.com/decentraland/unity-explorer/suites/82819928505/artifacts/8761710550
Download Mac S3	https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/fix/movement/double-jump-reset-by-impule/pr-24358-48addad/Decentraland_macos.zip
Built on	2026-07-30T13:51:52Z

[github-actions]

Warnings count reduced: 14060 => 14054

[github-actions]

Tests: 24500 passed, 0 failed ✅

[Ludmilafantaniella]

✅ Approved

QA'd on Windows and Mac.

Tested:

• Shroomzoom.dcl.eth — verified mushrooms correctly reset double jump on landing (aware of the known scene-side edge cases noted in the description regarding missing colliders / trigger area mismatches).
• Genesis Plaza (umbrellas/trampolines) — double jump resets correctly after impulse.
• Sky Chaser — double jump and gliding work as expected.
• Pigeon Deluxe — double jump and gliding work as expected.

Also did a basic smoke test on regular jumping (non-impulse) to confirm no regressions there.

9510.mp4

✅Smoke test performed:

• ✔️ Backpack and wearables in world
• ✔️ Emotes in world and in backpack
• ✔️ Teleport with map/coordinates/Jump In
• ✔️ Camera
• ✔️ Skybox

[claude]

Security & Dependency Review — re-run on synchronize

• Identify what changed since last review
• Dependency-change scan
• Workflow security review
• Review C# delta
• Post verdict

View job run · Branch

[pravusjif]

LGTM, this should only be affecting the reset of the jump counts when there is an external impulse involved, according to the code changes.

[claude]

Claude finished @popuz's task in 1m 52s —— View job

---

Security & Dependency Review — re-run on synchronize

• Identify delta since previous review (e69137459)
• Verify the HIGH arithmetic-context finding is closed
• Dependency-change scan
• Re-check carried-over findings
• Post verdict

---

HIGH from my last review is fixed ✅

One new commit (48addade8 "addressed CI security request"), touching only .github/workflows/pr-comment-warnings.yml`:

[[ "$COUNT"    =~ ^[0-9]+$ ]] || COUNT=0
[[ "$BASELINE" =~ ^[0-9]+$ ]] || BASELINE=""
...
total=$(jq -r '.pr_findings_total // 0' warning-result.json)
[[ "$total" =~ ^[0-9]+$ ]] || total=0

Verified this closes the issue properly, not just superficially:

• The guards sit before the first use — ahead of [ -z "$BASELINE" ], the -lt/-eq comparisons, and need=$((COUNT - BASELINE + 1)). No fork-controlled string can reach an arithmetic context any more.
• BASELINE="" on a malformed value falls into the existing if [ -z "$BASELINE" ] branch → "no baseline established yet", green badge. Fail-open on the ratchet, but the ratchet gate itself lives in test.yml, not here, so this only affects the cosmetic comment. Correct choice.
• The pre-existing $((total - shown)) on the pr_findings path got the same treatment even though I'd noted it was inherited from dev — good.
• Remaining unguarded numerics in the step (shown, dcount) come from jq … | length, which returns a number for every JSON type it accepts, so they can't carry an expression. Sound as-is.

Everything else in the PR is byte-identical to what I reviewed at e69137459.

Dependency scan — nothing to assess

git diff origin/dev...HEAD --name-only contains no manifest.json, packages-lock.json, .asmdef/.asmref, native or managed binary, Plugins/, or link.xml entry. STEP 1 finds no dependency changes, so 1.5–3 are vacuous. The new-dependency label is still stale — worth removing before merge.

C# — unchanged, still correct

ApplyExternalImpulse.cs re-read: the reset is gated behind ExternalImpulse.y > 0f, so horizontal/downward impulses and all regular jump paths are untouched, matching what @pravusjif and @Ludmilafantaniella verified. No new source changes to review.

---

Still open (all carried over, all re-verified against the current tree)

#	Severity	Concern	Location
1	MEDIUM	A suite whose artifact never uploads still renders green. [ -f "$file" ] || continue skips that mode and status stays passed. The new hasResults check only covers "ran but produced no XML" — a runner death before the upload step still slips through. Partly self-evident to a human (the table renders one row instead of two), so the practical impact is lower than the label suggests. Drive the mode list off steps.download-*.outcome.	pr-comment-test-failures.yml:86
2	MEDIUM	Six tag-pinned third-party actions in jobs holding pull-requests: write (W.7b, pre-existing on dev). SHA-pin when convenient.	pr-comment-test-failures.yml:43,54,150,159 + warnings file
3	LOW	set -euo pipefail + a jq type error on a fork-controlled artifact aborts the step and suppresses the comment. Fails closed.	pr-comment-test-failures.yml:104,111
4	LOW	Single-line markdown injection via fork-controlled test names / file paths — gsub("[\r\n]"; " ") is applied to .message but not .file. Spoofing/cosmetic only.	pr-comment-test-failures.yml:111, pr-comment-warnings.yml:191
5	LOW	GroundDistance's struct default of 0f satisfies <= 1f, so an upward impulse arriving before GroundDistanceSystem first runs reads as "on the ground" (MAX_GROUND_DISTANCE = 1000 is the real sentinel). ApplyGliding.cs:24 uses the opposite comparison, so the same 0f is fail-safe there and fail-permissive here. Worst case is one extra air jump on the first frame.	ApplyExternalImpulse.cs:28

Withdrawing L2 from my last review (droppable jq failing the lint job): .rule is built as (.ruleId // ""), so it is always a string by the time test() sees it, and the step pre-seeds echo '[]' > droppable.json. The failure path I described isn't reachable. L3 (droppable pointing devs at unrelated files) stands as a process question for a human, not a security one.

W.1/W.3/W.4/W.5/W.6/W.9 remain N/A (no LLM in any of these comment-posters; nothing reviews or executes PR content), W.2 mitigated by the random EOF_$(uuidgen) delimiter, W.8 N/A (workflow_run only, no pull_request_target, no checkout of PR code), W.10 fine (explicit contents: read + pull-requests: write).

---

Summary

Item	Version	Source	Type	Scope	Risk	Confidence
(no dependency changes)	—	—	—	—	—	—
pr-comment-warnings.yml (need=$(( )))	n/a	this PR	workflow	CI, base context	HIGH RESOLVED	HIGH
pr-comment-test-failures.yml (false-green)	n/a	earlier in PR	workflow	CI, base context	MEDIUM	HIGH
Tag-pinned actions in pull-requests: write jobs	n/a	pre-existing	workflow	CI, base context	MEDIUM	HIGH
ApplyExternalImpulse.cs (GroundDistance default)	n/a	earlier in PR	source	ships to client	LOW	HIGH

No blockers left. The remaining MEDIUMs are CI-integrity/hardening items, one of which predates this PR; neither reaches secrets or repo write, so they're reasonable to land as follow-ups rather than gate this fix.

Separately, on @pravusjif's question about the workflow files — factually, none of the four workflow changes relate to the double-jump fix: pr-comment-artifact-url.yml is a logo swap, and the other three add the droppable warning list, the "remove at least N warnings" message, the crashed-suite hasResults state, and now this sanitization. That's a maintainer call on scope, not a security one.

DEPENDENCY_REVIEW: NEEDS_ATTENTION
· Branch