Raised the limit of max items per stack to 999

[LOuroboros]

Description

I was requested to PR this, and here it is 👀
By default, these games are very compatible with amounts of item stacks bigger than 255 (the size of the u8 data type).
Why? Because GF allows the Players to get up to 999 berries by default, thus the save space for that has already been allocated.
Why did they not just let them collect 999 of any other item outside of Key Items though? Who knows. #JustGameFreakThings.

Now, what Game Freak didn't bother to account for was the Battle Pyramid bag, that exists separate from the common bag.
Because of that, in this PR I also added the extra code changes needed to make it suitable for 999 item stacks.
For this purpose I added a new equivalent to MAX_BAG_ITEM_CAPACITY called MAX_PYRAMID_BAG_ITEM_CAPACITY.
When MAX_PYRAMID_BAG_ITEM_CAPACITY is set to a number higher than 255, extra SaveBlock2 will be used to accomodate for the new max quantity amount.
With the default 10 Battle Pyramid bag slots, raising the max amount of items per stack in the Battle Pyramid to 999 takes 20 bytes of space.

I also made a few extra changes that complement the feature, namely, I removed BERRY_CAPACITY_DIGITS, BAG_ITEM_CAPACITY_DIGITS and also MAX_BERRY_CAPACITY.
Why? Because there wasn't a need to keep them around.
Since this feature is not toggleable and we're making the default limit of max items per stack 999 across the board, there's just no reason to keep constants like BERRY_CAPACITY_DIGITS o BAG_ITEM_CAPACITY_DIGITS, in my opinion.
I highly doubt someone would want to nerf very specific pockets of their bag, therefore, MAX_ITEM_DIGITS now controls the number of digits for item stacks across the board.
On that note, I updated the value of MAX_ITEM_DIGITS for InGame amount printing reasons. Now its value will be be either 2 or 3 digits depending the value given to MAX_BAG_ITEM_CAPACITY.

I think(tm) that I tested everything; additem works, removeitem works, checkitem works and the Battle Pyramid Bag works. The debug menu works too.

Still, being me, I suggest to perform extra tests to double check things.

Discord contact info

lunos4026

[Jaizu]

What about old saves that have stacks of 99 items?

[LOuroboros]

What about old saves that have stacks of 99 items?

They remain unaffected. As I said, save space for stacks of 999 items are already allocated because of berries.

Old savefiles may break specifically when raising the value of MAX_PYRAMID_BAG_ITEM_CAPACITY above 255, I mentioned it above and I also mention it in a comment at include/constants/items.h where the constant is defined.

[Jaizu]

So if someone has two stacks of 99 items they will load a stack of 198 items? Is this tested?

[LOuroboros]

So if someone has two stacks of 99 items they will load a stack of 198 items? Is this tested?

In existing saves separate stacks probably remain separate, I imagine they won't fuse automagically.

But if you add 2 fresh stacks of 99 of an item, the result is effectively 1 stack of 198.

Debug_EventScript_Script_1::
	additem ITEM_POKE_BALL, 99
	additem ITEM_POTION, 99
	additem ITEM_POKE_BALL, 99
	additem ITEM_POTION, 99
	end

EDIT: Actually, maybe they are fused by UpdatePocketItemList when opening the bag. I guess that's worth testing.

[AsparagusEduardo]

EDIT: Actually, maybe they are fused by UpdatePocketItemList when opening the bag. I guess that's worth testing.

That's handled specifically in the CompactItemsInBagPocket function, which UpdatePocketItemList calls. We ourselves also call it in the ball battle interface. I was wrong in this.
There shouldn't be a risk in increasing these values, only when decreasing them, so I wouldn't even suggest adding a config for this.
Review in process.

[AsparagusEduardo]

I was wrong, I tested and it does not compact them when opening the bag. In fact, It might be that CompactItemsInBagPocket is a misnomer, since it doesn't really compact them, but pushes empty stacks to the bottom.
I also did some tests related to selling/buyinh items and there're a little bit awkward with existing stacks.

[LOuroboros]

A lot of the functions I edited have pointless local variables for item digits just like the debug function I just updated.

Things like:

lunos:/mnt/c/Users/Lunos/Home/rhh$ diff
diff --git a/src/item_menu.c b/src/item_menu.c
index ef3c75ee6..55ec0acca 100755
--- a/src/item_menu.c
+++ b/src/item_menu.c
@@ -1210,8 +1210,7 @@ static void AddItemQuantityWindow(u8 windowType)

 static void PrintItemQuantity(u8 windowId, s16 quantity)
 {
-    u8 numDigits = MAX_ITEM_DIGITS;
-    ConvertIntToDecimalStringN(gStringVar1, quantity, STR_CONV_MODE_LEADING_ZEROS, numDigits);
+    ConvertIntToDecimalStringN(gStringVar1, quantity, STR_CONV_MODE_LEADING_ZEROS, MAX_ITEM_DIGITS);
     StringExpandPlaceholders(gStringVar4, gText_xVar1);
     AddTextPrinterParameterized(windowId, FONT_NORMAL, gStringVar4, GetStringCenterAlignXOffset(FONT_NORMAL, gStringVar4, 0x28), 2, 0, 0);
 }
@@ -1219,8 +1218,7 @@ static void PrintItemQuantity(u8 windowId, s16 quantity)
 // Prints the quantity of items to be sold and the amount that would be earned
 static void PrintItemSoldAmount(int windowId, int numSold, int moneyEarned)
 {
-    u8 numDigits = MAX_ITEM_DIGITS;
-    ConvertIntToDecimalStringN(gStringVar1, numSold, STR_CONV_MODE_LEADING_ZEROS, numDigits);
+    ConvertIntToDecimalStringN(gStringVar1, numSold, STR_CONV_MODE_LEADING_ZEROS, MAX_ITEM_DIGITS);
     StringExpandPlaceholders(gStringVar4, gText_xVar1);
     AddTextPrinterParameterized(windowId, FONT_NORMAL, gStringVar4, 0, 1, TEXT_SKIP_DRAW, 0);
     PrintMoneyAmount(windowId, 38, 1, moneyEarned, 0);
lunos:/mnt/c/Users/Lunos/Home/rhh$

I kept them because I didn't want to take any liberties. Idk what should be done about those.

[AsparagusEduardo]

I kept them because I didn't want to take any liberties. Idk what should be done about those.

We might as well remove them to keep the code clean :)

[AsparagusEduardo]

Looks good. The last thing I'd like is for this is to actually compact the item stacks such that existing stacks get combined into a single one upon opening the bag.

[LOuroboros]

I don't have any idea how to do that.

I'll try to look into it.

[AsparagusEduardo]

I'll try to look into it.

Oh, it just occured to me that #3938 might solve this 👀

[LOuroboros]

At a quick glance I'm not seeing any changes to the functions that initialize the bag's interface and contents, so I kinda doubt it. but maybe it does.

[AsparagusEduardo]

I did one more test and it seems that any awkwardness regarding multiple stacks can be fixed by stored the items into the player PC and back out, so no need to worry about that.

[LOuroboros]

Thank goodness, because I really didn't know how to go about doing this 😂

I tried something stupid, but it didn't work (shops were creating multiple full stacks of a same item in the Player's bag, that's how fucked up it was), and I didn't know what to try next.

diff --git a/src/item.c b/src/item.c
index dbbce1581..ea86d2091 100644
--- a/src/item.c
+++ b/src/item.c
@@ -594,7 +594,16 @@ void CompactItemsInBagPocket(struct BagPocket *bagPocket)
         for (j = i + 1; j < bagPocket->capacity; j++)
         {
             if (GetBagItemQuantity(&bagPocket->itemSlots[i].quantity) == 0)
+            {
                 SwapItemSlots(&bagPocket->itemSlots[i], &bagPocket->itemSlots[j]);
+            }
+            if (bagPocket->itemSlots[i].itemId == bagPocket->itemSlots[j].itemId)
+            {
+                u16 itemId = bagPocket->itemSlots[j].itemId;
+                u16 itemQty = bagPocket->itemSlots[j].quantity;
+                RemoveBagItem(itemId, itemQty);
+                AddBagItem(itemId, itemQty);
+            }
         }
     }
 }

The logic was something aong the lines of "if the Player has 2 stacks of the same item, then removing the items from that stack and giving them back to the Player would probably put them in the first stack", but yeah, it doesn't work like that,.