Followups on Pickup destruction race conditions

[Cat_Hill]

I read these two threads

• Another bug? Item duplication when changing scenes - #7 by piwakawaka
• Another bug with the portals' saving parts - #4 by Ayeng_AE

and I am not sure I understand why the problem only seems to happen to Pickups spawned by the PickupSpawner and not the ItemDropper. Both seem to use the item’s destruction (i.e. is it null) to determine its state but only the Pickup Spawner exhibits the bug for me. Brian’s fix in one of the threads also seems to only modify the PickupSpawner code and not pickups dropped by the ItemDropper.

The issue seems to be with GetComponent() / GetComponentInChildren() which maybe has more stale information??

But why rely on this nuance? If the underlying root cause is that we cannot use the gameObject’s destruction (or non-destruction) to determine state, wouldn’t the correct solution be to modify the Pickup script such that it keeps track of its own state? Asking so that we can be better prepared to avoid future bugs.

[Brian_Trotter]

@Cat_Hill

The issue seems to be with GetComponent() / GetComponentInChildren() which maybe has more stale information??

The only circumstance in which GetComponent() could be stale is if it is cached… in other words:

SomeComponent cachedCompnent;
void Awake()
{
    //This reference could be stale if the component is destroyed!
    cachedComponent = GetComponent<SomeComponent>();
}

void DoSomething()
{
    //This reference will NOT be stale for the life of this method as long as it's
    //not part of a coroutine (which this reference isn't.
    var tempComponent = GetComponent<SomeComponent>();
    if(tempComponent) tempComponent.DoSomethingInteresting();
}

In the case of the first post, this was dealing with a bug when using L to L)oad. This was a structural defect in the course, IMO, which was always pushed off with the promise that by the end of the course we wouldn’t be using “L”, but would instead be using a Pause Menu with Return to Main Menu and would always reload from there. That is actually implemented in the final course, Shops and Abilities.

There is, however, a simple patch for the L)oad bugs that happen when you L)oad with the keyboard instead of scene transition.

In SavingWrapper.Update(), change the contents if(Input.GetKeyDown(KeyCode.L) from

Load();

to

StartCoroutine(LoadLastScene());

[Cat_Hill]

@Brian_Trotter

The only circumstance in which GetComponent() could be stale is if it is cached… in other words:

OK I think I get it.

So it seems the proximate cause is the save after load from the portal, where it’s saving the pick up as not being collected. It’s captured as being present even though it’s not in the scene. When one attempts to load after that then it’s resurrected.

Q. But why is it being captured as present even though it’s not in the scene?
A. Destroying a game object and then immediately trying to reference it WILL not return null.

So to address the problem you

1. can do what you did below.
2. Or you can remove destroyed items before you save them (like ItemDropper)

Both seem to follow a pattern of trying to make sure the CaptureState method doesn’t save destroyed items. But what I’m unclear about is why is ItemDropper working correctly even though we’re doing what seems like the same thing of checking null. In the code below you implemented a separate boolean to track if something was destroyed during restore because we didn’t trust an object’s lack of a null status. I’m trying to figure out why only PickupSpawner needed this extra check.

Edit: I think I see the difference. PickupSpawner has an awake method that creates a new instance of the pickup. ItemDropper doesn’t.

It just feels like the root cause is tied to saving and restoring state on a known unreliable variable (whether a gameobject is null or not).

Brian - what’s the argument against always tracking destroyed state with a distinct parameter (rather then checking null)? Not just for pickups but in general.

        public Pickup GetPickup()
        {
            return GetComponentInChildren<Pickup>();
        }

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

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

        }

[Brian_Trotter]

@Cat_Hill

In the code below you implemented a separate boolean to track if something was destroyed during restore because we didn’t trust an object’s lack of a null status.

The hasBeenDestroyedByRestoreState is sort of a brute force approach, to get around a particular issue. The 2nd post you mention shows a better solution to the problem, by setting the pickup’s parent to null before destroying it, it will not show up in GetComponentInChildren(), making the hasBeenDestroyedByRestoreState boolean unneeded.

@Cat_Hill

Brian - what’s the argument against always tracking destroyed state with a distinct parameter (rather then checking null)? Not just for pickups but in general.

In most cases, caching state In can lead to a mismatch between the cached value of the state and the state itself. The code above actually illustrates this mismatch in action. The hasBeenDestroyedByRestoreState is artificially setting a state because the GetPickup() has a chance (during a portal transition) of returning a pickup that very well should be destroyed.

The ideal solution is to make a change to DestroyPickup to make it so that whether the pickup still exists or not, it is not picked up by GetPickup()

public Pickup GetPickup() => GetComponentInChildren<Pickup>();

public bool IsCollected() => GetPickup() == null;

private void DestroyPickup()
{
    var pickup = GetPickup();
    if (pickup)
    {
        pickup.transform.SetParent(null);
        Destroy(pickup.gameObject);
    }
}

[Cat_Hill]

Thanks Brian - this is exactly the sort of advice I was looking for and that I think will be most useful to students.

Edit: Also seeing something else unique in PickupSpawner vs ItemDropper. Documenting here for future reference in case anyone else stumbles into race conditions like this. (temporarily unmarking above as solution).

• PickupSpawner not only Spawns on Awake but it also attempts to destroy. Item Dropper just checks if it’s been destroyed. Beware of destroying and checking destroyed status all within the context of a single. (And probably another good reason to avoid GameObject.Find* methods as it might recreate the bug)

• Triggering a checkpoint save after scene transition - This seems to be ripe with all the “raw materials” for creating race conditions. It might even be beneficial for having a special checkpoint save method. The extra “yield return null” in portaling maybe is not so much of a hack then but a standard part of doing checkpoint saves???

[Brian_Trotter]

@Cat_Hill

Triggering a checkpoint save after scene transition - This seems to be ripe with all the “raw materials” for creating race conditions.

It is ripe with opportunities for race conditions. If, however, we use LazyValues and properly initialize things in Awake(), we make those opportunities dissappear.

If you’re extremely clever, you can avoid even using LazyValue, but it’s a very useful tool. Here’s my tried and true rules for (mostly) avoiding race conditions:

• Awake() - Cache component references on the GameObject here. Set LazyValues() and ensure that all needed Dictionaries, Lists, and Arrays are created (Setting up Dictionaries and Lists with = new() in their Declarations should have taken care of most of this already). You can subscribe to events on this GameObject here, as long as you don’t expect this object to be disabled or destroyed when the event’s owner will persist.
• OnEnable - Subscribe to events here (and unsubscribe in OnDisable())
• Start - You should be free to safel access all cached components now.

Here’s why this works: This is the order of events when the scene is loaded via Portal.cs:

• LoadSceneAsync()
• Awake/OnEnable invoked on all GameObjects in Scene
• Load() is called on all objects in scene
• Player is moved
• Save() is called on all objects in scene
• Start() is called on all objects in scene
• Update() loop for all objects in scene begins.

[Cat_Hill]

@Brian_Trotter

…for (mostly) avoiding race conditions:

The key word being “mostly” I use these rules and we seem to follow them in the course and yet we still find race conditions.

What do you think of this modification to what you wrote.

• LoadSceneAsync()
• Awake/OnEnable invoked on all GameObjects in Scene
• Load() is called on all objects in scene
• Player is moved
• CheckpointSave() is called on all objects in scene
• Start() is called on all objects in scene ← this may no longer be true with what I defined above?
• Update() loop for all objects in scene begins. ← this may not be true with what I defined above?

where CheckpointSave works something like this. CheckpointSave would need to be used for any programmatically driven save (such as after a portal). The normal save would be fine for user triggered saves.

public IEnumerator CheckpointSave()
{
   yield return null;            
   GetComponent<SavingSystem>().Save(defaultSaveFile);
}

[Brian_Trotter]

It’s one of those “it works, but I don’t like it” things.

I say “mostly” because without LazyValue, lots of folks have trouble wiping out the race conditions without LazyValue.
It can, however, be done. My personal project (Spellborne Hunter) has no race conditions, and…

[Cat_Hill]

Is there some general design principle you follow to avoid the need for lazy value?

• I assume you don’t use Game Manager to manage an overall game state machine
• I assume you don’t mess with script execution order?
• do you defer operations on state until update?
• do you make heavy use of events to sync stuff up?

[Brian_Trotter]

@Cat_Hill

I assume you don’t use Game Manager to manage an overall game state machine

I loath Game Managers.

@Cat_Hill

I assume you don’t mess with script execution order?

Not even in my version of the Turn Based Strategy Course. (Though I had to jump through some hoops to make that happen)

@Cat_Hill

do you defer operations on state until update?

At least until Start()

@Cat_Hill

do you make heavy use of events to sync stuff up?

Ding Ding Ding Ding Ding Ding We have a winner!
Many of my classes have significantly more events than our course project. I come from a background of event driven programming, so it’s second nature to me.

[Cat_Hill]

@Brian_Trotter

Ding Ding Ding Ding Ding Ding We have a winner!
Many of my classes have significantly more events than our course project. I come from a background of event driven programming, so it’s second nature to me.

Nice. Though it wasn’t second nature to me 3 months ago, it is my go to now and it’s what I thought of initially for my Pickup Spawner and Pickup interaction. Do you mind sharing your code for Pickup Spawner and Pickup if you have events between those two?

[Brian_Trotter]

PickupSpawner.cs

using UnityEngine;
using GameDevTV.Saving;
using Newtonsoft.Json.Linq;

namespace GameDevTV.Inventories
{
    /// <summary>
    /// Spawns pickups that should exist on first load in a level. This
    /// automatically spawns the correct prefab for a given inventory item.
    /// </summary>
    public class PickupSpawner : MonoBehaviour,  IJsonSaveable
    {
        // CONFIG DATA
        [SerializeField] InventoryItem item = null;
        [SerializeField] int number = 1;

        // LIFECYCLE METHODS
        private void Awake()
        {
            // Spawn in Awake so can be destroyed by save system after.
            SpawnPickup();
        }

        // PUBLIC

        /// <summary>
        /// Returns the pickup spawned by this class if it exists.
        /// </summary>
        /// <returns>Returns null if the pickup has been collected.</returns>
        public Pickup GetPickup()
        {
            return GetComponentInChildren<Pickup>();
        }

        /// <summary>
        /// True if the pickup was collected.
        /// </summary>
        public bool isCollected()
        {
            return GetPickup() == null;
        }

        //PRIVATE

        private void SpawnPickup()
        {
            var spawnedPickup = item.SpawnPickup(transform.position, number);
            spawnedPickup.transform.SetParent(transform);
        }

        private void DestroyPickup()
        {
            Pickup pickup = GetPickup();
            if (pickup)
            {
                pickup.transform.parent = null;
                Destroy(pickup.gameObject);
            }
        }



        public JToken CaptureAsJToken()
        {
            return isCollected();
        }

        public void RestoreFromJToken(JToken state)
        {
            bool shouldBeCollected = state.ToObject<bool>();

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

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

Pickup.cs

using GameDevTV.UI.Inventories;
using UnityEngine;

namespace GameDevTV.Inventories
{
    /// <summary>
    /// To be placed at the root of a Pickup prefab. Contains the data about the
    /// pickup such as the type of item and the number.
    /// </summary>
    public class Pickup : MonoBehaviour, IItemHolder
    {
        // STATE
        InventoryItem item;
        int number = 1;

        // CACHED REFERENCE
        Inventory inventory;

        // LIFECYCLE METHODS

        private void Awake()
        {
            var player = GameObject.FindGameObjectWithTag("Player");
            inventory = player.GetComponent<Inventory>();
        }

        // PUBLIC

        /// <summary>
        /// Set the vital data after creating the prefab.
        /// </summary>
        /// <param name="item">The type of item this prefab represents.</param>
        /// <param name="number">The number of items represented.</param>
        public void Setup(InventoryItem item, int number)
        {
            this.item = item;
            if (!item.IsStackable())
            {
                number = 1;
            }

            this.number = number;
        }

        public InventoryItem GetItem()
        {
            return item;
        }

        public int GetNumber()
        {
            return number;
        }

        public void PickupItem()
        {
            bool foundSlot = inventory.AddToFirstEmptySlot(item, number);
            if (foundSlot)
            {
                Destroy(gameObject);
            }
        }

        public bool CanBePickedUp()
        {
            return inventory.HasSpaceFor(item);
        }
    }
}

ClickablePickup.cs

using GameDevTV.Inventories;
using UnityEngine;
using UnityEngine.InputSystem;

namespace TkrainDesigns.Inventories
{
    public class ClickablePickup : Pickup, IRaycastable
    {
        public bool HandleRayCast(GameObject controller)
        {
            if (CanBePickedUp() && controller.TryGetComponent(out PickupCollector collector))
            {
                if (Mouse.current.leftButton.wasPressedThisFrame)
                {
                    collector.StartAction(this);
                }

                return true;
            }

            return false;
        }

        public int GetPriority()
        {
            return 5;
        }

        public ECursorType GetCursorType()
        {
            return ECursorType.Loot;
        }
    }
}

Note that my IHandleRaycast includes a GetPriority() which is used to determine sorting order when multiple RaycastHits exist, for example, regardless of order, we want CombatTargets to have a higher priority than Pickups.

[Cat_Hill]

Thanks. It seems an important design pattern is to make each object responsible for its own state.

An important test is that a load save load returns the same thing after each of the two loads… but that seems complex to orchestrate so there must be a simpler way to test that.

I have a few ideas i’m going to mess with to see if I can create simple tests. My code tends to have more edge case and boundary condition checking than what Sam covers since I don’t trust myself to write bug free code.

Thanks. Very useful stuff.

[Brian_Trotter]

@Cat_Hill

My code tends to have more edge case and boundary condition checking than what Sam covers since I don’t trust myself to write bug free code.

I’ll let you in on a little secret: The course project needs a ton of edge case and null reference checking.

@Cat_Hill

have a few ideas i’m going to mess with to see if I can create simple tests

If you mean Unit tests, you’ll have to start messing around with Assembly definitions to run those, and once you start dealing with Assembly definitions, you’ll find yourself buried in Assembly definitions. That’s a whole other topic.

[Cat_Hill]

@Brian_Trotter

If you mean Unit tests, you’ll have to start messing around with Assembly definitions to run those, and once you start dealing with Assembly definitions, you’ll find yourself buried in Assembly definitions. That’s a whole other topic.

I looked into Unit tests in Unity a few months ago and it was more complexity that I wanted to take on. I also looked into assembly definitions and implemented a few. Didn’t seem too bad.

What I’ve been doing is just writing a few private methods within that class that just test what’s going on in the class. Very basic stuff that can later be a unit test. It’s a reminder for me where the landmines are. They’re probably a hack but have proven to be valuable for catching my mistakes fast.

[Brian_Trotter]

I’ve messed around with both Assembly Defs and Unit Tests. I actually use Assembly Defs for the GamedevTV scripts in my personal project, as well as for the Saving System.

The advantage of Assembly Defs, is if you do it right, you ensure that the dependencies are one way, and you create code you can easily export and port into a new game that’s ready to go.

Unit Tests require that the tested code be in an Assembly. One project I’m working on is basically going through the course again (I do that from time to time to keep my head in the game!), but putting my own improvements into the code. I’ve added in Unit Testing to that (some things don’t make sense, but you can easily test to ensure that values can’t get out of range, that events fires, etc). This does mean that the entire codebase will be in modular assemblies when it’s done (it’s low on my priorities, so who knows when).

Generally speaking, my testing is done through lots of Debug.Logs, Debug.Asserts, and when needed, code that hunts down the corner cases. Ideally, I like to eliminate corner cases whenever possible.

[Cat_Hill]

@Brian_Trotter

The advantage of Assembly Defs, is if you do it right, you ensure that the dependencies are one way, and you create code you can easily export and port into a new game that’s ready to go.

Totally!

@Brian_Trotter

Generally speaking, my testing is done through lots of Debug.Logs, Debug.Asserts, and when needed, code that hunts down the corner cases. Ideally, I like to eliminate corner cases whenever possible.

Ok I don’t feel so bad. I do the same, incl
hunting corner cases. A great thing about this course is getting wide exposure to many corner cases and their solutions.