I have a situation where I invoke an event
public event Action<float> OnDamage;
OnDamage?.Invoke(amount);
and subscribe it:
core.Combat.OnDamage += (a) => { stateMachine.ChangeState(player.HurtState); };
But I can’t unsubscribe lambda expression, how to unsubscribe it then?
core.Combat.OnDamage -= (a) => { stateMachine.ChangeState(player.HurtState); };
It should be clear that a lambda defined twice even with the same code is not the same lambda function. Define your lambda as the action you want to (un)subscribe to.
public event Action<float> OnDamage;
Action<float> myHandler = (amount) => { Debug.Log(amount); };
private void OnEnable()
{
OnDamage += myHandler;
}
private void OnDisable()
{
OnDamage -= myHandler;
}
private void Update()
{
OnDamage?.Invoke(100.0f);
}
You need to store the lambda in a variable in order to remove it from an event.
Hello,
I have the same problem with a different case and I don’t know if it is possible to apply this method.
Here is the context:
I developing the inventory of my game and I use the Action type from the System as delegate event. When I had an item to my bag, I would like to the slot added to this bag be removed when the slot reaches an amount of 0.
public class Slot {
ItemData data;
int amount;
public Action OnNull;
public bool Remove(int value) {
if (amount - value >= 0) {
amount -= value;
if (amount == 0) {
OnNull?.Invoke();
return true;
}
}
return false;
}
}
public class Bag {
List<Slot> slots = new List<Slot>();
public void Add(Slot slot)
{
slot.OnNull += () => Remove(slot);
slots.Add(slot);
}
public bool Remove(Slot slot)
{
slot.OnNull -= () => Remove(slot);
slots.Remove(slot);
}
}
I tried to use the method given as an answer but it did not work since I have a variable in my lambda function I guess.
Do you have a solution to realize it, or it is a bad way of coding and should I rework my code architecture?
(Just before I begin, your Action should be preceded by the keyword event. You can use delegates like events, but they are less safe. Some external actor could erase all of the observers of that delegate. If you put “event” in front, then it’s just more encapsulated properly.)
Both you, and the original poster are using lambdas as closures. Closures are lambdas that refer to things other than the local data that was specifically passed into the method as arguments, or otherwise exists locally within the method. I can hardly blame you because I see this all the time! I see it in official Unity presentations. I see it in official .NET documentation and examples. It’s rampant. Many programmers just seem not to think much about this issue. Newer versions of C# allow you to put the static keyword in front of your lambdas to ensure that they can never accidentally become closures, but that is not supported yet in Unity.
When you create a lambda, the compiler automatically generates code for you, creating a new class, with a method, and an delegate instance which points to the class method. Lambdas are just syntactic sugar so that you don’t have to write that all out for yourself. If the lambda is not a closure, then there will only be one single, static delegate instance that ever gets instantiated. It knows that it only needs the data that you pass in, and so it can essentially be used like a static method, and you’ll only ever need a single instance of that delegate. It is lazy loaded, and it never gets discarded for garbage colleciton. This is usually great because now every time that line of code is encountered, no new delegate instance is instantiated. Delegates are objects! They are instances of the MulticastDelegate class.
As MelvMay pointed out with their solution, the compiler will look at two separate lambdas with identical contents and create two separate classes, with two separate methods. They will not be equatable. So, assigning the single expression of that lambda to a cached variable lets you add and remove it as an observer of an event because the cached delegate reference will be equatable.
As you have seen, now that your lambda is no longer contained within some other method, it can no longer reference the slot argument which was local to that method.
So, what should you do? Well, first of all, to get the slot value into the delegate, simply pass it as a parameter. Change the event type from Action to Action, and invoke it from the slot that is empty with OnNull?Invoke(this). The lambda expression would be rewritten as slot => Remove(slot). At this point you should be able to cache an instance of the lambda in the Bag class.
However, this is still not as good as it could be because the lambda is still a closure! Why? Because it refers to Bag.Remove(Slot), and the actual implementation method of the lambda is not in the bag class. So, the class that the compiler generated which contains the actual implementation of your lambda now needs to know WHICH Bag it should call Bag.Remove(Slot) on.
Edit: The following doesn’t apply to this situation. I was just spitting this off the top of my head, but whenever possible it is better IMHO to have one single static delegate instance pointing to the relevant method of a class, so all instances of the class just share one delegate. It’s not applicable with the direction of control and such in this example.
So, now if you try to make the delegate member cached within Bag static, which you should because if you have 5 bags they don’t need 5 different lambdas, I bet the compiler would complain because Bag.Remove(Slot) is not a static method. So, when you create different bags, and they all point to the same lambda expression as the delegate they want to cache, the compiler will generate 5 different instances of the class underlying your lambda because it needs 5 different Bag references.
So, now what? Stop using a lambda! Just cache a delegate that refers directly to the Bag.Remove(Slot) method! A lambda that just points directly to that method is just creating middleman classes unnecessarily. IMHO people should really only be using lambdas for NON-closures, unless they are 1000% sure they know what they’re doing and why. It will be really nice when the static lambda keyword is supported.
Now, it doesn’t apply to this case because of the way everything is set up, but in other use cases you could now make the cached delegate static. So, no matter how many instances of the class you have only one MulticastDelegate object will ever be instantiated, just like non-closure lambdas. I’ve started doing this pretty much religiously, even if I could use a lambda because it’s like my own version of static lambdas, until static lambdas are supported. It ensures that nobody can accidentally mess something up by changing the code, accidentally generating more code under the hood with difference performance characteristics that now generates garbage memory for GC. It only takes a few more lines of code to cache your own delegate object instances, and it forces you to think directly about what’s actually happening under the hood. At the end of the day it’s the exact same the compiler is doing for you behind the scenes even when you write a lambda properly, as a non-closure.
public class Slot
{
int amount;
public Action<Slot> OnNull;
public bool Remove(int value)
{
if(amount - value >= 0)
{
amount -= value;
if(amount == 0)
{
OnNull?.Invoke(this);
return true;
}
}
return false;
}
}
public class Bag
{
private List<Slot> slots = new List<Slot>();
// This allows other classes to refer to the cached delegate object instance that points to this Bag's Remove method, and it will only be lazy loaded if it's ever needed.
public Action<Slot> RemoveAction => _removeAction ??= Remove;
private Action<Slot> _removeAction;
public void Add(Slot slot)
{
slot.OnNull += RemoveAction;
slots.Add(slot);
}
// Added this Action<Slot> version of the method group.
public void Remove(Slot slot)
{
slot.OnNull -= RemoveAction;
slots.Remove(slot);
}
// Your version returns bool, so you would need to changed everything over to Func<bool, Slot>, but that doesn't make sense with events.
//public bool Remove(Slot slot)
//{
// slot.OnNull -= RemoveFunc;
// return slots.Remove(slot);
//}
}
If you don’t use lambdas, and you don’t cache specific delegate instances that refer to the relevant object+method, then every time you write something like slot.OnNull += Remove, a new delegate instance will get automatically created which creates unnecessary pressure on the garbage collector. The += add and -= remove equality comparisons still work even though you’re creating a bunch of different delegate instances because equality is overloaded for delegates so that if they both refer to the same object and the same method, then they are equal, even if they are not the same delegate instance. That’s why we even cache the RemoveAction delegate instance on the class at all, so we’re not automatically calling new MulticastDelegate(Remove) every time we call slot.OnNull += Remove without realizing it. Static lambdas automatically implement all of this awesomeness, which is why I think people use lambdas so much, but then they don’t make their lambdas static, they turn them into closures!!! 
Note that I am in know way commenting on the actual patterns in your code, like how Bag and Slot are designed. I’m not criticizing either. Just not commenting on the code itself. This is just my public service rant about lambdas, closures, and caching your own delegate instances, etc.
Woww !!! Thank you so much for this answer very rich and well explained.
Very nice tips, I was looking for this for a while and tried with private get and public set but I was not able to do the += on the action, now I am. Thank you !
I do not know this operator, and not sure to understand it.
I see that you added “private” before, I used not to do it because it is the same, isn’t it? Should I continue or not?
I the beginning I wanted to make as a bool return function but I changed my mind and forgot to change it.
public Action<Slot> OnNull;
I am very surprised to see that, I thought to this solution but to me, I made no sense of doing that since slot contains the the action to which we can subscribe so it means that we have access to slot.
I wrote the most important lines to make you understand my problem but they are a lot more and I really don’t know I to manage it with the events, bags, slots, equipments, which could be controlled by the code and the UI.
I am really confused if I am doing things right or not since I have been working on my invenroty for weeks now without seeing the end of the tunnel.
I am still trying to find how to better architecture my code since working with a well coded program is much more pleasant.
If you have any other solutions, they are welcomed.
Sorry about that. I like to use the null-coalescing assignment operator here because I think it looks clean, but I totally understand that it looks foreign to some. I go on and on about clarity, but then I use this obscure operator. 
That code is equivalent to this:
private Action<Slot> _removeAction;
// You can get the Remove delegate reference publicly,
// but instantiating the instance is handled privately.
public Action<Slot> RemoveAction
{
get
{
// Lazy load, instantiate only if requested.
if(_removeAction == null)
_removeAction = Remove; // Short version
// or _removeAction = new Action<Slot>(Remove);
// See how it's obvious now that the delegate is an instance of a wrapper object
// which points to this object and its Remove method?
return _removeAction;
}
}
However, you should never use null coalescing operators with any Unity object that is derived from UnityEngine.Object. It will not always provide accurate results. Unity overrides the == operator so that an object that has been “Destroyed()” will appear to be null, even though it is not technically null. However, the null coalescing operators do not share that behavior, so they will treat an object that has been destroyed as a live object. But, in this case, delegates are not Unity engine objects. So, it’s totally fine, and I like the way it looks. But, it’s understandable that what looks more readable to me may be less readable to others.
That is just my personal preference. It is private by default. I prefer to explicitly label the scope of everything. It doesn’t change anything functionally.
Delegates reference objects in a way that can sometimes seem backward logically. It is the object that invokes the event which keeps all of the references to the objects that will observe the event and receive the invocation, not the other way around. So, if ObjectA subscribes to an event on ObjectB, it is ObjectB that is now holding a reference to ObjectA. If you nullify all of your other references to ObjectA, but ObjectA never unsubscribed from the event, ObjectB will keep a reference to ObjectA and it will not be garbage collected. It will continue receiving the event. So, it’s the invoker, Slot, which is holding a reference to Bag, simply because Bag subscribed to Slot’s event.
There are so many ways to set up these relationships. You could maintain your own lists of objects and know which methods you want to call, this doesn’t have to be handled with events. You can have events going in either direction. They could all be communicating through a 3rd party class that operates like a messaging service. I hesitate to dive into inventory architecture advice, but hopefully some of what I said adds more clarity than confusion.
Oh, one other thing I should mention is that you don’t always need to cache a delegate instance. If something only ever subscribes to an event once, at the start of the application, and then it never unsubscribes, or only does so at the end of the application for graceful cleanup, then it’s not really necessary.
Caching an instance just reduces the amount of garbage that is generated when adding and removing the delegate to and from an event. If you don’t cache the instance, each time you say SomeEvent += MyMethod or SomeEvent -= MyMethod, a new delegate object is instantiated and then thrown away. If that happens constantly because your object is often subscribing and unsubscribing, then caching will generate less garbage. However, caching comes at a cost. It increases the memory footprint of that class. It’s by a relatively small amount, but it’s not free. Everything is a tradeoff. So, if a class only subscribes once, it may as well just get garbage collected that once.
That is sort of the reason why I mentioned that if you can manage to make the method static, and you can cache a static delegate instance, then all instances of the class can just use that one delegate. But, if you can make it static, then it may as well be a lambda! (Except that a lambda is more fragile, it can accidentally become a closure.) So, it’s a bit circular.
Also, you cannot completely mitigate the garbage issue because garbage will be generated when you += and -= anyway, just LESS garbage if you cache a delegate instance.
Anyway, I feel like I’m going in circles now.
Thank you very much for your time and your explanations.
First:
If I have to do a quick recap, I always should add the variable into the Action cause it this one which carry the information and the subscriber which treat when it corresponds.
public class Slot {
public event Action<Slot, int> OnAdded;
public event Action<Slot, int> OnRemoved;
public event Action<Slot, int> OnChanged;
public event Action<Slot> OnNull;
}
Second:
If a class (like my Player class) only subscribe once to its bag events and unsubscribe at the end of application, it is fine not to use the static method.
When you say, “graceful cleanup”, what happen if I don’t do it because for now I am not and I would like to understand the cons of it.
Like CodeRonnie said, it’s the null-coalescing operator.
A??=B is a short hand for A = A ?? B
The ?? operator does a reference null check on the first argument and if it is not null, it just returns the first argument. However if the first argument is null, it will returns the second one. So A ??= B essentially does this:
if (Object.ReferenceEquals(A, null))
A = B;
else
A = A;
As a single line expression:
A = (Object.ReferenceEquals(A, null))?B:A;
Note that unlike the majority of operators, the null-coalescing operators are “right associative”. That means when you chain them, they are “grouped” from right to left. However the evaluation still happens from left to right.
So if we have x = a ?? b ?? c it is grouped like this x = a ?? (b ?? c). This can be confusing at first. Though note that the ?? operator does “short-circuit” or early exit when it is evaluated. So in this expression, when “a” is not null, “a” will be assigned to “x” and that’s it. The right side won’t be evaluated at all. So when you have a function call in there, that won’t happen. The right side is only evaluated when the left side is null. This can be really confusing in cases like
b = null;
x = a ??= b ??= MyFunction();
In this case if a is not null, x will get the value of “a”. “b” will stay at null and MyFunction is never called.
In the case where a is null, the right side is evaluated. Here since b is also null, the nested expression is also evaluated so MyFunction is called. The return value is assigned to “b” and in turn assigned to “a” and in the end also to “x”.
Though you should avoid constructs that are hard to understand or to reason about. So the ??= operator should only be used “once” in an expression.
Finally note that ?? and ??= does not use the == operator to check for null but does a direct null check as I’ve shown above. That’s why it doesn’t work well with Unity’s fake null objects as they are not really null. Only a check with the == operator will “fake” that the object is null. So ?., ?[ ], ?? and ??= should not be used on UnityEngine.Object derived types unless you know you’re only dealing with “true” null values. Destroying an object does not turn every reference null but the referenced object will “fake” that it’s null.
Very interesting, thank you for those information !