Merge pull request #837 from nolt/fix-money-drop-lock

Release a money drop again when nobody could take it

(cherry picked from commit 83e942e4c778a14eeade8433632a6180ef286e3f)

src/GameLogic/DroppedMoney.cs

@@ -80,51 +80,16 @@ public sealed class DroppedMoney : AsyncDisposable, ILocateable
             this._availableToPick = false;
         }
 
-        if (player.Party is { } party)
+        if (!this.TryGiveMoneyTo(player))
         {
-            var partyMembers = party.PartyList
-                .OfType<Player>()
-                .Where(p => p.CurrentMap == player.CurrentMap && !p.IsAtSafezone() && p.Attributes is { })
-                .ToList();
-
-            if (partyMembers.Count > 0)
+            // Nobody got the money, so the drop is released again. Keeping it claimed would leave it
+            // lying on the map, unpickable for everyone until it expires - and then lost.
+            using (await this._pickupLock.LockAsync())
             {
-                var share = (int)(this.Amount / partyMembers.Count);
-                foreach (var member in partyMembers)
-                {
-                    member.TryAddMoney((int)(share * member.Attributes![Stats.MoneyAmountRate]));
-                }
+                this._availableToPick = true;
             }
-        }
-        else
-        {
-            var clampMoneyOnPickup = player.GameContext?.Configuration?.ClampMoneyOnPickup ?? false;
-            if (clampMoneyOnPickup)
-            {
-                var maxMoney = player.GameContext?.Configuration?.MaximumInventoryMoney ?? int.MaxValue;
-                var currentMoney = player.Money;
-                var amountToAdd = (int)Math.Min(this.Amount, (uint)Math.Max(0, maxMoney - currentMoney));
 
-                if (amountToAdd <= 0)
-                {
-                    player.Logger.LogDebug("Player is at maximum money limit, Player {0}, Money {1}", player, this);
-                    return false;
-                }
-
-                if (!player.TryAddMoney(amountToAdd))
-                {
-                    player.Logger.LogDebug("Money could not be added to the inventory, Player {0}, Money {1}", player, this);
-                    return false;
-                }
-            }
-            else
-            {
-                if (!player.TryAddMoney((int)this.Amount))
-                {
-                    player.Logger.LogDebug("Money could not be added to the inventory, Player {0}, Money {1}", player, this);
-                    return false;
-                }
-            }
+            return false;
         }
 
         await this.DisposeAsync().ConfigureAwait(false);
@@ -158,6 +123,64 @@ public sealed class DroppedMoney : AsyncDisposable, ILocateable
         await base.DisposeAsyncCore().ConfigureAwait(false);
     }
 
+    /// <summary>
+    /// Tries to hand the money over to the player, or to its party. Returns <c>false</c> if it could not be
+    /// given to anyone, e.g. because the receiver is already at the maximum inventory money.
+    /// </summary>
+    /// <param name="player">The player which picks the money up.</param>
+    /// <returns><c>True</c>, if at least one player received money; Otherwise, <c>false</c>.</returns>
+    private bool TryGiveMoneyTo(Player player)
+    {
+        if (player.Party is { } party)
+        {
+            var partyMembers = party.PartyList
+                .OfType<Player>()
+                .Where(p => p.CurrentMap == player.CurrentMap && !p.IsAtSafezone() && p.Attributes is { })
+                .ToList();
+
+            if (partyMembers.Count == 0)
+            {
+                player.Logger.LogDebug("No party member could receive the money, Player {0}, Money {1}", player, this);
+                return false;
+            }
+
+            var share = (int)(this.Amount / partyMembers.Count);
+            var received = false;
+            foreach (var member in partyMembers)
+            {
+                received |= member.TryAddMoney((int)(share * member.Attributes![Stats.MoneyAmountRate]));
+            }
+
+            if (!received)
+            {
+                player.Logger.LogDebug("No party member could take the money, Player {0}, Money {1}", player, this);
+            }
+
+            return received;
+        }
+
+        var amountToAdd = (int)this.Amount;
+        if (player.GameContext?.Configuration?.ClampMoneyOnPickup ?? false)
+        {
+            var maxMoney = player.GameContext?.Configuration?.MaximumInventoryMoney ?? int.MaxValue;
+            amountToAdd = (int)Math.Min(this.Amount, (uint)Math.Max(0, maxMoney - player.Money));
+
+            if (amountToAdd <= 0)
+            {
+                player.Logger.LogDebug("Player is at maximum money limit, Player {0}, Money {1}", player, this);
+                return false;
+            }
+        }
+
+        if (!player.TryAddMoney(amountToAdd))
+        {
+            player.Logger.LogDebug("Money could not be added to the inventory, Player {0}, Money {1}", player, this);
+            return false;
+        }
+
+        return true;
+    }
+
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "VSTHRD100:Avoid async void methods", Justification = "Catching all Exceptions.")]
     private async void OnTimerTimeout(object? state)
     {

tests/MUnique.OpenMU.Tests/DroppedMoneyTest.cs

@@ -0,0 +1,65 @@
+// <copyright file="DroppedMoneyTest.cs" company="MUnique">
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+// </copyright>
+
+namespace MUnique.OpenMU.Tests;
+
+using MUnique.OpenMU.GameLogic;
+using MUnique.OpenMU.Pathfinding;
+using NUnit.Framework;
+
+/// <summary>
+/// Tests for the <see cref="DroppedMoney"/>.
+/// </summary>
+[TestFixture]
+public class DroppedMoneyTest
+{
+    private const uint DroppedAmount = 1000;
+
+    /// <summary>
+    /// Tests that a failed pick up doesn't consume the drop, so it's still available for the next player.
+    /// A player at the maximum inventory money can't take it, and used to claim it nevertheless - which left
+    /// it lying on the map, unpickable for everyone until it expired.
+    /// </summary>
+    [Test]
+    public async Task FailedPickUpKeepsMoneyAvailableAsync()
+    {
+        var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false);
+        player.GameContext.Configuration.MaximumInventoryMoney = int.MaxValue;
+        var maximumMoney = player.GameContext.Configuration.MaximumInventoryMoney;
+        player.Money = maximumMoney;
+
+        var money = new DroppedMoney(DroppedAmount, new Point(100, 100), player.CurrentMap!);
+
+        Assert.That(await money.TryPickUpByAsync(player).ConfigureAwait(false), Is.False);
+        Assert.That(player.Money, Is.EqualTo(maximumMoney));
+
+        var otherPlayer = await PlayerTestHelper.CreatePlayerAsync(player.GameContext).ConfigureAwait(false);
+        otherPlayer.Money = 0;
+
+        Assert.That(await money.TryPickUpByAsync(otherPlayer).ConfigureAwait(false), Is.True);
+        Assert.That(otherPlayer.Money, Is.EqualTo((int)DroppedAmount));
+    }
+
+    /// <summary>
+    /// Tests that a successful pick up still consumes the drop, so it can't be picked up twice.
+    /// </summary>
+    [Test]
+    public async Task SuccessfulPickUpConsumesMoneyAsync()
+    {
+        var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false);
+        player.GameContext.Configuration.MaximumInventoryMoney = int.MaxValue;
+        player.Money = 0;
+
+        var money = new DroppedMoney(DroppedAmount, new Point(100, 100), player.CurrentMap!);
+
+        Assert.That(await money.TryPickUpByAsync(player).ConfigureAwait(false), Is.True);
+        Assert.That(player.Money, Is.EqualTo((int)DroppedAmount));
+
+        var otherPlayer = await PlayerTestHelper.CreatePlayerAsync(player.GameContext).ConfigureAwait(false);
+        otherPlayer.Money = 0;
+
+        Assert.That(await money.TryPickUpByAsync(otherPlayer).ConfigureAwait(false), Is.False);
+        Assert.That(otherPlayer.Money, Is.EqualTo(0));
+    }
+}