fix: bank deposit behavior #1720

Arufonsu · 2023-02-03T08:24:17Z

Should resolve #1681

Fixes:

Adjusts quantity and maxQuantity for stackable item's slider.
Checks if item is stackable when required so we don't unnecessarily show up the deposit slider on single equipment items.
Context menu bank deposit d/c fix: when destination slot is < 0 (context menu deposit) -> FindsOpenSlot and deposit.
Non-stackable items fix: we weren't checking stackable items properly, which translated into non-stackable items being placed over each other, leading to glitched bank slots.
Single stackable items (ie, an arrow) won't overwrite existing item(s) in slot anymore and they stack along with the rest of the same stackable items.

More fixes along with improvements:

We can now easily fill the bank when depositing stackable items, just drag the slider to the right! the server will make sure to adjust the amount and fill the required slots.
If the bank slot that an item is dropped on (slot is used as "hint") does not stack or is full of stackable items but there is still available slots in the bank, the server makes sure to find a new slot for non stackable items and fills the non-fully-stacked items.
If the bank slot used as "hint" is empty, the server fills the non-fully-stacked items and deposits the rest within the "hint" slot.
If there's no initial "hint" (ie, context menu deposit), then finds an open slot to deposit the rest of the stackable items.
Updates and simplifies the "CanStore" method to properly work along with this commit changes.
Creates the "FindAvailableSpaceForStackableItem" method which finds the amount of available space in the bank for a stackable item.
Added in-game system messages to be sent to players when successfully depositing and withdrawing items from the bank.

Previews

~
(April 15th, 2023)

2023-04-15.01-30-35.mp4

~

2023-04-12.21-09-07.mp4

~

2023-04-12.21-10-50.mp4

~

2023-04-12.20-47-54.mp4

lodicolo · 2023-02-05T19:26:41Z

@Arufonsu Both of the videos look like the broken behavior, where is a video of the correct behavior with this fix?

lodicolo · 2023-02-06T22:03:52Z

From Discord:

If the bank slot does not stack but there is space in the bank still, the move should not fail
the slot that an item is dropped on should be a "hint" not an absolute "this is where it goes"

That should be addressed too in this PR.

Arufonsu · 2023-04-12T02:57:20Z

Updated PR, code and video previews, so far this manages to fix the issues and behaves as expected, discussed and suggested 👍🏽

Arufonsu · 2023-04-15T05:42:02Z

Updated PR (lot of force pushes, won't happen after the first review - unless is required by the maintainer).
Fixed some logic issues i've encountered, also added some nice additions, preview:

2023-04-15.01-30-35.mp4

Hopefully this yields the expected behavior, PR feels ready for review as well 👍🏽

Arufonsu · 2023-08-27T05:16:12Z

self-reminder: restored the ability to deposit multiple non-stackable items

2023-08-27.00-58-01.mp4

PR is ready for review and it's about to be built and published by @intersectbot for the ease of testing ~

Arufonsu · 2023-08-27T18:48:34Z

Introduces the 'FindAvailableBankSpaceForItem' method to calculate the total available space within the bank for specific items. This enhancement streamlines the bank deposit process by automatically determining the appropriate values for the 'quantity' and 'maxQuantity' parameters in the deposit slider, improving user experience and accuracy when depositing items in the bank.

2023-08-27.14-41-01.mp4

Arufonsu · 2023-08-27T19:10:50Z

client enhancement: NoSpace bank system message

Instead of blindly opening the item deposit slider when there's no space left in the bank, we now inform the player "There is no space left in your bank for that item!" before the slider is up with a max quantity of 0 (which is pointless to do).

2023-08-27.15-09-12.mp4

lodicolo

This isn't an exhaustive review but there is a missing thing and a few over-complicated things that I think can be simplified

lodicolo · 2023-08-27T21:50:32Z

Intersect.Server/Entities/BankInterface.cs

+            int spaceLeft = 0;
+            for (int i = 0; i < mMaxSlots; i++)
+            {
+                if (mBank[i]?.ItemId == itemId)


Before the if statement add this as a short-circuit to skip costly searches

if (spaceLeft >= amount) { return amount; }

lodicolo · 2023-08-27T21:51:40Z

Intersect.Server/Entities/BankInterface.cs

-                // Not a stacking item, so can we contain the amount we want to give them?
-                else
-                {
-                    if (FindOpenSlots().Count >= item.Quantity)


This case appears to not be covered in the new code?

This should be covering it when it comes to non stackable items:

Intersect-Engine/Intersect.Server/Entities/BankInterface.cs

Lines 205 to 209 in 0765628

// If this item isn't stackable we just need to find an open slot.

if (!item.Descriptor.Stackable)

{

return FindOpenSlot() != -1;

}

Yeah but the code specifically said >= item.Quantity not 1 -- it makes me think there's a case where something can be not stackable with a quantity > 1...

that would be weird (?) but just to make sure, i can add this check back, although i would have to see how it behaves with this... which tbh feels weird, right now we are just looking away for an available slot whenever it comes to non-stackables, since they aren't supposed to ... stack together in a single a slot (?)

lodicolo · 2023-08-27T21:55:04Z

Intersect.Server/Entities/BankInterface.cs

+            var depositMultipleItems = mPlayer.FindInventoryItemQuantity(inventoryItemItemId) > 1;
+            var maxAmount = depositMultipleItems ? Math.Min(mPlayer.FindInventoryItemQuantity(inventoryItemItemId),
+                    FindAvailableSpaceForItem(inventoryItemItemId, amount)) : 1;
+            amount = Math.Min(amount, maxAmount);


depositMultipleItems should be based on this line instead?

lodicolo · 2023-08-27T21:55:41Z

Intersect.Server/Entities/BankInterface.cs

+            var maxAmount = depositMultipleItems ? Math.Min(mPlayer.FindInventoryItemQuantity(inventoryItemItemId),
+                    FindAvailableSpaceForItem(inventoryItemItemId, amount)) : 1;


mPlayer.FindInventoryItemQuantity(inventoryItemItemId) is repeated in both the condition and the branch, the branch is totally unnecessary here.

This could be changed to

var inventoryQuantity = mPlayer.FindInventoryItemQuantity(inventoryItemItemId); amount = Math.Min(amount, inventoryQuantity); // This is to optimize the next line var availableStorageSpace = FindAvailableSpaceForItem(inventoryItemItemId, amount)); var maxAmount = Math.Min(inventoryQuantity, availableStorageSpace);

lodicolo · 2023-08-27T21:58:02Z

Intersect.Server/Entities/BankInterface.cs

+            if (depositMultipleItems)
+            {
+                if (destSlot < 0 || mBank?[destSlot]?.ItemId != Guid.Empty)
+                {
+                    // Try to find an open slot with the same item type and not at maximum stack size
+                    for (int i = 0; i < mMaxSlots; i++)
+                    {
+                        var bankItem = mBank[i];
+                        if (bankItem.ItemId == Guid.Empty ||
+                            (bankItem.ItemId != Guid.Empty && bankItem.ItemId == inventoryItemItemId &&
+                             amount + bankItem.Quantity < bankItem.Descriptor.MaxBankStack))
                        {
-                            if (itemBase.IsStackable)
-                            {
-                                PutItem(new Item(itemBase.Id, amount), destSlot, sendUpdate);
-                                mPlayer.TryTakeItem(itemBase.Id, amount, ItemHandling.Normal, sendUpdate);
-
-                                if (mGuild != null)
-                                {
-                                    DbInterface.Pool.QueueWorkItem(mGuild.Save);
-                                }
-
-                                return true;
-                            }
-                            else
-                            {
-                                PutItem(mPlayer.Items[slot], destSlot, sendUpdate);
-
-                                mPlayer.Items[slot].Set(Item.None);
-                                mPlayer.EquipmentProcessItemLoss(slot);
-
-                                if (sendUpdate)
-                                {
-                                    PacketSender.SendInventoryItemUpdate(mPlayer, slot);
-                                }
-
-                                if (mGuild != null)
-                                {
-                                    DbInterface.Pool.QueueWorkItem(mGuild.Save);
-                                }
-
-                                return true;
-                            }
+                            destSlot = i;
+                            break;
                        }
-                        else
-                        {
-                            PacketSender.SendChatMsg(mPlayer, Strings.Banks.banknospace, ChatMessageType.Bank, CustomColors.Alerts.Error);
-                        }  
                    }
                }
-                else
+            }
+            else
+            {
+                if (destSlot < 0 || mBank?[destSlot]?.ItemId != Guid.Empty)
                {
-                    PacketSender.SendChatMsg(mPlayer, Strings.Banks.depositinvalid, ChatMessageType.Bank, CustomColors.Alerts.Error);
+                    destSlot = FindOpenSlot();
+                }
+
+                if (destSlot < 0)
+                {
+                    PacketSender.SendChatMsg(mPlayer, Strings.Banks.banknospace, ChatMessageType.Bank,
+                        CustomColors.Alerts.Error);
+                    return false;
                }
            }


I'm wondering if generic and simpler logic is better than introducing an "if 1" condition

lodicolo · 2023-08-27T21:58:14Z

Intersect.Server/Entities/BankInterface.cs

+                    FindAvailableSpaceForItem(inventoryItemItemId, amount)) : 1;
+            amount = Math.Min(amount, maxAmount);
+
+            if (amount <= 0)


maxAmount < 1 after the suggested changes (?)

No, it would still be amount < 1 because you would not be getting rid of the last amount = Math.Min(amount, maxAmount);

* Fixes: - Adjusts quantity and maxQuantity for stackable item's slider. - Checks if item is stackable when required so we don't unnecessarily show up the deposit slider on single equipment items. - Context menu bank deposit d/c fix: when destination slot is < 0 (context menu deposit) -> FindsOpenSlot and deposit. - Non-stackable items fix: we weren't checking stackable items properly, which translated into non-stackable items being placed over each other, leading to glitched bank slots. - Single stackable items (ie, an arrow) won't overwrite existing item(s) in slot anymore and they stack along with the rest of the same stackable items. * More fixes along with improvements: - We can now easily fill the bank when depositing stackable items, just drag the slider to the right! the server will make sure to adjust the amount and fill the required slots. - If the bank slot that an item is dropped on (slot is used as "hint") does not stack or is full of stackable items but there is still available slots in the bank, the server makes sure to find a new slot for non stackable items and fills the non-fully-stacked items. - If the bank slot used as "hint" is empty, the server fills the non-fully-stacked items and deposits the rest within the "hint" slot. - If there's no initial "hint" (ie, context menu deposit), then finds an open slot to deposit the rest of the stackable items. - Updates and simplifies the "CanStore" method to properly work along with this commit changes. - Creates the "FindAvailableSpaceForStackableItem" method which finds the amount of available space in the bank for a stackable item. - Added in-game system messages to be sent to players when successfully depositing and withdrawing items from the bank.

changes to CanStoreItem: - restored as public method. - properly checks for partial stack filling. - the for loop for stackable items has been removed from this boolean method as it was unnecessary. Checking for the destination slots is already performed outside of this method, making it redundant to have the loop here.

- Introduces the 'FindAvailableBankSpaceForItem' method to calculate the total available space within the bank for specific items. This enhancement streamlines the bank deposit process by automatically determining the appropriate values for the 'quantity' and 'maxQuantity' parameters in the deposit slider, improving user experience and accuracy when depositing items in the bank.

- Instead of blindly opening the item deposit slider when there's no space left in the bank, we now inform the player "There is no space left in your bank for that item!" before the slider is up with a max quantity of 0 (which is pointless to do).

( we don't really need to *Get* this value on each loop when calculating bank space ! )

- Properly checks default values - We don't need to get the maxBankStack on each calculation loop !

Arufonsu requested review from a team February 3, 2023 08:24

Arufonsu marked this pull request as draft February 3, 2023 16:57

Arufonsu marked this pull request as ready for review February 3, 2023 17:22

Arufonsu marked this pull request as draft February 8, 2023 00:41

Arufonsu added the help wanted Extra attention is needed label Mar 29, 2023

Arufonsu force-pushed the aru/fix-TryDepositItem branch from ccc385b to 53d4df3 Compare April 12, 2023 02:51

Arufonsu marked this pull request as ready for review April 12, 2023 02:52

Arufonsu force-pushed the aru/fix-TryDepositItem branch from 53d4df3 to b8782af Compare April 12, 2023 03:12

Arufonsu marked this pull request as draft April 12, 2023 19:12

Arufonsu force-pushed the aru/fix-TryDepositItem branch from b8782af to 21d88be Compare April 13, 2023 00:46

Arufonsu marked this pull request as ready for review April 13, 2023 00:48

Arufonsu marked this pull request as draft April 13, 2023 00:57

Arufonsu force-pushed the aru/fix-TryDepositItem branch from 21d88be to 267ef4a Compare April 13, 2023 01:08

Arufonsu marked this pull request as ready for review April 13, 2023 01:08

Arufonsu mentioned this pull request Apr 13, 2023

bug: single stackable item from inventory to the bank #1681

Closed

1 task

Arufonsu marked this pull request as draft April 14, 2023 02:39

Arufonsu force-pushed the aru/fix-TryDepositItem branch 4 times, most recently from 74b76c2 to f18b668 Compare April 15, 2023 05:27

Arufonsu changed the title ~~fix: TryDepositItem behavior~~ fix: bank deposit behavior Apr 15, 2023

Arufonsu added the enhancement Minor feature addition or quality of life change label Apr 15, 2023

Arufonsu marked this pull request as ready for review April 15, 2023 05:42

Arufonsu marked this pull request as draft April 16, 2023 03:28

Arufonsu force-pushed the aru/fix-TryDepositItem branch from 948ff6b to e5e8c88 Compare April 16, 2023 05:09

Arufonsu requested review from AlexVild, jcsnider, WeylonSantana and a team August 27, 2023 05:12

Arufonsu marked this pull request as ready for review August 27, 2023 05:12

AscensionGameDev deleted a comment from intersectbot Aug 27, 2023

Arufonsu added the bug Something isn't working label Aug 27, 2023

AscensionGameDev deleted a comment from intersectbot Aug 27, 2023

lodicolo requested changes Aug 27, 2023

View reviewed changes

Arufonsu requested a review from lodicolo August 27, 2023 23:59

Arufonsu force-pushed the aru/fix-TryDepositItem branch from e6db3c8 to 0f879b4 Compare October 2, 2023 23:38

Arufonsu added 10 commits October 12, 2023 21:17

self-reminder: ability to deposit multiple non-stackable items

a8fd9ea

review: short-circuit to skip costly searches

e68455d

chore: FindAvailableBankSpaceForItem

329b0ad

review: TryDepositItem

2ff8c40

chore: maxBankStack as variable within FindAvailableBankSpaceForItem

3a292ad

( we don't really need to *Get* this value on each loop when calculating bank space ! )

chore: bankInterface - FindAvailableSpaceForItem

748bd38

- Properly checks default values - We don't need to get the maxBankStack on each calculation loop !

Arufonsu force-pushed the aru/fix-TryDepositItem branch from 0f879b4 to 748bd38 Compare October 13, 2023 00:17

Arufonsu self-assigned this Oct 13, 2023

Arufonsu closed this Oct 21, 2023

Arufonsu mentioned this pull request Oct 21, 2023

refactoring: bank #1992

Merged

Arufonsu deleted the aru/fix-TryDepositItem branch March 10, 2024 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bank deposit behavior #1720

fix: bank deposit behavior #1720

Arufonsu commented Feb 3, 2023 •

edited

Loading

lodicolo commented Feb 5, 2023

lodicolo commented Feb 6, 2023

Arufonsu commented Apr 12, 2023

Arufonsu commented Apr 15, 2023 •

edited

Loading

Arufonsu commented Aug 27, 2023 •

edited

Loading

Arufonsu commented Aug 27, 2023

Arufonsu commented Aug 27, 2023

lodicolo left a comment

lodicolo Aug 27, 2023

lodicolo Aug 27, 2023

Arufonsu Aug 27, 2023

lodicolo Aug 27, 2023

Arufonsu Aug 27, 2023 •

edited

Loading

lodicolo Aug 27, 2023

lodicolo Aug 27, 2023

lodicolo Aug 27, 2023

lodicolo Aug 27, 2023

lodicolo Aug 27, 2023

Arufonsu Aug 27, 2023

lodicolo Aug 27, 2023

	// If this item isn't stackable we just need to find an open slot.
	if (!item.Descriptor.Stackable)
	{
	return FindOpenSlot() != -1;
	}

		var maxAmount = depositMultipleItems ? Math.Min(mPlayer.FindInventoryItemQuantity(inventoryItemItemId),
		FindAvailableSpaceForItem(inventoryItemItemId, amount)) : 1;

fix: bank deposit behavior #1720

fix: bank deposit behavior #1720

Conversation

Arufonsu commented Feb 3, 2023 • edited Loading

Should resolve #1681

Fixes:

More fixes along with improvements:

Previews

lodicolo commented Feb 5, 2023

lodicolo commented Feb 6, 2023

Arufonsu commented Apr 12, 2023

Arufonsu commented Apr 15, 2023 • edited Loading

Arufonsu commented Aug 27, 2023 • edited Loading

Arufonsu commented Aug 27, 2023

Arufonsu commented Aug 27, 2023

lodicolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arufonsu Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arufonsu commented Feb 3, 2023 •

edited

Loading

Arufonsu commented Apr 15, 2023 •

edited

Loading

Arufonsu commented Aug 27, 2023 •

edited

Loading

Arufonsu Aug 27, 2023 •

edited

Loading