Fix ConsumeScroll

Fixes #5617

Source/inv.cpp

@@ -1938,28 +1938,28 @@ void ConsumeScroll(Player &player)
 {
 	const spell_id spellId = player.executedSpell.spellId;
 
-	// Try to remove the scroll from selected inventory slot
-	int itemSlot = player.executedSpell.spellFrom;
-	int itemIndex = 0;
-	Item *item;
-	if (itemSlot <= INVITEM_INV_LAST) {
-		itemIndex = itemSlot - INVITEM_INV_FIRST;
-		item = &player.InvList[itemIndex];
-	} else {
-		itemIndex = itemSlot - INVITEM_BELT_FIRST;
-		item = &player.SpdList[itemIndex];
-	}
-
 	const auto isCurrentSpell = [spellId](const Item &item) {
 		return item.isScrollOf(spellId) || item.isRuneOf(spellId);
 	};
 
-	if (!item->isEmpty() && isCurrentSpell(*item)) {
-		if (itemSlot <= INVITEM_INV_LAST)
-			player.RemoveInvItem(static_cast<int>(itemIndex));
-		else
+	// Try to remove the scroll from selected inventory slot
+	const int8_t itemSlot = player.executedSpell.spellFrom;
+	if (itemSlot >= INVITEM_INV_FIRST && itemSlot <= INVITEM_INV_LAST) {
+		const int itemIndex = itemSlot - INVITEM_INV_FIRST;
+		const Item *item = &player.InvList[itemIndex];
+		if (!item->isEmpty() && isCurrentSpell(*item)) {
+			player.RemoveInvItem(itemIndex);
+			return;
+		}
+	} else if (itemSlot >= INVITEM_BELT_FIRST && itemSlot <= INVITEM_BELT_LAST) {
+		const int itemIndex = itemSlot - INVITEM_BELT_FIRST;
+		const Item *item = &player.SpdList[itemIndex];
+		if (!item->isEmpty() && isCurrentSpell(*item)) {
 			player.RemoveSpdBarItem(itemIndex);
-		return;
+			return;
+		}
+	} else if (itemSlot != 0) {
+		app_fatal(StrCat("ConsumeScroll: Invalid item index ", itemSlot));
 	}
 
 	// Didn't find it at the selected slot, take the first one we find

test/inv_test.cpp

@@ -206,13 +206,32 @@ TEST_F(InvTest, RemoveSpdBarItem)
 }
 
 // Test removing a scroll from the inventory
-TEST_F(InvTest, RemoveCurrentSpellScroll_inventory)
+TEST_F(InvTest, RemoveCurrentSpellScrollFromInventory)
 {
 	clear_inventory();
 
 	// Put a firebolt scroll into the inventory
 	MyPlayer->_pNumInv = 1;
 	MyPlayer->executedSpell.spellId = static_cast<spell_id>(SPL_FIREBOLT);
+	MyPlayer->executedSpell.spellFrom = INVITEM_INV_FIRST;
+	MyPlayer->InvList[0]._itype = ItemType::Misc;
+	MyPlayer->InvList[0]._iMiscId = IMISC_SCROLL;
+	MyPlayer->InvList[0]._iSpell = SPL_FIREBOLT;
+
+	ConsumeScroll(*MyPlayer);
+	EXPECT_EQ(MyPlayer->InvGrid[0], 0);
+	EXPECT_EQ(MyPlayer->_pNumInv, 0);
+}
+
+// Test removing the first matching scroll from inventory
+TEST_F(InvTest, RemoveCurrentSpellScrollFromInventoryFirstMatch)
+{
+	clear_inventory();
+
+	// Put a firebolt scroll into the inventory
+	MyPlayer->_pNumInv = 1;
+	MyPlayer->executedSpell.spellId = static_cast<spell_id>(SPL_FIREBOLT);
+	MyPlayer->executedSpell.spellFrom = 0; // any matching scroll
 	MyPlayer->InvList[0]._itype = ItemType::Misc;
 	MyPlayer->InvList[0]._iMiscId = IMISC_SCROLL;
 	MyPlayer->InvList[0]._iSpell = SPL_FIREBOLT;
@@ -233,6 +252,27 @@ TEST_F(InvTest, RemoveCurrentSpellScroll_belt)
 	}
 	// Put a firebolt scroll into the belt
 	MyPlayer->executedSpell.spellId = static_cast<spell_id>(SPL_FIREBOLT);
+	MyPlayer->executedSpell.spellFrom = INVITEM_BELT_FIRST + 3;
+	MyPlayer->SpdList[3]._itype = ItemType::Misc;
+	MyPlayer->SpdList[3]._iMiscId = IMISC_SCROLL;
+	MyPlayer->SpdList[3]._iSpell = SPL_FIREBOLT;
+
+	ConsumeScroll(*MyPlayer);
+	EXPECT_TRUE(MyPlayer->SpdList[3].isEmpty());
+}
+
+// Test removing the first matching scroll from the belt
+TEST_F(InvTest, RemoveCurrentSpellScrollFirstMatchFromBelt)
+{
+	SNetInitializeProvider(SELCONN_LOOPBACK, nullptr);
+
+	// Clear the belt
+	for (int i = 0; i < MaxBeltItems; i++) {
+		MyPlayer->SpdList[i].clear();
+	}
+	// Put a firebolt scroll into the belt
+	MyPlayer->executedSpell.spellId = static_cast<spell_id>(SPL_FIREBOLT);
+	MyPlayer->executedSpell.spellFrom = 0; // any matching scroll
 	MyPlayer->SpdList[3]._itype = ItemType::Misc;
 	MyPlayer->SpdList[3]._iMiscId = IMISC_SCROLL;
 	MyPlayer->SpdList[3]._iSpell = SPL_FIREBOLT;