Location Loader issue

[gabaguiarp]

Short description
I have spotted an issue with the Location Loader where, at some point, an error will cause two locations to be stacked, which results in critical bugs, like having two instances of the Player at the same time.

Expected behaviour
New scenes/locations must be able to load seamlessly, without causing errors. Therefore, previous scenes must be unloaded first.

To Reproduce

1. Start on the Beach scene.
2. Exit to Forest.
3. Exit to Beach.
4. Keep repeating several times, until an "Index out of range" exception is thrown and you'll have two different locations loaded at the same time!

Notes

• This is a repost of from the Unity forums thread.
• Possible fix: I've noticed that the LocationLoader script loads new scenes before unloading the existing ones, which makes me wonder: shouldn't this happen reversely? I mean, it makes more sense to unload the existing scenes first, before setting the new ones to load.

Screenshot

[ErikBehar]

So this is happening because the two exit locations ( forest to beach & beach to forest) happen to be on top of each other. If you move one of the two so that its not overlapping then you wont have this issue.

The loading order is random, since we do both load and unload Asynch.

So this is likely to trigger in the case where the new scene loads before the older scene is unloaded, thus the old pig triggers the enter in the new scene. This could also happen the other way if the spawn point happened to be on top of where an old trigger is in the other scene.

Overall this could probably be prevented by:
a) destroying the character prior to doing any load/unload ( this could also prevent other issues regarding SO's surviving past scene load, between the two character loads )
b) making sure exit / enters don't overlap at spawn or at exit
c) adding a global cool down to exit locations, so that they can't be triggered so fast ( a few seconds should be enough?)
d) add more defensive code, like checking if the pig that triggered is in the active scene

[ErikBehar]

tested option D) 👍
add
to line 19 of LocationExit : && other.gameObject.scene == SceneManager.GetActiveScene()

seems to work fine

[gabaguiarp]

So this is happening because the two exit locations ( forest to beach & beach to forest) happen to be on top of each other. If you move one of the two so that its not overlapping then you wont have this issue.

The loading order is random, since we do both load and unload Asynch.

So this is likely to trigger in the case where the new scene loads before the older scene is unloaded, thus the old pig triggers the enter in the new scene. This could also happen the other way if the spawn point happened to be on top of where an old trigger is in the other scene.

Overall this could probably be prevented by:
a) destroying the character prior to doing any load/unload ( this could also prevent other issues regarding SO's surviving past scene load, between the two character loads )
b) making sure exit / enters don't overlap at spawn or at exit
c) adding a global cool down to exit locations, so that they can't be triggered so fast ( a few seconds should be enough?)
d) add more defensive code, like checking if the pig that triggered is in the active scene

Good point, that didn't occur to me at all!

I agree with option D, but I have one more defensive solution to add:
e) Since this is open world and every location is "connected", why not position them as they should appear in relation to each other, as if they were pieces of a bigger world map?
What I mean is, for instance: if the world map has two locations, both 10 units wide, and the Beach location pivot is at (0, 0, 0), the Forest location pivot would be at (0, 0, 5). This way, it's like the Pig Chief had crossed real world coordinates.
Of course, to do this, every GameObject in each location scene should be parented to a top GameObject that acts as the location's pivot and can easily be modified as the dimensions of the scene are increased/decreased. This not only avoids the issue where the Player hits two "exit triggers" at the same time, as it is also helpful to "build" the complete world map in the editor in a modular way, by having two or more scenes open at the same time, without them overlapping.

[ErikBehar]

You might run into issues trying to position a scene, because it might not actually match real world sizes to each other, and which exit entrance combo should the pivot be on.

Overall it might be more helpful to separate them in a grid instead ?

[gabaguiarp]

Yes, that's sort of what I'm suggesting. It's basically like each scene is a piece of a grid.
One scene's pivot should match the direction the previous scene exit leads to. It's just like you were building it all in one Master Scene and then slicing each location to pieces (scenes), very similar to what this tutorial describes.

[jandd661]

The scenes where being loaded additively. Unless there is a plan to do some fancy level phasing, the scenes should be loaded LoadSceneMode.Single am I missing something? Anyway, I have a PR #231 for a proposed fix.

Video: https://youtu.be/9rZnrfmYvfw

[gabaguiarp]

Scenes need to be loaded additively, because the game starts with an Initialization scene that must be persistent. That scene is responsible for displaying the loading screen interface between loads, managing audio, and possibly carry Player Data, along with other background operations.

It's true that each scene in the project has a reference to the Initialization scene and automatically loads it on Start if it is not already present in the hierarchy, but that is meant for debugging purposes. Loading scenes with LoadSceneMode.Single will prevent that session's data from being carried, since the Initialization scene will be reloaded and therefore reset.

[ErikBehar]

Also we are sending an array of scenes ( meaning you could load several scenes if required)

[jandd661]

Ah, I see. Nothing is ever easy is it :D Ok, I closed the PR #231 and will take another look with that info in mind. Thanks.

[jandd661]

OK, I got it worked out. It now works as you guys said it should work. I also left some TODOs for expansion that I can come back to if needed. Pretty handy little scene loader. Still have lots to do though =)
PR: #236
Video: https://youtu.be/VQWs_yalU6o

[ErikBehar]

OK, I got it worked out. It now works as you guys said it should work. I also left some TODOs for expansion that I can come back to if needed. Pretty handy little scene loader. Still have lots to do though =)
PR: #236
Video: https://youtu.be/VQWs_yalU6o

I think these are good changes, albeit :

1. does not properly support loading multiple scenes (of course can be added in future)
2. should still do the check I suggested on the exit, since the issue is still happening, just being prevented from surfacing symptoms (but could have other side effects in the future) ... I've taken the liberty of implementing both A) & D) suggested solutions above, to be extra safe in this PR Issue #229 : prevent old pig from triggering anything in new scene #237

PS: note that we don't prevent the new pig from possibly triggering something (other than exits) in the old scene yet. Possibly we should wait to spawn the new pig until we are sure the old scenes are unloaded ?

[jandd661]

@ErikBehar Good deal. Please explain "does not properly support loading multiple scenes" so I know what the intent is. I get the ability to layer scenes for phasing and such but it seems to be a very convoluted way of doing things just to persist data. Thanks.

[gabaguiarp]

I agree with the changes too, but also think that @ErikBehar suggestion on adding a check method is essential to avoid issues. It's always safer to double-check when we are working with operations that handle a lot of instances at the same time.

PS: As far as I've checked, @jandd661 changes do support loading multiple scenes; that's the whole point of the video posted. Are you missing something @ErikBehar ?

[gabaguiarp]

@ErikBehar As for destroying the player, I wonder if it wouldn't be more reliable to do it in the Spawn method itself. Something like:

IEnumerator Spawn(int spawnIndex)
{
var player = GameObject.FindGameObjectWithTag("Player");
if (player != null) Destroy(player);
yield return new WaitUntil(() => SceneManager.GetActiveScene() == gameObject.scene);
// Spawn code
// ...
}

[ErikBehar]

@ErikBehar As for destroying the player, I wonder if it wouldn't be more reliable to do it in the Spawn method itself. Something like:

IEnumerator Spawn(int spawnIndex)
{
var player = GameObject.FindGameObjectWithTag("Player");
if (player != null) Destroy(player);
yield return new WaitUntil(() => SceneManager.GetActiveScene() == gameObject.scene);
// Spawn code
// ...
}

I think we want to destroy the old pig as early as possible, so on the exit script is the earliest... if you wait to the spawn, you might run into the same issues we are trying to prevent.

I do think adding the wait for the new pig might be good idea, although I think it would be best to wait for all the old unloading scenes to be done loading instead of just the active scene. This would require listening to some event from the loader.

[ErikBehar]

@ErikBehar Good deal. Please explain "does not properly support loading multiple scenes" so I know what the intent is. I get the ability to layer scenes for phasing and such but it seems to be a very convoluted way of doing things just to persist data. Thanks.

Sorry, didnt look at the video, I only looked at the code changes, and I was looking at "SetActiveScene" function and wasn't completely sold on it working well when I read the code before. I see that its always setting active_scene now. I guess this is fine with the current assumption that the active scene should always be the first one.

[gabaguiarp]

@ErikBehar As for destroying the player, I wonder if it wouldn't be more reliable to do it in the Spawn method itself. Something like:
IEnumerator Spawn(int spawnIndex)
{
var player = GameObject.FindGameObjectWithTag("Player");
if (player != null) Destroy(player);
yield return new WaitUntil(() => SceneManager.GetActiveScene() == gameObject.scene);
// Spawn code
// ...
}

I think we want to destroy the old pig as early as possible, so on the exit script is the earliest... if you wait to the spawn, you might run into the same issues we are trying to prevent.

I do think adding the wait for the new pig might be good idea, although I think it would be best to wait for all the old unloading scenes to be done loading instead of just the active scene. This would require listening to some event from the loader.

Seems acceptable to me.

Maybe moving _scenesToLoadAsyncOperations[0].completed += SetActiveScene to the end of the TrackLoadingProgress coroutine will get the job done. That way, the first scene will only become active after all the others finish loading.
Of course, we'll add this inside the else statement in LoadScenes too, or create a method called OnLoadingFinished to do both the activation and _scenesToLoadAsyncOperations clearing.

[ciro-unity]

Thanks everyone, the issue is fixed... for now!
We need to do some more work on the scene loading, specifically to support multiple entrances/exits from locations. I'll talk about it during the stream on Tuesday, and I'll create a task card for it!