Skip to content

fix: banned users cannot teleport after second attempt - #8868

Merged
sandrade-dcl merged 3 commits into
devfrom
fix/banned-users-cannot-teleport-after-second-attempt
Jun 1, 2026
Merged

fix: banned users cannot teleport after second attempt#8868
sandrade-dcl merged 3 commits into
devfrom
fix/banned-users-cannot-teleport-after-second-attempt

Conversation

@sandrade-dcl

Copy link
Copy Markdown
Contributor

Pull Request Description

Fix #7505

What does this PR change?

  • Banned users could not teleport after the second attempt to the banned scene; the chat showed An item with the same key has already been added. Key: SceneCommunicationPipe+SubscriberKey in builds, or Cannot add the same SystemGroupWorld twice in editor, and all further teleports stopped working.
  • Root cause: after ECSBannedScene.TrySetCurrentSceneAsBannedAsync disposed the banned scene it reset SceneLoadingState to UNINITIALIZED/PromiseCreated=false so the scene could load again once the ban was lifted, but left BannedSceneComponent on the entity. On the next frame ResolveSceneStateByIncreasingRadiusSystem.UpdateLoadingState did not filter by BannedSceneComponent and issued a fresh AssetPromise<ISceneFacade>. LoadSceneSystem resolved it into a new SceneFacade (registering it in SystemGroupSnapshot), but ControlSceneUpdateLoopSystem.HandleNotCreatedScenes does filter by BannedSceneComponent so the promise was never consumed and the SceneFacade leaked along with its SystemGroupWorld registration. Re-entering the scene later created a second SceneFacade with the same registration key and collided with the orphan.
  • Fix: short-circuit UpdateLoadingState for entities that carry BannedSceneComponent, matching the semantics already used by HandleNotCreatedScenes. The orphan promise (and therefore the orphan SceneFacade) is never created, and the unban path is unaffected — once BannedSceneComponent is removed the entity flows through UpdateLoadingState as before.

Test Steps

  • As an admin, ban a user from a scene (e.g. Genesis Plaza).
  • As the banned user, enter the banned scene → scene unloads and ban notification shows (unchanged behavior).
  • Teleport to a non-banned parcel.
  • Teleport back to the banned scene → no chat error, no SystemGroupWorld twice exception in editor.
  • Continue teleporting freely between scenes; behavior matches a non-banned user.
  • Unban the user, return to the previously banned scene → scene loads normally.

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.

- Added a check in `UpdateLoadingState` to return early for entities with `BannedSceneComponent`.
@sandrade-dcl
sandrade-dcl requested review from a team as code owners May 26, 2026 13:37
@sandrade-dcl sandrade-dcl self-assigned this May 26, 2026
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Claude reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging.

@m3taphysics

Copy link
Copy Markdown
Contributor

PR #8868, run #26454320267

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Metric Change Baseline Delta Improvement
Samples 2701 2700
CPU average 33.3 ms 33.3 ms -0.0 ms 0.0%
CPU 1% worst 33.4 ms 33.8 ms -0.4 ms 1.3%
CPU 0.1% worst 33.5 ms 36.8 ms -3.3 ms 8.9% 🟢
GPU average 8.5 ms 8.5 ms 0.0 ms -0.3%
GPU 1% worst 17.1 ms 17.0 ms 0.1 ms -0.5%
GPU 0.1% worst 23.0 ms 23.4 ms -0.4 ms 1.9%

- Add a static query to detect scenes with `BannedSceneComponent`.
- Introduce `IsSceneBanned` method to prevent readiness reports for banned scenes, ensuring proper loading screen behavior.
@claude

claude Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @sandrade-dcl's task in 2m 46s —— View job


Review

  • Gather context
  • Read changed files and diff
  • Analyze root cause vs fix
  • Post review feedback

Root-cause check

Problem (two distinct bugs):

  1. SceneFacade leak / duplicate-key collisionECSBannedScene disposed the banned scene and reset SceneLoadingState to UNINITIALIZED/PromiseCreated=false, but left BannedSceneComponent on the entity. UpdateLoadingState had no guard, so on the next frame it issued a fresh AssetPromise<ISceneFacade>. HandleNotCreatedScenes already excludes [None(typeof(BannedSceneComponent))], so the promise was never consumed — the SceneFacade and its SystemGroupWorld registration leaked. Re-entering the scene created a second registration under the same key, producing the crash.

  2. Loading screen hang on banned-destination teleport — When teleporting to a banned scene, TeleportAsync returned a WaitForSceneReadiness but no system ever dequeued the readiness report (the scene won't load), so the loading screen hung indefinitely.

Assessment: ✅ Both fixes address the actual causes. The ResolveSceneStateByIncreasingRadiusSystem guard is placed at the exact point where the orphan promise originates, and is symmetric with HandleNotCreatedScenes. The TeleportController change short-circuits the readiness wait at the right point — after PlayerTeleportIntent is added (so the avatar moves) but before a readiness guard is installed.


New changes in this sync (commit 2b680dce)

TeleportController.cs — reviewed now for the first time.

The IsSceneBanned implementation is sound:

  • Uses chunk-level iteration to avoid the delegate/closure allocation of World.Query(ForEach) (allocation-free hot path in the common case when the banned-archetype set is empty).
  • null checks for both world and sceneId are at the boundary of the method where they belong.
  • The PlayerTeleportIntent is still added for banned destinations — the avatar correctly moves to the requested parcel, the scene just stays unloaded. Same UX as entering a banned world via cross-realm.
  • Called after await UniTask.Yield(PlayerLoopTiming.PostLateUpdate), so the ECS world read happens on the main thread.

No blocking issues found.


On alejandro-jimenez-dcl's suggestion (query-level filter)

Filtering at the Arch query level would require decorating UpdateLoadingState with [Query] and [None(typeof(BannedSceneComponent))]. That's not straightforward here: UpdateLoadingState is called from CreatePromisesFromOrderedData, which iterates orderedDataManaged — a sorted managed list populated by AddNewSceneDefinitionToList at scene-entity creation time, separate from Arch's source-generated query dispatch. BannedSceneComponent is added later (ban fires after the entity is already in the list), so a query-level [None] filter can't be retrofitted without restructuring the iteration mechanism. The imperative guard is the correct fix given the current architecture.


REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches ResolveSceneStateByIncreasingRadiusSystem (core scene-loading ECS system) and TeleportController (async teleport flow with ECS world access and load-report lifecycle).
QA_REQUIRED: YES

@m3taphysics

Copy link
Copy Markdown
Contributor

PR #8868, run #26760353421

Builds: Windows change, Windows baseline, macOS change, macOS baseline

Framework 13 i7

Values are the median across 3 run(s); the (min–max) range shows how much the runs varied. A metric is only flagged when the change is larger than both the 3% threshold and that run-to-run variation.

Metric Change Baseline Delta Improvement
Samples 2701 (×3) 2700 (×3)
CPU average 33.3 ms 33.3 ms 0.0 ms -0.0%
CPU 1% worst 33.8 ms (33.6–33.9) 33.5 ms (33.4–33.8) 0.3 ms -0.9%
CPU 0.1% worst 36.1 ms (33.8–37.2) 35.2 ms (34.1–36.5) 0.9 ms -2.6%
GPU average 8.3 ms (7.2–8.3) 8.6 ms (7.7–10.0) -0.4 ms 4.6%
GPU 1% worst 19.9 ms (19.2–21.4) 18.4 ms (17.8–18.9) 1.5 ms -8.4%
GPU 0.1% worst 24.7 ms (24.2–24.8) 24.9 ms (22.7–25.2) -0.2 ms 0.8%
Exceptions logged 48 14 +34 🔴

@DafGreco DafGreco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✔️ 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
  • [ ✔️ ] Camera

Evidence:

20260601-1444-21.7306164.mp4
Screen.Recording.2026-06-01.At.15.44.03.1.mp4

@sandrade-dcl
sandrade-dcl merged commit 97dffdc into dev Jun 1, 2026
17 of 19 checks passed
@sandrade-dcl
sandrade-dcl deleted the fix/banned-users-cannot-teleport-after-second-attempt branch June 1, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[QA] Explorer | Banned users cannot teleport after second attempt

4 participants