Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: scene start up failure #2822

Merged
merged 23 commits into from
Nov 26, 2024
Merged

Fix: scene start up failure #2822

merged 23 commits into from
Nov 26, 2024

Conversation

mikhail-dcl
Copy link
Collaborator

@mikhail-dcl mikhail-dcl commented Nov 18, 2024

Fixes #2684

Got rid of empty scenes

There was an issue

NullReferenceException: Object reference not set to an instance of an object.
  at ECS.SceneLifeCycle.Systems.UpdateCurrentSceneSystem.Update (System.Single t) [0x00000] in <00000000000000000000000000000000>:0 
  at ECS.Abstract.BaseUnityLoopSystem.Update (System.Single& t) [0x00000] in <00000000000000000000000000000000>:0 

which indicates ISceneStateProvider is not set for the scene facade.

Instead of finding and fixing a potential source of the issue I got rid of the concept of empty scene entirely as it's been completely redundant since we introduced Terrain:

  • There will be no entity representing an empty scene that will lead faster querying over SceneEntityDefinitionComponent
  • Scene readiness will be resolved immediately just like for LODs, and roads.

Centralized handling of TeleportIntent

Now the single system TeleportCharacterSystem is responsible for the whole flow of handling TeleportIntents:

  • Happy path (before it was only this)
  • Cancellation (including Timeout), all local timeouts were removed
  • Failure with exceptions
  • Synchronizing with the existing queue
  • Recovery from failed teleport intents

Strict binding of loading progress with loading screen

There was no direct binding of the state of AsyncLoadProcessReport with the view.

Empowered LoadingScreen to handle all possible outcomes of the process (which is driven externally) and reflect them on the report itself, including:

  • TimeOut
  • Hard Cancellation
  • Failure of the internal process
  • Happy path

Now, UI fully relies on the state of AsyncLoadProcessReport and does not do any checks of timeouts, errors, etc locally.

Exception-free flow is fixed up

There was a mix of exceptions and Results in Teleport and Start-up flows. The guidelines are formalized and the code was refactored to follow this principle.

Before it was hard to follow the flow that can freely violate the contracts.

Related issues

The loading process can be interrupted at any time, leaving the game in an unpredictable state. This PR does not cover this case as it's much wider. It will be addressed in the separate issue #2821

How to QA:

  • Try to break the loading process by:
  • teleporting between realms, empty parcels, and real scenes
  • adjusting the debug loading timeout (by default is 2 minutes) to low values to simulate cancellation in the middle of the loading process. I expect you will find more bugs for which you will be able to create tickets (and later we will address them as part of No Recovery from critical failures during start-up, teleport & at runtime #2821
  • no matter what the loading screen should always correspond to the loading process: whether it's failed, canceled or succeeded, and never be stuck
  • Even if teleport has failed the user should be properly teleported back, and all the surroundings loaded properly (inc. without moving)

image

* Let Loading Screen Fade after the main operation finishes
…e-statr-up-fail

# Conflicts:
#	Explorer/Assets/DCL/LOD/Systems/VisualSceneStateResolver.cs
#	Explorer/Assets/DCL/Minimap/MinimapController.cs
#	Explorer/Assets/Scripts/ECS/SceneLifeCycle/Systems/GatherGltfAssetsSystem.cs
#	Explorer/Assets/Scripts/Global/Dynamic/RealmNavigator.cs
#	Explorer/Assets/Scripts/Global/Dynamic/TeleportOperations/LoadLandscapeTeleportOperation.cs
#	Explorer/Assets/Scripts/Global/Dynamic/TeleportOperations/StopRoomAsyncTeleportOperation.cs
* Fix recovery to the same position after failed teleport
@dalkia
Copy link
Collaborator

dalkia commented Nov 21, 2024

Question about the original requirement of the scene fail

On Init.js, delete line 60. This will make the Rapture scene (-89,-64) fail to load.

Start the client normally, and go to that position. You will see the ScriptEngineException: ReferenceError: __VERSION__ is not defined error straight away. This scene wont load.

But, I still have to wait for the loading screen timeout.

Is it possible to stop set the result of the teleport as Faulted, so we don't have to wait for the timeout?

@mikhail-dcl
Copy link
Collaborator Author

@dalkia I addressed your comment so it will fail immedeately by propagating the exception

@dalkia
Copy link
Collaborator

dalkia commented Nov 25, 2024

@mikhail-dcl

It works, but I get the following result

  1. I start in editor in 0,0
  2. I try to teleport to -89,-64 with goto
  3. It fails immediatly
  4. I get thrown into an unloaded version of GP, and the -89,-64 scene is still loaded in the hierarchy

If I recall correctly, the rollback teleport is for another PR. But shouldn't the failed scene be unloaded?

Copy link

@DafGreco DafGreco left a comment

Choose a reason for hiding this comment

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

Fix verified on both platforms! Windows and macOS ✅

@mikhail-dcl mikhail-dcl merged commit 6f084dc into dev Nov 26, 2024
5 checks passed
@mikhail-dcl mikhail-dcl deleted the fix/scene-statr-up-fail branch November 26, 2024 10:05
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.

Scene failed to load on startup
4 participants