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: 19087 => 14680 Warnings/errors in files changed by this PR (49)
|
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
…oints' into feat/support-scene-named-spawn-points
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 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 comment has been minimized.
This comment has been minimized.
|
Claude finished @popuz's task in 1m 48s —— View job Security-focused review (re-run on sync) — dependency / workflow / CI-script changes
What changed since the last security review: only the CI-script changes —
|
| Artifact | Version | Source | Type | Scope | Risk | Confidence |
|---|---|---|---|---|---|---|
| ReSharper CommandLineTools | 2023.1.2 |
download.jetbrains.com |
managed binary (CLI) | CI + local dev lint only | LOW | HIGH |
JetBrains.Unity extension (nupkg) |
2023.1.0.150 |
plugins.jetbrains.com |
managed binary (ReSharper ext) | CI + local dev lint only | LOW | HIGH |
Provenance [PR]: both URLs point at official JetBrains domains and are pinned to exact, immutable version strings embedded in the URL — no floating tags, no third-party mirror. In-file comments explain why 2023.1 (later Unity plugins schedule a deferred-cache flush that crashes headless on Linux — RIDER-122490 / resharper-unity#2491) and the extension does exactly what the diff says (makes InspectCode Unity-aware so Assets//Packages/ stop firing false CheckNamespace). Purpose matches capability.
Shipping impact [PR/Inference]: these scripts run only during linting — the test.yml pull_request job (read-only token) and the local Stop hook. Nothing here ships in the end-user desktop client, so there is no new runtime attack surface for users.
Minor notes (non-blocking, LOW): no checksum/signature verification on the two wget downloads (download-resharper.sh:35,41). Pinned URLs over TLS on official domains mitigate this and it matches the pre-existing pattern; a SHA-256 check would harden against a compromised-mirror/MITM scenario.
Workflow files — the "PR-file warnings" feature
Trust split verified [PR]: test.yml triggers on pull_request (not pull_request_target) with top-level permissions: contents: read, packages: write, checks: write, actions: read — no pull-requests: write. The new Collect PR-file warnings step runs there and only reads git data (git merge-base / git diff --name-only / jq / sed) — no PR content is executed or eval'd, and interpolated values (pull_request.number, steps.pr-findings.outputs.total) are integers, not injectable. It only produces warning-result.json.
The write-capable side (pr-comment-warnings.yml) runs on workflow_run in the trusted base context with explicit minimal contents: read + pull-requests: write. No LLM, no PR code execution, no pull_request_target. Correct.
Carried-over finding (still open):
-
[MEDIUM]
pr-comment-warnings.yml:89renders the findings table with.fileand.ruleunsanitized, and.messagestripped only of|/\r/\n(gsub("[|\r\n]"; " ")) — not</>— into apull-requests: writebot comment. These fields originate from linting PR-controlled source (file paths, rule ids, message text) and cross a trust boundary. Real-world impact is limited (GitHub strips<script>, camo-proxies images, token is onlypull-requests: write) — worst case is a cosmetically defaced bot comment. Recommend wrappingfile/rulein code spans and HTML-escaping</>inmessage. (This file is under.github/workflows/, which the GitHub App cannot edit — @popuz would need to apply it.)Heredoc note: the static
EOFdelimiter (pr-comment-warnings.yml:73) is not exploitable via these fields — each finding is emitted as a single line and.messagenewlines are stripped, so a standaloneEOFline can't be forged. A random delimiter (DELIM="EOF_$(uuidgen)") is good hygiene but non-blocking.
[LOW, optional] SHA-pin the third-party actions the comment job uses (actions/download-artifact@v4, peter-evans/find-comment@v2, peter-evans/create-or-update-comment@v3) — they run in the pull-requests: write job, so a compromised upstream tag would inherit that scope (W.7a). All are well-known and tag-pinned today.
ℹ️ Label note (non-blocking): the PR carries
new-dependency, but no external runtime dependency is added —Packages/manifest.json,packages-lock.json,Assets/Plugins/, and all.asmdeffiles are untouched. The only additions are the test-onlyDCL.Chat.Commands.Tests.asmref(bare in-repo GUID reference) and the CI-only lint tooling above. Consider dropping the label to keep the supply-chain signal accurate.
Verdict
No HIGH findings. The CI lint-script changes are LOW risk — official, exact-pinned, dev/CI-only, non-shipping; removing --no-swea only widens lint coverage and does not change the trust posture. One pre-existing MEDIUM (unescaped cross-boundary file/rule/message content in the bot comment) remains open.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
Scope: security review of the workflow / dependency / CI-script files only. The feature-code review (teleport pipeline) is covered separately by Jarvis (
REVIEW_RESULT: PASS).
2 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.