decentraland / decentraland/unity-explorer
[TECH DEBT] Teleportation | Replace the landOnParcel flag with an explicit teleport destination
- Dominant language
- C#
- Stars
- 23
- Forks
- 17
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 101
Description
# Replace the `landOnParcel` flag with an explicit teleport destination
**Type:** tech debt / refactor (no user-facing behaviour change)
**Area:** RealmNavigation, CharacterMotion
**Size:** ~9 production files in the navigation stack, no new feature surface
> File paths and line numbers below are as of `dev` @ `80ee7584b`, i.e. **after** PR #9567 was merged
> (merge commit `ede02eb29`, 2026-08-03). They will drift as `dev` moves — the symbol names are the
> stable part.
## Problem
`parcel` in the teleport pipeline carries two different meanings at once:
1. *which scene to load* (the scene is looked up by the parcel), and
2. *where the player should stand*.
Historically only meaning (1) was honoured. `Explorer/Assets/DCL/RealmNavigation/TeleportController.cs:88`
overwrites the caller's parcel and the parameter changes meaning halfway through the method:
```csharp
// When landing on the exact parcel, keep the requested parcel; otherwise snap to the
// scene base so the spawn point is used.
if (!landOnParcel)
parcel = sceneDef.metadata.scene.DecodedBase; // Override parcel as it's a new target
```
`TeleportPositionCalculationSystem` then placed the player at `base + spawnPoint`, so in one large
scene such as Genesis Plaza every parcel resolved to the same spawn point.
PR #8942 needed meaning (2) for the Events page and, instead of disambiguating the parcel, added a
boolean beside it. Consequences visible today:
### 1. Flag argument threaded through the whole stack, one caller sets it
| File | Line | Role |
|---|---|---|
| `Explorer/Assets/DCL/Events/EventCardActionsController.cs` | 100 | **the only** `landOnParcel: true` in the codebase |
| `Explorer/Assets/DCL/Infrastructure/SceneLifeCycle/Realm/IRealmNavigator.cs` | 83, 87 | `TryChangeRealmAsync`, `TeleportToParcelAsync` |
| `Explorer/Assets/DCL/RealmNavigation/RealmNavigator.cs` | 93, 132, 228, 238, 277, 294, 298, 309, 312, 316, 330 | forwards it through five private methods |
| `Explorer/Assets/DCL/RealmNavigation/TeleportOperations/ITeleportOperation.cs` | 24, 31, 38 | `TeleportParams.LandOnParcel` |
| `.../TeleportOperations/TeleportToSpawnPointOperationBase.cs` | 45, 49, 69, 81 | |
| `.../TeleportOperations/MoveToParcelInSameRealmTeleportOperation.cs` | 24 | |
| `.../TeleportOperations/MoveToParcelInNewRealmTeleportOperation.cs` | 18 | |
| `Explorer/Assets/DCL/RealmNavigation/ITeleportController.cs` | 18, 20, 27, 29 | interface + extension |
| `Explorer/Assets/DCL/RealmNavigation/TeleportController.cs` | 56, 68, 72, 82, 84, 90, 99 | the decision point |
| `Explorer/Assets/DCL/Character/CharacterMotion/Components/PlayerTeleportIntent.cs` | 36, 53, 62 | ECS component |
| `.../CharacterMotion/Systems/TeleportPositionCalculationSystem.cs` | 54 | picks the landing |
| `.../CharacterMotion/Systems/TeleportCharacterSystem.cs` | 185 | gates the floor probe |
Roughly 14 signatures carry a boolean that exactly one call site ever sets.
### 2. Three fields encode one decision
`PlayerTeleportIntent` (`PlayerTeleportIntent.cs:26-42`) holds `Parcel`, `LandOnParcel` and
`SpawnPointName` (the last added by #9369 for deeplink overrides) plus `SceneDef`, and
`TeleportController.TeleportAsync:68` has a *fourth* boolean, `nullifySceneDef`, for the debug widget:
```csharp
private async UniTask TeleportAsync(Vector2Int parcel, AsyncLoadProcessReport loadReport,
CancellationToken ct, bool nullifySceneDef = false, bool landOnParcel = false, string? spawnPointName = null)
```
`TeleportPositionCalculationSystem.CalculateTeleportPosition:41-99` reads all of them back and is
already a switch over destination kinds — the kinds simply are not a value anywhere:
```csharp
if (sceneDef == null) { /* empty parcel + terrain height */ }
else if (TeleportUtils.IsRoad(...)) { /* parcel base */ }
else if (teleportIntent.LandOnParcel) { /* parcel centre + floor probe later */ }
else { /* PickTargetWithOffset(spawnPointName) */ }
```
### 3. The invented landing point needs a physics crutch
`landOnParcel` does not take the landing spot from creator data; it computes it geometrically
(`TeleportPositionCalculationSystem.cs:61`):
```csharp
Vector3 targetWorldPosition = ParcelMathHelper.GetPositionByParcelPosition(parcel)
+ new Vector3(ParcelMathHelper.HALF_PARCEL_SIZE, 0f, ParcelMathHelper.HALF_PARCEL_SIZE);
```
Because that point can land inside geometry, the same PR added `SnapToSceneFloor`
(`TeleportCharacterSystem.cs:210-270`, constants at `:38-52`) — a 3×3 `Physics.RaycastAll` grid with a
`Physics.SyncTransforms()`, a `List<>` allocation and a **15 m** step-up tolerance
(`LAND_ON_PARCEL_MAX_STEP_UP`), reachable only through this flag:
```csharp
Vector3 targetPosition = teleportIntent.LandOnParcel ? SnapToSceneFloor(teleportIntent.Position) : teleportIntent.Position;
// ...
float ceiling = lowestFloor + LAND_ON_PARCEL_MAX_STEP_UP; // anything below is "a walkable step"
```
That `ceiling` is what produced the bug in #9546: the parcel centre of the reported scene sits inside
its centrepiece asset, the asset's top is within 15 m of the parcel floor, so it counted as a walkable
step and the avatar was placed on top of it.
## History
| PR / issue | What happened |
|---|---|
| [#8942](https://github.com/decentraland/unity-explorer/pull/8942) | Introduced `landOnParcel` plus `SnapToSceneFloor` so an event at the Theatre (`0,5`) inside Genesis Plaza lands at the Theatre and not at the plaza's spawn point. Merged 2026-06-15. Its own description already names the clean solution: *"A cleaner long-term option (out of scope here) is author-defined named spawn points in `scene.json` that an event/place can reference."* |
| [#9369](https://github.com/decentraland/unity-explorer/pull/9369) | Delivered exactly that: named spawn points addressable through `spawnPointName`, anchored on the scene base (`TeleportUtils.TryPickNamedSpawnPoint`). |
| [#9546](https://github.com/decentraland/unity-explorer/issues/9546) | Bug report: Events "Jump In" drops the player inside the scene's centrepiece asset, while "Jump In" from the map lands correctly. |
| [#9567](https://github.com/decentraland/unity-explorer/pull/9567) | Fix for #9546, merged 2026-08-03, and the point where the two branches converge: `TeleportUtils.TryPickSpawnPointNameInParcel` resolves the name of the spawn point standing in the requested parcel, and `TeleportController.cs:82` routes the teleport through the `spawnPointName` path, so `landOnParcel` — and with it the floor probe — is bypassed. The flag survives only for its original case: a parcel that holds **no** spawn point of its own. |
## Proposal
Represent the destination as one explicit value instead of a boolean plus an overwritten parcel.
Sketch — names are negotiable, the shape is the point:
```csharp
public enum TeleportPlacement
{
SceneSpawnPoint, // default: /goto, map clicks, places, friends
NamedSpawnPoint, // deeplink override, and the Events case after #9567
RequestedParcel, // Events at a parcel that declares no spawn point (Theatre at 0,5)
ParcelOnly, // debug widget: ignore the scene entirely (today's nullifySceneDef)
}
public readonly struct TeleportDestination
{
public readonly Vector2Int Parcel; // always the scene selector, never overwritten
public readonly TeleportPlacement Placement;
public readonly string? SpawnPointName; // set only for NamedSpawnPoint
public static TeleportDestination SceneSpawnPoint(Vector2Int parcel);
public static TeleportDestination NamedSpawnPoint(Vector2Int parcel, string name);
public static TeleportDestination RequestedParcel(Vector2Int parcel);
public static TeleportDestination ParcelOnly(Vector2Int parcel);
}
```
What this buys, concretely:
* `TeleportController.TeleportAsync` stops reassigning `parcel`. The base-parcel substitution becomes
a property of the placement (`SceneSpawnPoint`/`NamedSpawnPoint` anchor on `scene.DecodedBase`),
resolved where the position is computed instead of by mutating an argument two layers earlier.
* The #9567 decision reads as one expression: if the requested parcel names a spawn point, the
destination *is* `NamedSpawnPoint`; otherwise it stays `RequestedParcel`.
* `TeleportPositionCalculationSystem` switches on `Placement` (plus the two `sceneDef` guards that are
genuinely about the scene, not about intent), so a new placement cannot be added without the
compiler pointing at every site that must handle it.
* `nullifySceneDef` disappears as a separate concept.
* The four optional parameters collapse to one argument across the whole chain.
Watch out: `PlayerTeleportIntent.Parcel` is read outside the teleport code —
`TeleportUtils.GetTeleportParcel` and `PlayerTeleportIntent.JustTeleported` feed scene-loading and
LOD logic. Keep a `Parcel` accessor on the intent (forwarding to `Destination.Parcel`) rather than
touching those consumers.
This is a pure refactor: the four existing landings must stay bit-for-bit identical.
It was kept **out of scope of #9567** on purpose: nine files of navigation-stack churn with no
behavioural change would have buried the bug fix on review, and QA could not have told a refactor
regression from a fix regression. #9567 is merged, so nothing gates this work any more.
### Second half of the work
Re-examine `SnapToSceneFloor` (`TeleportCharacterSystem.cs:210`). It exists to rescue a geometrically
invented point, and since #9567 the only path that invents one is `RequestedParcel` — a parcel that
declares no spawn point. Decide whether the probe still earns its
~60 lines, its `Physics.SyncTransforms()` and its 15 m tolerance on that single path, or whether
`LAND_ON_PARCEL_MAX_STEP_UP` should shrink so it can no longer place the avatar on top of an asset.
Do not assume the path is rare: of the 65 non-world events listed on the Events page on 2026-08-03,
**23** fell into exactly that branch. The classification was done by replaying the #9567 rule over
`https://events.decentraland.org/api/events` joined with
`POST https://peer.decentraland.org/content/entities/active` — worth repeating before deciding.
## Acceptance criteria
- [ ] `landOnParcel` is gone from `IRealmNavigator`, `RealmNavigator`, `TeleportParams`,
`TeleportToSpawnPointOperationBase`, both `MoveToParcel*` operations, `ITeleportController`,
`TeleportController` and `PlayerTeleportIntent`; one destination value carries the information.
- [ ] `TeleportController.TeleportAsync` no longer reassigns its `parcel` parameter, and
`nullifySceneDef` is expressed through the same destination value.
- [ ] `TeleportPositionCalculationSystem` selects the landing by switching on the placement.
- [ ] `PlayerTeleportIntent` still exposes `Parcel` to its existing consumers
(`TeleportUtils.GetTeleportParcel`, `JustTeleported`) without changes on their side.
- [ ] All four landings are unchanged from `dev`: scene spawn point (`/goto`, map, places, friends),
named spawn point (deeplink + Events since #9567), requested parcel (event on a parcel without a
spawn point, e.g. Theatre `0,5`), empty parcel / road.
- [ ] `TeleportUtilsShould`, `TeleportPositionCalculationSystemShould` and
`TeleportCharacterSystemShould` pass with assertions adapted to the new value, not weakened.
- [ ] A decision is recorded on `SnapToSceneFloor`: keep, shrink `LAND_ON_PARCEL_MAX_STEP_UP`, or remove.
Contributor guide
Research direction
Start by tracing TeleportController, RealmNavigator, PlayerTeleportIntent, and TeleportPositionCalculationSystem, then run TeleportUtilsShould, TeleportPositionCalculationSystemShould, and TeleportCharacterSystemShould. Replace the threaded flag and related parameters with one destination value while preserving all four landing behaviors and the existing Parcel consumers. Record the decision for SnapToSceneFloor and verify the acceptance criteria without weakening assertions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp, unity
- Domain
- game-dev
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100