Another bug? Item duplication when changing scenes

[piwakawaka]

I think there’s another bug related to this - if you pick up an item that is spawned and then drop it in a scene, then go through the portal to another scene, and then back again to the first scene, the original item will be missing and the dropped item will be present, as expected.

If you now stop the game and then start it again, then both the spawned item and the dropped item will be present in the savefile that results from walking back through the portal. The item has been duplicated.

If I repeat it again I end up with three of the item. Seems like an infinite dupe bug

It seemed to be an issue before and after this video’s fix, so it may be unrelated. However I can’t seem to reproduce this by simply saving and restarting the game - it seems I have to traverse the portal (twice, there and back) to get the duplication.

Is anyone able to reproduce this?

[piwakawaka]

I think I’ve tracked it down, although I don’t quite understand why this only occurs after a Portal traversal.

Look at ISaveable.RestoreState() in PickupSpawner.cs:

        void ISaveable.RestoreState(object state)
        {
            bool shouldBeCollected = (bool)state;

            if (shouldBeCollected && !isCollected())
            {
                DestroyPickup();
            }

            if (!shouldBeCollected && isCollected())
            {
                SpawnPickup();
            }
        }

In the situation I’ve described, for reasons I don’t understand yet we end up with this function being called with state set to false and isCollected() also returning false. Given this logic, the pickup is neither spawned nor destroyed, however due to the SpawnPickup() call in Awake(), it remains spawned.

The fix is to make sure the spawned pickup is destroyed in this situation:

        void ISaveable.RestoreState(object state)
        {
            bool shouldBeCollected = (bool)state;

            if (shouldBeCollected && !isCollected())
            {
                DestroyPickup();
            }

            if (!shouldBeCollected && !isCollected())
            {
                DestroyPickup();
            }

            if (!shouldBeCollected && isCollected())
            {
                SpawnPickup();
            }
        }

But what I really don’t know yet is why we end up in this situation after restarting the game following a Portal traversal, but not after restarting the game after hitting “S” to save. Any ideas?

EDIT: I’ve been able to reproduce this without stopping/starting the game, but I still need to walk through a portal:

1. Delete any save file and start the game fresh
2. Pick up a spawned item (in my case a sword) then drop it on the ground somewhere nearby
3. Walk through a portal, then turn around and walk back through it again to return
4. You should see your discarded item on the ground
5. Hit “L” to load the save (which was created by walking through the portal) - lo and behold the original item will appear, and your dropped item will remain - you now have two of them.

Alas, my workaround above doesn’t work in this case - only when starting/stopping the game, so I’m starting to think Race Condition…

[Brian_Trotter]

Hi,
Thank you for pointing this out, and for your hunting for a solution.
Unfortunately, I’m on my way out the door, so I don’t have time to explore this until later tonight or possibly tomorrow night. The one thing that I’m unsure of is the state of IsCollected()… As this is a function call, not a stored variable, it should accurately be giving the state of the item. In other words, if(!shouldbeCollected) && !isCollected) should be a state in which the item exists, and is supposed to exist. I’ll have to look closer tonight and replicate this.

[Brian_Trotter]

Hmm… I am able to confirm the bug exists. I’ll root out what’s going on after work, if I can.

[Brian_Trotter]

Actually, I had a think about this and did a very fast workaround… The issue is that the scene is not truly reloaded when we use the L)oad button. This is actually related to another state bug, where the enemy is saved as alive, but dead when you load (and stuck in the animation state of dead). The issue is that we’re not truly reloading the scene just resetting variables. We could patch every corner case until we’re exhausted, or we can make this tiny change in SavingWrapper…
Instead of calling Load() when the L key is pressed, instead use StartCoroutine(LoadLastScene()); This will force the scene to reload and reset, and appears to eliminate this bug.

[piwakawaka]

Hi Brian, thank you for looking into this and finding a workaround for the “L” feature.

I appreciate that the “L” key to load is mostly a developer tool and it’s not practical to fix every edge case, so I’d like to emphasise that this issue occurs in a more common scenario that doesn’t involve the “L” key:

1. Delete the save file and start with a clean state
2. Pick up a spawned sword from the ground, and then drop it somewhere nearby
3. Walk through a nearby portal, then back again, returning to where you dropped the sword
4. At this point, everything looks OK.
5. Stop the Unity Player (CTRL-P or click the Play button)
6. Start the Unity Player (CTRL-P or click the Play button)
7. Observe the item has duplicated

There’s also a variant of this where you can drop the item in a difference scene, return to the original scene, stop then play, and it’ll reappear in the original scene, and where you dropped it.

I haven’t checked yet whether this occurs in a full build.

[piwakawaka]

Also, I tested your workaround and unfortunately it is not working for me - if it’s a timing-related race condition perhaps a difference in our systems results in a difference in behaviour?

I’m still working to get my head around the logic, but I added some debug statements and I can see that in both cases (player stop/start and “L”), at the time the duplication occurs, my game is getting into a state where both shouldBeCollected and isCollected are false. In the original code, this is not handled and the original spawn persists due to Awake(). I’m trying to get my head around why the serialised state is false in this case.

1. start with deleted save file
2. pick up sword, drop it, walk into portal: serialised state is set to true
3. walk back into portal, shouldBeCollected is set to true, isCollected is false (seems OK).
4. after loading the scene, the save occurs again and sets serialised state to false
5. after stopping/playing the scene, shouldBeCollected is set to false, isCollected is false.

I think this might be where the problem is occurring - when the state is saved after returning through the second portal, it is serialised as false because isCollected is returning a value that hasn’t been properly updated at that point. Therefore the serialised state becomes false indicating that the pickup hasn’t been collected yet.

From Portal.cs:

            wrapper.Load();
            
            Portal otherPortal = GetOtherPortal();
            UpdatePlayer(otherPortal);

            wrapper.Save();

Somehow the DestroyPickup() occurring as a result of calling wrapper.Load() is not correctly affecting the result of isCollected() by the time the subsequent wrapper.Save() is called, and it’s capturing the wrong state. Since isCollected is using the hierarchy to work out whether the pickup exists or not, maybe there’s a race in the lifetime of objects created/destroyed in this manner? Does the Destroy happen immediately or at the end of the frame?

I’ll keep digging.

[Brian_Trotter]

The Destroy happens at the end of the frame (it could be as late as into the next frame). Unfortunately, that’s not something we can completely control, as Unity manages the actual destruction of objects. It likely is the culprit in this particular scenario… perhaps a slight modification could be:

bool hasBeenDestroyedByRestoreState=false;

bool IsCollected()
{
     return GetPickup()==null || hasBeenDestroyedByRestoreState;
}

void DestroyPickup()
{
      Destroy(GetPickup().gameObject);
      hasBeenDestroyedByRestoreState=true;
}

[piwakawaka]

Hi Brian, I think you’re right - I’ve been through the logic and I’ve convinced myself that the core problem is that in Portal.cs the wrapper.Save() is happening before the pickup has actually been destroyed, resulting in the wrong state being saved. Introducing your new variable seems to be sufficient to catch this case and ensure that the pickup state is correctly serialised.

I’ve also added your StartCoroutine(LoadLastLevel()) suggestion and that still seems to resolve the related issue when "L"oading, so together I think that closes this issue.

I’m starting to think that basing game logic on the lifecycle of objects is a tricky topic, given all the care we already need to take with race conditions during awake/start. I’m already referring to Unity in casual conversation with friends as my Pet Race Condition Generator

Thank you for your help with this.