Update itemList on equipmentUpdated?

[AkakiAkakievitch]

Hi there,
I encounter a problem I can’t solve easily.
It happens that my UI looks like that:

So it is very tempting, during shopping, to interact with the Equipment panel.
It becomes a real headache when you are in selling mode.
In fact, the shop list doesn’t updated correctly, because I don’t have an event that traps the inventory changes.
I tried to subscribe to the equipmentUpdated event, but it still doesn’t work fine:

• When you drag and drop an item from the inventory to the equipment, its ok (the quantities in shop list in selling mode update as expected)
• But when you drag and item from your equipment to your inventory, it doesn’t work (ie the list doesn’t update until next UI refresh…)
  That’s because, in the DragItem.cs, the source is always processed (addItem or removeItem) before the destination (and the equipmentUpdated event is -indirectly- invoked by addItem and removeItem of the EquipmentSlotUI).
  I can’t see any “good” solution there…
  Should I go on and implement an “inventoryUpdated” event? I feel that it wouldn’t be very efficient when it comes to confirm a selling transaction with a lot of items in it…
  Should I implement a coroutine that waits for a small amount of time in the method that I subscribe to the equipmentUpdated?
  Any other suggestion?
  Thanks!

PS: Honestly, it looks like the problem is more on the UI side, for me.
If it had to start from a blank page, I would go on and design a UI shop inspired from Diablo II: a panel for the player’s inventory, a panel for the shop keeper’s inventory and the drag an drop mechanism between those two panels does the transaction. But I don’t feel like restarting from scratch for the moment…

[Brian_Trotter]

What’s tricky is that if the shop responds to inventoryUpdated when confirming a transaction, you’re likely to crash the game entirely…
inventoryUpdated already creates a bit of a nightmare when buying and selling large quantities of items (especially big stacks) because each individual item in a transaction with Inventory triggers inventoryUpdated, which destroys and rebuilds the InventorySlotUI objects, leading to excess garbage collection. I crafted a solution for that issue here: Big problems with how UI is being redrawn - #6 by Brian_Trotter

In terms of crafting a solution for the inventory changing while the shop window is open, I’ll have to take a look at this over the weekend. One simple solution might be to subscribe to inventoryUpdated (to have it redraw the shop window), but at the beginning of ConfirmTransaction, unsubscribe from the event (to prevent a potentially cataclysm) and re-subscribing at the end of ConfirmTransaction… but… there are some things that need to be factored in (for example: If I have something set to sell, but I equip it before confirming, then that needs to be backed out).

[AkakiAkakievitch]

Thank you for your answer and for the very interesting link.

All put together, I think I managed to come up with a solution.
I don’t subscribe to the equipmentUpdated anymore but I do subscribe to the inventoryUpdated.
There I add references to two methods:

currentShopper.TryGetComponent<Inventory>(out shopperInventory);
shopperInventory.inventoryUpdated += RefreshCurrentStock;
shopperInventory.inventoryUpdated += TryTriggerOnChange;

• One that basically refreshes the item list presented in the shop

private void RefreshCurrentStock()
{
    if (!confirmingTransaction)
    {
        if (IsBuyingMode())
        {
            currentStock = shopStock;
        }
        else
        {
            BuildShopperStock();
            currentStock = shopperStock;
        }
    }
}

(NB: it looks a little bit strange because I added the ability for the players to sell items that are not available in the initial stock of the shop)

• And another reference to a method that invokes the event that triggers the UpdateUI in ShopUI:

private void TryTriggerOnChange()
{
    if (onChange != null && !confirmingTransaction)
    {
        onChange();
    }
}

You’ll note that these two methods are meant to run only under some condition, ie !confirmingTransaction.
Now, the confirmingTransaction boolean is always false, unless we are actually executing the ConfirmTransaction method:

public void ConfirmTransaction()
{
    if (currentShopper == null) return;
    if (shopperInventory == null || shopperPurse == null) return;
    if (IsBuyingMode() && !CanPay()) return;

    confirmingTransaction = true; //No more UI updates from here

    //Item transfering
    int transactionDirection;
    if(IsBuyingMode())
    {
        transactionDirection = -1; //From shop to shopper
    }
    else
    {
        transactionDirection = 1; //From shopper to shop
    }

    foreach (InventoryItem item in transaction.Keys)
    {
        int quantity = transaction[item];
        if (quantity > 0)
        {
            //Manages the process of adding items to / removing items from the inventory
            //Manages the stackability of the item
            ProcessItemTransfert(item, quantity);
        }
        //The shop stock has to be updated, adding items (if player is selling) or removing items (if player is buying)
        AddToShopStock(item, transactionDirection * quantity);
    }

    //Fund transfering, one shot, using a cached float: transactionTotal
    shopperPurse.UpdateBalance(transactionDirection * transactionTotal);
    

    confirmingTransaction = false; //UI can be updated again
    TransactionReinit();
    RefreshCurrentStock();

    //Call onChange
    TryTriggerOnChange();
}

That should do the trick, without impacting too much the performances, I think.
Do you see any flaw in the logic or in the code? (parts of code a lacking, I can add them if needed).

[AkakiAkakievitch]

@Brian_Trotter

for example: If I have something set to sell, but I equip it before confirming, then that needs to be backed out

For this particular point, I must admit that I have to recreate the transaction dictionary.
Here it goes (this method is called at the end of the BuildShopperStock(), added at the end of the block, for better understanding. FYI, it’s this ):

private void CheckTransactionQuantities()
{
    Dictionary<InventoryItem, int> tmp = new Dictionary<InventoryItem, int>();
    foreach(InventoryItem item in transaction.Keys)
    {
        if(currentStock.ContainsKey(item))
        {
            tmp[item] = Mathf.Min(transaction[item], currentStock[item]);
        }
    }
    transaction = tmp;
}


private void BuildShopperStock()
{
    shopperStock = new Dictionary<InventoryItem, int>();

    for (int i = 0; i < shopperInventory.GetSize(); ++i)
    {
        InventoryItem itemInSlot = shopperInventory.GetItemInSlot(i);
        int numberInSlot = shopperInventory.GetNumberInSlot(i);
        if (itemInSlot == null) continue;

        if (!shopperStock.TryAdd(itemInSlot, numberInSlot))
        {
            //item already exists in stock
            shopperStock[itemInSlot] += numberInSlot;
        }
    }

    CheckTransactionQuantities();
}

So, yes, it will be called every time we update the inventory, unless we are executing ConfirmTransaction (which seems to me, by far, the most inventory update intensive process here).
At the end of the day, it will execute “only” when the player swaps the content of its inventory slots, changes its equipment or drops items…

I can’t wait to see your solution!
Thanks!

[Brian_Trotter]

Actually, I’m not seeing any flaws in this methodology. As long as we’re not trying to act on inventoryUpdated in the middle of a ConfirmTransaction, which you’ve taken care of with a boolean rather than subscribing/unsubscribing.

So here’s the tradeoff between using a blocking approach (your boolean) and using a Do Not Call approach (unsubscribing from the method in ConfirmTransation and resubscribing at the end…

The smaller quantity of items (and each element of a stack is +1 to the quantity), the more performant the blocking method will be. The larger the quantity of items, the more performant the Do Not Call approach will be. Of course, without the adjustments from the link I mentioned earlier, the less performant the overall approach will be when dealing with multiple items.

[AkakiAkakievitch]

@Brian_Trotter

The smaller quantity of items (and each element of a stack is +1 to the quantity), the more performant the blocking method will be. The larger the quantity of items, the more performant the Do Not Call approach will be.

I don’t get why.
When you unsubscribe to the event, you still have to test if the event onChange == null (which will be), so it will be one bool operation.
When using the boolean blocking method, you test if !confirmingTransaction, which is one bool operation too.
I don’t get the difference of performance. What am I missing?

[Brian_Trotter]

Actually, inventoryUpdated will seldom be null, as the InventoryUI will have subscribed to it.
Each subscriber requires that the subscribing event has to be called which pushes the return address onto the stack.

if(event) //1 operation
event();  //push return address to stack 1 operation, branch to location 1 operation
if(confirming) //1 operation
return; //1 operation

vs

if(event) // 1 operation

[AkakiAkakievitch]

Of course. So stupid I asked!
Forgot about the fact that it triggers the event anyway and calls back the function!
Sorry!
Thus, I´ll probably turn it into unsubscribe / subscribe.
To be honest, I’m always a little bit reluctant to do subscribe / unsubscribe out of the standard start / OnDestroy or OnEnable / OnDisable.
I don’t know, I alway fear that something goes wrong and I begin a memory leak somehow.
Just like using while loop and the endless loop curse, you know? Kind of dark magic phobia
Anyway… these are my demons!
Thanks for your advice!

[Brian_Trotter]

@AkakiAkakievitch

To be honest, I’m always a little bit reluctant to do subscribe / unsubscribe out of the standard start / OnDestroy or OnEnable / OnDisable.
I don’t know, I alway fear that something goes wrong and I begin a memory leak somehow.
Just like using while loop and the endless loop curse, you know? Kind of dark magic phobia
Anyway… these are my demons!

To be honest, I’ve probably re-enforced that subscribes/unsubscribes should be paired up in Start/Awake → Destroy and OnEnable → OnDisable many many times over the various courses that I TA. It’s an oversimplification of the rules. The general rule is that every subscription must be matched by an unsubscription at the same scope…
So Awake/Start goes with OnDestroy because they are events of the same depth, occurring only once at the beginning and ending of the component’s lifetime. OnEnable and OnDisable similarly correspond to the same depth, a component being enabled or disabled.

In the case of ConfirmTransaction, if we’re unsubscribing in the start of the method and subscribing at the end, we’re also matching, and at the end of the method, the state of the subscription should be right where we left it. It’s a bit inverted from the way we’re used to doing things, but a perfectly valid and safe operation.

While loops are their own demons, right next to recursive functions. They both have their uses, but unless carefully crafted can lead to unexplained hangs or outright crashing the editor or game.

[AkakiAkakievitch]

@Brian_Trotter

While loops are their own demons, right next to recursive functions.

I can see that both of us have been there in the past
Anyway, one day I’ll go beyond my fears and dare to implement subscribe/unsubscribe events other than in the academic way!
Thank you for your answers and your time!
I’ll keep on training!