feat: support scene named spawn points#9369
Conversation
…rget No behavior change. All /goto argument parsing moves out of GoToChatCommand into a pure static ChatParamUtils.ParseGotoTarget returning a GotoTarget value (world, parcel, spawn point, random/crowd flags). The command becomes a thin dispatcher. GotoTarget.SpawnPoint is always null in this slice — the field exists so the named-spawn-point grammar (#6848) lands additively. Covers the existing grammar with unit tests: x,y, random, crowd, world, world/x,y, plus junk/edge inputs.
PickSpawnPoint selects a case-insensitively matched named point directly, bypassing the default/closest logic (unmatched name warns and falls back). The name is threaded as string? spawnPointName through the same-realm teleport chain, mirroring landOnParcel. ParseGotoTarget accepts x,y/name.
…x,y/name
The spawn point name is threaded through the realm-change chain
(TryChangeRealmAsync → TeleportParams → MoveToParcelInNewRealmTeleportOperation
→ TeleportToSpawnPointOperationBase) and through TryChangeToGenesisAsync,
so /goto x,y/name also works from a world. For the coordinate-less form the
manifest still picks the scene parcel; the name selects the point within it.
ParseGotoTarget accepts world/name and world/x,y/name.
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
|
New build in progress, come back later! |
…oints' into feat/support-scene-named-spawn-points
popuz
changed the title
|
Warnings count reduced: 14668 => 14644 Warnings/errors in files changed by this PR (13)
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: support scene named spawn points
STEP 2 — Root-cause check ✅
This PR adds a new feature (named spawn point selection) to the teleport pipeline. The diff addresses a genuine gap: scene manifests already declare named spawn points, but the explorer had no way to target them. The implementation is a feature addition, not a symptom fix. PASS.
STEP 3 — Design & integration ✅
Threading approach: The spawn point name is threaded as string? spawnPointName = null through the existing teleport pipeline methods (ChatTeleporter → RealmNavigator → TeleportController → TeleportOperations → PlayerTeleportIntent → TeleportPositionCalculationSystem → TeleportUtils.PickSpawnPoint). This is the minimally invasive approach — it extends existing method signatures with an optional parameter rather than requiring a teleport-request-object refactoring. The parameter is never stored long-term; it flows through a single teleport invocation.
New units introduced:
GotoTarget— transient readonly struct for parsed/gotocommand results. Not a long-lived unit; no lifecycle management. Justified: it replaces ad-hoc parsing logic that was spread acrossGoToChatCommand, and the grammar-table test suite (ChatParamUtilsShould) validates every form.DeepLinkExtensions— pure static extension methods extracted fromDeepLinkHandleImplementation. The original methods were private static; the extraction is justified because the same deep-link parsing logic is now shared with the spawn point parameter, and the extension method pattern matches theDeepLinkAPI surface cleanly.
MANDATORY OWNER SEARCH: No new systems, plugins, managers, controllers, or long-lived units that manage entity/scene/connection lifecycles are introduced. The spawn point name is a transient parameter flowing through a single teleport invocation. No persistent state, no per-frame reconciliation, no collections outside ECS. PASS — no owner-search violations.
TEARDOWN / CONSUMPTION TRACE: No new subscriptions, event hookups, connections, rooms, buffers, or measurements are added. The only new state is StartParcel.SpawnPointName (consumed once at startup alongside the parcel). PASS.
STEP 4 — Member audit ✅
| Member | Consumers | Assessment |
|---|---|---|
GotoTarget.{World,Parcel,SpawnPoint,IsRandom,IsCrowd} |
GoToChatCommand, GoToLocalChatCommand, tests |
Multi-consumer; well-scoped struct fields |
ChatParamUtils.ParseGotoTarget() |
GoToChatCommand, GoToLocalChatCommand, tests |
Multi-consumer; replaces scattered parsing |
ChatParamUtils.IsSpawnPointName() |
ParseGotoTarget, GoToLocalChatCommand.ValidateParameters |
Dual-use; validates spawn point segment |
DeepLinkExtensions.{Realm,Position,SpawnPoint,Community}() |
DeepLinkHandleImplementation |
Single consumer, but justified extraction (shared parsing surface, replaces private static methods) |
StartParcel.SpawnPointName |
TeleportStartupOperation, DeepLinkHandleImplementation.Assign |
Dual-use; consumed at startup |
TeleportParams.SpawnPointName |
MoveToParcelIn{New,Same}RealmTeleportOperation |
Dual-consumer |
PlayerTeleportIntent.SpawnPointName |
TeleportPositionCalculationSystem |
Single consumer; ECS component field used by the system that calculates teleport position — correct placement |
No single-use intermediaries, no absent-≠-false predicates, no redundant derivations. PASS.
STEP 5 — Line-level review
[P2] GoToLocalChatCommand.ExecuteCommandAsync — semantic gap in bare-name form (see inline comment)
In the bare-name form (/goto-local physics), ParseGotoTarget returns the token in target.World (since it can't distinguish world names from spawn-point names at parse time). ExecuteCommandAsync then passes target.World as the spawnPointName parameter, which reads as passing a world name where a spawn point name is expected. A clarifying comment at the call site would make this self-documenting.
STEP 5 — Security review ✅
- Input validation: Spawn point names are validated by
IsSpawnPointName(non-empty, no commas, no slashes). The name is used only for case-insensitive string comparison against scene metadata — never in SQL, file I/O, URL construction, or any injection-prone context. - Deep link parameters: Extracted via
DeepLink.ValueOf()which handles URL decoding. Validated withstring.IsNullOrEmpty. - CLI arguments: Parsed via
ApplicationParametersParser. Validated with!string.IsNullOrEmpty. - Log messages: Spawn point names appear in
ReportHub.LogWarning(not user-facing chat). No injection risk. - No secrets, credentials, or sensitive data involved. No security issues found.
Positive observations
- Core mechanism is clean: single chokepoint in
TeleportUtils.PickSpawnPointwith soft fallback — teleports never fail because of a bad name. - Named lookup uses manual iteration (not LINQ), consistent with CLAUDE.md §4.
- Excellent test coverage:
TeleportUtilsShould(named selection core),ChatParamUtilsShould(grammar table — 13 test cases covering every form),RealmLaunchSettingsShould(deep link / CLI),ChatTeleporterShould(already-in-realm branching). TrimEnd()inCommandsHandleChatMessageBusis a good cross-cutting fix — trailing whitespace previously broke all command validation, not just/goto.DeepLinkExtensionsextraction is a clean refactoring that eliminates duplicated parsing logic.
STEP 6 — Complexity
COMPLEX — Modifies shared interfaces (IRealmNavigator, ITeleportController) and the teleport pipeline across 6+ assemblies, introduces new parsing grammar, and changes cold-start and runtime teleport flow.
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes affect user-facing teleport behavior across all entry points (chat commands, deep links, CLI arguments, cold start, runtime realm changes). The PR description includes a thorough test matrix.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
CI Status
- ✅ Playmode tests pass (23,542 passed)
- ❌ Editmode tests: 1 failure —
ControlSceneUpdateLoopSystemShould.StartWorldSceneWithoutReadinessReportRegardlessOfSceneRoom. This test is inControlSceneUpdateLoopSystemShould.cs, a file not touched by this PR, and tests scene update loop behavior unrelated to spawn points. Likely a pre-existing flaky test. - ⏳ Builds (Windows, macOS) and Lint still in progress
- ✅ PR title follows semantic commit format (
feat: ...)
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies shared interfaces (IRealmNavigator, ITeleportController) and the teleport pipeline across 6+ assemblies, introduces new parsing grammar, and changes cold-start and runtime teleport flow
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
popuz
added
clean-build
…ed-spawn-points # Conflicts: # Explorer/Assets/DCL/RuntimeDeepLink/DeepLinkHandleImplementation.cs
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit 84b1782.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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
- [ ✔️ ] Skybox
- [ ✔️ ] Settings
Evidence of all points
20260715-1617-22.4109911.mp4
20260715-1621-16.1543480.mp4
20260716-0829-21.6603936.mp4
20260716-0825-13.4973618.mp4
20260716-0835-41.0890537.mp4
20260717-1033-43.7435144.mp4
20260716-0834-29.9240551.mp4
This comment has been minimized.
This comment has been minimized.
|
Claude finished @popuz's task in 1m 8s —— View job Security-focused review (re-run on sync) — workflow / dependency / CI files
ScopeChanged review surface vs Dependency side: PASS. The workflow diff is byte-identical to the version reviewed on the previous sync — nothing new to flag. ✅ MEDIUM
|
4 participants
Pull Request Description
What does this PR change?
Adds named spawn point selection to the teleport pipeline (part of #6848). Scenes already declare named spawn points in
scene.json; until now the explorer always ran the default selection (default-flagged → closest). This PR lets a teleport request a spawn point by name, end-to-end.Core mechanism (single chokepoint, applies to every entry point below):
TeleportUtils.PickSpawnPointgainsstring? spawnPointName. When set and matched (case-insensitive), that point is used directly, bypassing the default/closest logic.ReportHubwarning (Spawn point '<name>' not found in scene, falling back to default spawn point selection). The teleport itself never fails because of a bad name."x": [min, max]) still scatter arriving players randomly within the range;cameraTargetstill applies.Entry points wired in this PR:
/goto(Genesis)/goto x,y/spawnPoint/goto(worlds)/goto world/spawnPoint,/goto world/x,y/spawnPoint/goto-local/goto-local x,y/spawnPoint,/goto-local spawnPoint(current scene)decentraland://?realm=...&spawnpoint=spawnPointExplorer.exe --realm ... --spawnpoint spawnPointAlso included:
/gotoparsing extracted into pureChatParamUtils.ParseGotoTargetwith full grammar-table unit tests (ChatParamUtilsShould);GoToChatCommandis now a thin dispatcher./goto 0,0(trailing space) no longer fails validation — for all commands./goto world/name(and the runtime deep link withspawnpointbut noposition) used while already inside the target realm now teleports to the named spawn point of the scene the player stands in, instead of stopping at🟡 You are already in .... Without a name the old message is kept.TeleportUtilsShould(named selection core),ChatParamUtilsShould(grammar),RealmLaunchSettingsShould(deep link / CLI arg parsing),ChatTeleporterShould(already-in-realm branching).Out of scope: SDK entry points (
teleportTo/changeRealmwithspawnPoint) — they require protocol + js-sdk-toolchain changes first; nothing for them in this repo yet.Test Instructions
Steps (standard run):
Prerequisites
flutterecho.dcl.eth(4x4 parcels, spotlight cubes in each corner, physics playground near the origin corner), deployed with:spawn(default)centerphysicsfar-lightcameraTargetlooking back at scene centermid-cornerscattertwin(declared twice)metaforge explorer logs tail --filter "spawn point"Test Cases
1.
/goto— world forms/goto flutterecho/center→ realm change, land at scene center (32, 32). No coordinates given: the world manifest picks the parcel, the name picks the point./goto flutterecho.dcl.eth/0,0/far-light→ land at the far corner and the camera faces back toward scene center (cameraTarget)./goto flutterecho/0,0/physics→ teleport within the world to the physics playground./goto flutterecho/center→ in-world teleport to the scene center, no realm change (the name targets a spawn point of the scene you are standing in)./goto flutterecho(no name) →🟡 You are already in..., no teleport (pre-existing behavior, unchanged)./goto flutterechofrom Genesis → land at the defaultspawn(2, 2), exactly as before.2.
/goto— Genesis form/goto x,y/nameagainst any Genesis scene that declares named spawn points → lands at the named point./goto 0,0/nosuch→ normal teleport to 0,0 + warning in logs./goto 0,0,/goto random,/goto crowdbehave exactly as before./goto 0,0/name→ realm change to Genesis carries the name (it is not dropped).3.
/goto-local(inside the world / any realm)/goto-local 0,0/center→ parcel + name form./goto-local physics→ bare-name form: teleports to the named point of the scene you are currently standing in./goto-local far-light→ also verify camera orientation./goto-local random//goto-local crowd→🔴 Invalid parameters+ usage./goto-local nosuch→ teleport to your current parcel, default spawn selection + warning in logs (soft fallback)./goto-local 0,0unchanged.4. Deep link — cold start (client closed)
spawn).decentraland://?realm=flutterecho.dcl.eth&dclenv=org&position=0,0&spawnpoint=far-light....&spawnpoint=nosuch→ lands at default spawn + warning in logs.5. Deep link — runtime (client already running)
position+spawnpoint→ in-world teleport to the named point.spawnpointbut withoutposition(decentraland://?realm=flutterecho.dcl.eth&dclenv=org&spawnpoint=far-light) → in-world teleport to the named spawn point of the current scene (previously this silently did nothing).realmonly (noposition, nospawnpoint) → nothing happens (pre-existing "already in realm" behavior, unchanged).6. Launch parameters (cmd / CI)
--realm <genesis peer> --position 0,0 --spawnpoint <name>.7. Cross-cutting edge cases (any entry point)
/goto-local PHYSICS,/goto flutterecho/Far-Light,...&spawnpoint=CENTER→ all match./goto-local scatterseveral times in a row → each landing is a different spot within the 26..38 square (players must not stack on one point)./goto-local twinrepeatedly → always lands at (10, 50) (first declared), never (50, 10); warning about duplicates in logs.Spawn point '<name>' not found...in logs. Nothing red in chat./goto flutterecho/center(trailing spaces) → works (previously any trailing space failed validation of any command)./goto myworld//lobby,/goto myworld/0,0/→ treated as a (non-existent) world →🔴 ... doesn't exist or not reachable./goto flutterecho/random→randomhere is a spawn point name (soft fallback if the scene has none), not the random-parcel flag.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.