Unsubscribing using anonymous delegate does not work

Unity Courses Talk
#1

See Memory leak unsubscribing using anonymous delegate.

player.Wallet.TotalCoins.OnValueChanged -= (oldCoins, newCoins) => HandleCoinsChanged(player.OwnerClientId, newCoins); simple does not work, because it is a new delegate every time and it cannot find the original delegate in the delegates list.

The solution would be creating a disposable subscriber and store them in a List/Dictionary and dispose them on despawn.

2 Likes
#2

One way to solve this (I have energy instead of coins):


    public class EnergyChangedSubscription : IDisposable
    {
        private readonly Action<ulong, int> _handleEnergyChanged;
        private readonly TankPlayer _player;

        public EnergyChangedSubscription(
            TankPlayer player,
            Action<ulong, int> handleEnergyChanged)
        {
            _player = player;
            _handleEnergyChanged = handleEnergyChanged;

            _player.EnergyStorage.TotalEnergy.OnValueChanged += HandleTotalEnergyChanged;
        }

        public void Dispose()
        {
            _player.EnergyStorage.TotalEnergy.OnValueChanged -= HandleTotalEnergyChanged;
        }

        private void HandleTotalEnergyChanged(int previousValue, int newValue)
        {
            _handleEnergyChanged(_player.OwnerClientId, newValue);
        }
    }

Subscribing:

// Just in case if we have already subscribed
            if (_energyChangedSubscriptions.TryGetValue(player.OwnerClientId, out var subscription))
            {
                subscription.Dispose();
            }

            _energyChangedSubscriptions[player.OwnerClientId] =
                new EnergyChangedSubscription(player, HandleEnergyChanged);

Unsubscribing:


            if (_energyChangedSubscriptions.TryGetValue(player.OwnerClientId, out var subscription))
            {
                subscription.Dispose();
                _energyChangedSubscriptions.Remove(player.OwnerClientId);
            }
#3

Or even easier, just create a function to handle it and subscribe to/unsubscribe from the function

private void ValueChanged(int oldCoins, int newCoins)
{
    // do what you want to do
}

private void OnEnable()
{
    player.Wallet.TotalCoins.OnValueChanged += ValueChanged;
}

private void OnDisable()
{
    player.Wallet.TotalCoins.OnValueChanged -= ValueChanged;
}
1 Like
#4

This cannot be done in Leaderboard class, since ValueChanged does not know for which player the event was raised. Do you plan to subscribe to OnValueChanged in some other class?

#5

It’s my bad, I was talking about events in general. I haven’t finished this course because I got what I needed from it. Didn’t realise it’s a NetworkVariable.

Personally, I would listen for the change in the Wallet (or EnergyStorage) and then fire a normal event. Then I can bind to the event instead of the NetworkVariable

In CoinWallet.cs

public event EventHandler<ValueChangedEventArgs<int>> CoinValueChanged;

public void TotalCoinsChanged(int oldValue, int newValue)
{
    CoinValueChanged?.Invoke(this, new ValueChangedEventArgs<int>(oldValue, newValue));
}

for this I’d need the ValueChangedEventArgs (I made it generic for use with other events too)

public class ValueChangedEventArgs<TType> : EventArgs
{
    public TType OldValue { get; }
    public TType NewValue {get; }
    public ValueChangedEventArgs(TType oldValue, TType newValue)
    {
        OldValue = oldValue;
        NewValue = newValue;
    }
}

And then, in the leaderboard, I can subscribe and unsubscribe. The sender will hold the relevant CoinWallet and since it’s a NetworkBehaviour I can get the OwnerClientId from it. I think

private void HandlePlayerSpawned(TankPlayer player)
{
    leaderboardEntities.Add(new LeaderboardEntityState
    {
        ClientId = player.OwnerClientId,
        PlayerName = player.PlayerName.Value,
        TeamIndex = player.TeamIndex.Value,
        Coins = 0
    });

    player.Wallet.TotalCoins.CoinValueChanged += OnPlayerCoinsChanged;
}

private void HandlePlayerDespawned(TankPlayer player)
{
    foreach (LeaderboardEntityState entity in leaderboardEntities)
    {
        if (entity.ClientId != player.OwnerClientId) { continue; }

        leaderboardEntities.Remove(entity);
        break;
    }

    player.Wallet.TotalCoins.OnValueChanged -= OnPlayerCoinsChanged;
}

private void OnPlayerCoinsChanged(object sender, ValueChangedEventArgs<int> e)
{
    // cast the sender to CoinWallet (it should be) and return if it's not a CoinWallet
    if (!(sender is CoinWallet wallet)) return;
    // get the client id from the wallet
    var clientId = wallet.OwnerClientId;

    for (int i = 0; i < leaderboardEntities.Count; i++)
    {
        if (leaderboardEntities[i].ClientId != clientId) { continue; }

        leaderboardEntities[i] = new LeaderboardEntityState
        {
            ClientId = leaderboardEntities[i].ClientId,
            PlayerName = leaderboardEntities[i].PlayerName,
            TeamIndex = leaderboardEntities[i].TeamIndex,
            Coins = e.NewValue;
        };
        return;
    }
}

But that’s just what I would do. I’d prefer to not look at/bind to network variables outside of the owning class - no Idea why, just how I feel about it. I’m sure your way will do just fine.

2 Likes

Privacy & Terms