Concettenation of Actions

[BulbasaurLvl5]

Hey,

so yesterday everything worked and today…

So here is my Problem:

class1 is there to detect if an itemslot is clicked

public class ItemSlotBehaviour : MonoBehaviour, IPointerClickHandler
{
    public event Action<Item> OnShiftLeftClickEvent;

    public void OnPointerClick(PointerEventData eventData)
    {
        if(eventData != null && eventData.button == PointerEventData.InputButton.Left)// && Input.GetKey(KeyCode.LeftControl))
        {
            Debug.Log(OnShiftLeftClickEvent);
            if (Item !=null && OnShiftLeftClickEvent != null)
            {
                Debug.Log("ItemSLot.PointerClick");
                OnShiftLeftClickEvent(Item);
            }
        }
    }

the second class just know about the slots and is an intermediate for the action:

public class StashBehaviour : MonoBehaviour
{
    //[SerializeField] List<Item> items = new List<Item>();
    public List<Item> items = new List<Item>();
    [SerializeField] Transform itemSlotsParent;
    [SerializeField] ItemSlotBehaviour[] itemSlots;

    public event Action<Item> OnItemInStashShiftClickedEvent;

    private void Awake()
    {
        foreach(ItemSlotBehaviour isb in itemSlots)
        {
            //Debug.Log(isb.name);
            isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent;
            isb.OnShiftLeftClickEvent += TestAction;
        }
    }

    public void TestAction(Item item)
    {
        Debug.Log(this.name + item);
    }

So far everything works, TestAction(item) is executed just fine, tells me the item etc and Debug.Log(OnShiftLeftClickEvent); from the first class tells me there is indeed an action

The third class now should do something

public class InventoryBehaviour : MonoBehaviour
{
    [SerializeField] EquipmentBehaviour equipment;
    [SerializeField] StashBehaviour stash;

    private void Awake()
    {
        stash.OnItemInStashShiftClickedEvent += EquipFromStash;
        equipment.OnEquipmentShiftClickedEvent += UnequipFromEquipment;
        stash.OnItemInStashShiftClickedEvent += TestAction;
        stash.TestAction(stash.items[0]);
    }

    public void TestAction(Item item)
    {
        Debug.Log(this.name + item);
    }

And here for some reason i cannot add a function to the second class’s action, i.e. stash.OnItemInStashShiftClickedEvent += TestAction; does not add the function. If i remove TestAction from the second class (stashbehave) the first debuglog correctly tells me that the action is null. However i can execute stash.TestAction(stash.items[0]); just fine, which tells me that stash is correctly set and, thus, the action should be correctly applied.
Also stash.OnItemInStashShiftClickedEvent += TestAction; does not show any error and it is all the same as for StashBehaviour… I can not see where things do not work.

thanks in advance

[Kurt-Dekker]

I feel like what you are doing is certainly 100% reasonable, so there must be some key step missing or setup missing, and I don’t see it from the code above. I’m sure that adding multiple listeners to an event did not magically stop working, so I have to conclude your code flow isn’t quite what you think it is.

To help gain more insight into your problem, I recommend liberally sprinkling (EVEN MORE!) Debug.Log() statements through your code to display information in realtime.

Doing this should help you answer these types of questions:

• is this code even running? which parts are running? how often does it run?
• what are the values of the variables involved? Are they initialized?

Knowing this information will help you reason about the behavior you are seeing.

You could also just display various important quantities in UI Text elements to watch them change as you playtest.

If you are running a mobile device you can also view the console output. Google for how on your particular mobile target.

Here’s an example of putting in a laser-focused Debug.Log() and how that can save you a TON of time wallowing around speculating what might be going wrong:

Coroutine missing hint and error

This is so unnecessary. Use the beauty and power of instantly and immediately putting a Debug.Log() statement into the very start of the coroutine, right at the top. Do this always at the very first sign of trouble or question in your mind. The instant you did this you would see “Oh man, this code is simply not getting run!” And you could LASER-FOCUS 100% your efforts on studying what you expect to run it, eg., the call location, and isolate why it is not running. You would look at literall…

[Bunny83]

You have a race condition here and the offending line is this one:

isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent;

Note that delegates are immutable classes. So you can not modify the content of a delegate (only through reflection). When you “add” a method or delegate to a multicast delegate, you essentially create a new delegate with the Delegate.Combine method.

That means when the offending line is executed, you combine the delegate in OnShiftLeftClickEvent with the the delegate that is currently stored in OnItemInStashShiftClickedEvent into a new delegate which will replace the old one in OnShiftLeftClickEvent. That means if you later change / replace the delegate “OnItemInStashShiftClickedEvent” it won’t affect what is stored in your “OnShiftLeftClickEvent”.

What you can do is use a closure or an explicit calling method like this:

public event Action<Item> OnItemInStashShiftClickedEvent;
private void InvokeItemInStashShiftClickedEvent(Item item))
{
    OnItemInStashShiftClickedEvent(item);
}

Now in your subscription code, you subscribe this method and not the delegate from the event field. So now you would do

isb.OnShiftLeftClickEvent += InvokeItemInStashShiftClickedEvent;

This should work as you had intended.

[BulbasaurLvl5]

Hey,

thanks for taking your time to look over the code. I did sprinkle Debug.logs around and that is the reason i came to the conclusion that the thirds class function is not added to the seconds class action. I did try to add further logs, but things like class3{debug.log(class2.action)} are forbidden

i rewatched the tutorial (min 7 - min9) and he literally did the same thing. I also added new actions to the classes with minimal code and it is the same result. Other than that i do not have any idea how i could miss some set up, as the the only set up involved is the [Serializefiled] stash, which i checked about 10times by now

@Bunny83

You have a race condition here and the offending line is this one…

ok, give a moment to understand this[/QUOTE]

[lordofduct]

[edit]Looks like while I was typing this up Bunny83 basically said the same thing I’m saying here[/edit]

OK… I’m not fully understanding what exactly you’re trying to describe.

BUT

I do notice a huge issue in your code:

public class StashBehaviour : MonoBehaviour
{
    //[SerializeField] List<Item> items = new List<Item>();
    public List<Item> items = new List<Item>();
    [SerializeField] Transform itemSlotsParent;
    [SerializeField] ItemSlotBehaviour[] itemSlots;

    public event Action<Item> OnItemInStashShiftClickedEvent;

    private void Awake()
    {
        foreach(ItemSlotBehaviour isb in itemSlots)
        {
            //Debug.Log(isb.name);
            isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent; //RIGHT HERE
            isb.OnShiftLeftClickEvent += TestAction;
        }
    }

This line of code is an issue. You’re adding your event OnItemInStashShiftClickedEvent to isb.OnShiftLeftClickEvent during Awake.

Here’s the thing the “event” keyword is effectively just an access modifier (there’s a little more to it… but you can just think of it as one). It basically says “only the owning class can call this delegate or set it, and anyone else can add/remove delegates to its multicast list (as long as the public/private/internal is respected)”.

So when you say:

isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent;

I get the feeling you believe this would forward OnShiftLeftClickEvent to OnItemInStashShiftClickedEvent. If more listeners are added to OnItemInStashShiftClickedEvent, then OnShiftLeftClickEvent would continue forwarding.

But that’s NOT what will happen.

When you say:

isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent;

Really what you’re doing is adding the current multicast list that is added to OnItemInStashShiftClickedEvent at time of += will be added.

This is an issue since you’re doing this in Awake. So over in InventoryBehaviour:

public class InventoryBehaviour : MonoBehaviour
{
    [SerializeField] EquipmentBehaviour equipment;
    [SerializeField] StashBehaviour stash;

    private void Awake()
    {
        stash.OnItemInStashShiftClickedEvent += EquipFromStash; //HERE
        equipment.OnEquipmentShiftClickedEvent += UnequipFromEquipment;
        stash.OnItemInStashShiftClickedEvent += TestAction; //AND HERE
        stash.TestAction(stash.items[0]);
    }

You’re adding these to StashBehaviour. But that doesn’t mean that when ItemSlotBehaviour.OnShiftLeftClickEvent raises these will necessarily raise.

It hinges on WHEN Awake is called. If InventoryBehaviour has Awake called first… it will be forwarded on. But if Awake is called after it will not. And the order of Awake isn’t guaranteed (unless you explicitly set the execution order and both objects exist at load).

To put it more simply try this:

public class Test : MonoBehaviour
{

    void Start()
    {
            var a = new Foo();
            var b = new Bar();
            b.Blargh += () => Debug.Log("DID IT EARLY");
            b.Register(a);
            b.Blargh += () => Debug.Log("DID IT LATE");

            a.DoBlargh();
    }

    class Foo
    {
        public event Action Blargh;

        public void DoBlargh()
        {
            Blargh?.Invoke();
        }
    }

    class Bar
    {
        public event Action Blargh;

        public Bar()
        {

        }

        public void Register(Foo foo)
        {
            foo.Blargh += Blargh;
        }
    }

}

If you ran this you will notice that “DID IT EARLY” will display, but “DID IT LATE” will NOT display. Because of the order that you added the events.

[lordofduct]

@BulbasaurLvl5

i rewatched the tutorial (min 7 - min9) and he literally did the same thing. I also added new actions to the classes with minimal code and it is the same result. Other than that i do not have any idea how i could miss some set up, as the the only set up involved is the [Serializefiled] stash, which i checked about 10times by now

I will point out… tutorials aren’t necessarily bullet proof. They can often contain bugs. It’s one of the big reasons I’m not a fan of tutorials… especially ones from unofficial sources.

This even goes for us here on the forums. We can only offer the knowledge we have and we too can make mistakes. It’s why I suggest never copying pasting code from other people especially if you don’t understand what the code they’re handing you does. Especially if you’re attempting to learn.

[BulbasaurLvl5]

@lordofduct

This is an issue since you’re doing this in Awake. […] It hinges on WHEN Awake is called.

Hey, i also do not totally understand what your saying but i think i can boil it down to this? If i understand that correctly i already had the same thought and did put the last function addition into start (which should be called after awake) and also in update (because i was desperate), with no change in results

€: but yeah the idea i this: create action → add action2 → add function to action2, thus, function is called when action1 is called

[lordofduct]

Changing the order to Start is not going to fix what is effectively code smell.

This:

    public event Action<Item> OnItemInStashShiftClickedEvent;

    private void Awake()
    {
        foreach(ItemSlotBehaviour isb in itemSlots)
        {
            //Debug.Log(isb.name);
            isb.OnShiftLeftClickEvent += OnItemInStashShiftClickedEvent; //RIGHT HERE

This should never be done. It’s just bad code on the part of who wrote the tutorial.

If you want forwarding of event’s Bunny83 is offers up a way to forward events.

…

As to if this is your problem… no idea. I don’t fully understand what your problem is. You say:

And here for some reason i cannot add a function to the second class’s action, i.e. stash.OnItemInStashShiftClickedEvent += TestAction; does not add the function. If i remove TestAction from the second class (stashbehave) the first debuglog correctly tells me that the action is null. However i can execute stash.TestAction(stash.items[0]); just fine, which tells me that stash is correctly set and, thus, the action should be correctly applied.
Also stash.OnItemInStashShiftClickedEvent += TestAction; does not show any error and it is all the same as for StashBehaviour… I can not see where things do not work.

What do you mean by “I cannot add a function to the second class’s action”? Are you getting an error? Is it not behaving the way you expect? What is happening? I believe you’re saying it’s not behaving the way you expect and the code Bunny83 and I pointed to is a potential cause of it because that specific line of code is unintuitive and generally just bad code on the part of the tutorial writer.

Instead of using arbitrary language (and this is why I gave example code at the end of my post to try to demonstrate what it is I was explaining)… describe what you expect as results, and what you get as results.

“I Expect the message log to say ‘a,b,c’; but instead I get ‘a,c’.”

[BulbasaurLvl5]

Bunnys solution did the job, but i do not understand why. If you guys would bear with a bit longer i would be again thankful.

I’ll try to explain with some pseudo code

What i want is one class that has an action

class1
{
action1
}

a second class with another action, that “adds” to the first action

class2
{
action2

class1.action1 += action2 //you guys pointed out that this is the problem
}

to finally add a function from a third class

class3
{
function3

class2.action2 += function3
}

what i did expact was: i call action1 and through “chain execution” function3 is executed. What did happen was, that unity told me that action1 is null.

Bunnys solution is to wrap action2 into another function

class2
{
action2
functionInvokeAction2

class1.action1 += functionInvokeAction2 //this seems to solve the race condition

}

However the part that i do not understand is this:

@Bunny83

Note that delegates are immutable classes. So you can not modify the content of a delegate (only through reflection). When you “add” a method or delegate to a multicast delegate, you essentially create a new delegate with the Delegate.Combine method.

The part i do not understand is “…When you “add” a method or delegate…”, i would expect that i does not matter if i add a delegate (before) or a function (now) but apparently it does.

If it would make a difference, this part would make sense to me:

@Bunny83

That means when the offending line is executed, you combine the delegate in OnShiftLeftClickEvent with the the delegate that is currently stored in OnItemInStashShiftClickedEvent into a new delegate

My take-home-message would be: Do not add a delegate to another delegate

€ my guess would be: the “race” is if class3 manages to add function3, before the delegates are merged. If it is to late, than the delegates are already merged and a function added to action2 will not be handed to action1 (which is a new delegate by now). Thus, by moving function3 assignment to start, should make the chain not work, which happened. Then also moving class1.action1 += action2 into start should “solve” the issue, because the delegates will only be merged after function3 is added, which i just tried and also “solved” the issue

€€ awake should be executed in order of the parent objects in the editor right? Then, everything should have worked out, because the first thing that should be done is class3 execution. maybe that is what you meant with execution order is not set