fix: events JumpIn lands in the middle of the scene - #9567
Conversation
…arcel holds one Jumping into an event dropped the player at the centre of the event's parcel, which is where a single-parcel scene tends to stand its centrepiece asset — the raycast floor snap then lifted him onto it. Reported for "BBQ Sauce Recipe" at -148,141, whose only spawn point (SpawnArea1, local 0..3 by 0..3) sits in the very parcel the event names, so the map/chat path landed 7-11 m away in the parcel corner instead. Events is the sole caller passing landOnParcel: true. Rather than merely suppressing the flag and falling back to the base-anchored selection — which in a scene with several spawn points can land even further from the event — TeleportController now names the spawn point standing in the requested parcel and lets PickTargetWithOffset address it, the path already used by deeplink spawn point overrides, so its cameraTarget is honoured as well. A parcel holding no spawn point of its own keeps landing on the parcel, preserving the reason landOnParcel exists (an event at the Theatre, 0,5 inside Genesis Plaza). A nameless spawn point cannot be addressed and counts as absent. point (SpawnArea1, local 0..3 by 0..3) sits in the very parcel the event names, so the map/chat path landed 7-11 m away in the parcel corner instead. Events is the sole caller passing landOnParcel: true. Rather than merely suppressing the flag and falling back to the base-anchored selection — which in a scene with several spawn points can land even further from the event — TeleportController now names the spawn point standing in the requested parcel and lets PickTargetWithOffset address it, the path already used by deeplink spawn point overrides, so its cameraTarget is honoured as well. A parcel holding no spawn point of its own keeps landing on the parcel, preserving the reason landOnParcel exists (an event at the Theatre, 0,5 inside Genesis Plaza). A nameless spawn point cannot be addressed and counts as absent. Along the way: the span clamping duplicated between the new overlap test and GetSpawnPositionOffset became a single TryGetClampedRange, and the four open-coded PARCEL_SIZE / 2f are now ParcelMathHelper.HALF_PARCEL_SIZE. Refs #9546
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
Warnings not reduced: 13974 => 13974 — remove at least 1 warning to merge. Warnings/errors in files changed by this PR (7) |
popuz
changed the title
|
All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 2 — Root-cause check ✅
Problem: Events Jump In aims at the geometric centre of the event's parcel, which lands the player inside/on top of the scene's centrepiece asset.
Root cause: EventCardActionsController.JumpInEvent is the sole caller that passes landOnParcel: true, and the flag unconditionally overrode the scene's spawn point — even when a creator placed one in that very parcel.
This diff fixes the cause, not a symptom. It adds a check in TeleportController.TeleportAsync that queries scene metadata for named spawn points in the requested parcel. When one exists, landOnParcel is cleared and the spawn point name is passed through the existing spawnPointName path. When none exists, the parcel-centre fallback is preserved unchanged.
STEP 3 — Design & integration ✅
Placement: The new check sits in TeleportController.TeleportAsync, which already decides how to configure the PlayerTeleportIntent component. This is parameter preparation — the check runs before the component is created. Moving it into TeleportPositionCalculationSystem would split intent resolution across two files and complicate the system's single responsibility (resolve an already-configured intent into a Vector3). Correct as-is.
No new long-lived units introduced. TryPickSpawnPointNameInParcel, CoversParcel, and TryGetClampedRange are all stateless static methods on TeleportUtils, which already hosts all spawn-point resolution logic (PickTargetWithOffset, PickSpawnPoint, TryPickNamedSpawnPoint).
OWNER SEARCH: No owner search required — no new system, plugin, manager, service, or stateful helper is introduced.
Teardown trace: The only resource acquired is ListPool<SpawnPoint>.Get() (line 136), matched by .Release() (line 149). The loop body is pure arithmetic (Mathf.Clamp, comparisons) — no I/O, no async, negligible exception risk. Pattern matches the existing PickSpawnPoint at lines 250/276. No subscriptions, events, callbacks, connections, or IDisposables added.
Cross-concern observation (non-blocking): The hand-off between phases is name-based: TryPickSpawnPointNameInParcel picks a spawn point from the parcel-filtered subset, then passes only its name to PickTargetWithOffset → TryPickNamedSpawnPoint, which re-looks up the name in the full list. If a scene declares duplicate spawn-point names across parcels, the re-lookup could resolve to a different instance than the one originally selected. This is a pre-existing limitation of TryPickNamedSpawnPoint (which already warns about duplicates), widened but not introduced by this PR. Low real-world risk.
STEP 4 — Member audit
| Member | Type | Consumers | Assessment |
|---|---|---|---|
TryPickSpawnPointNameInParcel |
public static | TeleportController.TeleportAsync + 4 tests |
Single entry point for the feature — correctly encapsulated |
HALF_PARCEL_SIZE |
public const | 7 sites (system, utils ×4, tests ×2) | Correctly replaces 4 inline PARCEL_SIZE / 2f expressions |
CoversParcel |
private static | TryPickSpawnPointNameInParcel |
Internal overlap test, single-caller scope is appropriate |
TryGetClampedRange |
private static | CoversParcel + GetSpawnPositionOffset |
Shared extraction that eliminates duplication — justified |
No single-use-wrapper, absent≠false, or redundant-guard issues detected.
STEP 5 — Line-level findings
See inline comments. One P2 finding (XML doc grammar).
Security review ✅
- Secrets / credentials: None introduced.
- Injection: Spawn point names are compared via
string.Equals(..., OrdinalIgnoreCase)— never interpolated into paths, queries, or commands. - Auth / authz: N/A — changes only which in-scene coordinate a teleport resolves to.
- Resource exhaustion: Iteration is O(spawn point count), bounded by scene JSON already in memory. No new nesting or recursion.
- Input validation:
Mathf.Clamphandles edge values. NaN spawn coordinates causeCoversParcelcomparisons to return false (spawn point skipped) — safe. Infinity is clamped to scene bounds.
No security issues found.
STEP 6 — Complexity: COMPLEX
Touches the teleportation pipeline (TeleportController → PlayerTeleportIntent → TeleportPositionCalculationSystem), spawn-point resolution logic, and affects how ~60% of current live events resolve their landing position.
STEP 7 — QA: YES
All changes are runtime code affecting player-visible landing behaviour for events. The PR provides exhaustive manual test cases covering all branches (spawn point present, absent, multiple, nameless, multi-parcel, world events).
STEP 8 — Non-blocking warnings
semantic / title-matches-convention is failing. Tests (editmode, playmode) are still in progress.
PickSpawnPoint (line 251): spawnPoints.Where(sp => sp.@default) allocates an enumerator + closure. Not introduced by this PR and not in a hot path, but worth noting for a future cleanup.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the teleportation pipeline (TeleportController intent resolution, TeleportUtils spawn-point selection, TeleportPositionCalculationSystem constant)
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. |
…scene' into fix/spawn-points-landing-inside-scene
…s.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Vitaly Popuzin <35366872+popuz@users.noreply.github.com>
…scene' into fix/spawn-points-landing-inside-scene
There was a problem hiding this comment.
✅ Approve
Tested on Windows and Mac, following the protocol (/goto → note landing → Jump In → compare).
A. Bug fix - Jump In should stop landing on/inside the asset
-
-136,85Grill master Path - lands in same corner as/goto, not centre -
-122,150Vinyl Tea Party - same corner as/goto, 1-parcel scene makes it obvious -
-113,-38Cathouse Presents - lands in corner (0,0), not centre (~11m divergence resolved)
B. Must NOT change — landOnParcel still works for large scenes
-
0,5Watch scary movies (Genesis Plaza) - still lands on 0,5, not entrance -
0,0Domino Game Day (Genesis Plaza) - lands on 0,0 -
144,-7Nights of Antrom - lands on 144,-7 - No regression on #8942 behavior
C. Same scene, two branches (CBD Plaza)
-
-141,98Beyond The NFT - lands on SpawnArea1 (new behavior) -
-139,96Community Building - lands on parcel centre (unchanged)
D. Remaining branches
- Scene without spawnPoints (
-150,95) - lands on parcel centre, unchanged - Scene with multiple spawn points (
-150,144) - lands on one of them, inside parcel - World event (Poker Night) - untouched path, realm change + world's own spawn point works fine
Camera correctly looks at the spawn point's cameraTarget on arrival for the group A cases.
No blockers on either OS. Road/empty-parcel and nameless-spawn-point paths not reproducible from live data (as noted in the PR), consistent with what's currently listed on Events.
9567-evi.mp4
✅Smoke test performed:
- ✔️ Log In/Log Out
- ✔️ Backpack and wearables in world
- ✔️ Emotes in world and in backpack
- ✔️ Teleport with map/coordinates/Jump In
- ✔️ Chat and multiplayer
- ✔️ Camera
- ✔️ Skybox
6 participants
What does this PR change?
Jumping into an event from the Events page dropped the player at the geometric centre of the event's parcel — which is exactly where a small scene tends to stand its centrepiece asset. The floor probe (
SnapToSceneFloor, 15 m step-up tolerance) then lifted him on top of it. Jumping into the same scene from the map or chat landed him correctly on the scene's spawn point, so one destination produced two different landings, one of them inside geometry — which also breaks whatever game mechanic the scene builds around that asset.Refs #9546 (bug report), #9368 (related request: creator-side spawn point control).
Probably PR that introduced the issue #8942
Root cause
Evidence from the reported case:
BBQ Sauce Recipeat-148,141,world: falsebase: "-148,141", 4 parcels, a single spawn pointSpawnArea1,default: true,position: { x: [0,3], y: [0,0], z: [0,3] },cameraTarget: { x: 8, y: 1, z: 8 }The event names the very parcel that holds the scene's only spawn point. The map path landed in the
(0..3, 0..3)corner square; the Events path aimed at local(8, 8)— the parcel centre, which is literally the creator'scameraTarget, i.e. the spot the centrepiece asset occupies. Divergence 7–11 m diagonally.There is no creator error here: both the event coordinates and the spawn point are correct.
EventCardActionsController.JumpInEventis the only caller in the codebase that passeslandOnParcel: true, and until now that flag unconditionally overrode the scene's spawn point.New logic
TeleportController.TeleportAsyncnow asks whether the creator placed a spawn point in the requested parcel and, if so, addresses it by name through thespawnPointNamepath that already exists for deeplink spawn point overrides:cameraTargetand look-at are honoured, and the floor probe no longer runsPickSpawnPoint: defaults first, then the nearestlandOnParcel, i.e. the parcel centre. This is why the flag exists: an event at the Theatre,0,5, inside the Genesis Plaza scene must land at the TheatreSpawn point coordinates are scene-local and anchored on the base parcel, so the overlap test converts the requested parcel into that space and clamps each spawn point's span to the scene bounds exactly as the placement code does. Parcel borders are inclusive, so a spawn point sitting on an edge counts as inside it.
No change to the Events API or to
scene.jsonis required — the decision is derived from scene metadata the client already loads.Drive-by cleanups
GetSpawnPositionOffsetbecame a singleTryGetClampedRange, which also removed a disagreement between the two over an emptyMultiValuearrayPARCEL_SIZE / 2fbecameParcelMathHelper.HALF_PARCEL_SIZEUnit coverage lives in
TeleportUtilsShouldandTeleportPositionCalculationSystemShould; the cases below are the manual ones.Test Instructions
Every event currently listed in the Events page was run through the new rule to pick these cases: of 65 non-world events, 41 change behaviour and 24 keep the current one. The list is valid as of 2026-08-03 — re-check that the event is still listed before testing, events expire.
Protocol for every case below: first
/goto x,y(or click that parcel on the map) and note where you land, then open Events, find the event and press Jump In. Compare the two landings.A. The bug — Events Jump In must stop landing on/inside the asset
-136,85SpawnArea1, x 0–3, z 0–3/goto, not in the middle of the parcel-122,150Spawn Point 1, x 0–3, z 0–3-113,-38Spawn Point 1at exactly(0, 0)-136,85is the same creator and the same event series as the original report, so it reproduces the reported scenario directly.B. Must NOT change — the reason
landOnParcelexists0,50,5, not at Genesis Plaza's entrance./goto 0,5legitimately differs0,00,0144,-7144,-7A regression here means the fix broke #8942 — the whole point of jumping to a specific spot of a large scene.
C. Same scene, two different branches
CBD Plaza,
base -147,91, 116 parcels:-141,98SpawnArea1(new behaviour)-139,96This pair proves the rule is evaluated per parcel, not per scene.
D. Remaining branches
spawnPoints-150,95, The Absolute Basics of Bui… (La Cantina Village, 15 parcels)-150,144, Grill Master Path week 7 (4 parcels, 2 spawn points)pokernight.dcl.ethAdditional Testing Notes
IsRoadand "no scene" paths cannot be covered manually from live data — the fix does not touch them.Spawn Point 1,SpawnArea1,spawn1), so it is not reproducible from live data either.cameraTarget, since the named path now supplies it.Quality Checklist
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.