Clean up spell book rendering

CMakeLists.txt

@@ -156,7 +156,7 @@ else()
 endif()
 
 # By default, devilutionx.mpq is built only if smpq is installed and MPQ support is enabled.
-if(SUPPORTS_MPQ)
+if(SUPPORTS_MPQ AND NOT UNPACKED_MPQS)
   if(NOT DEFINED BUILD_ASSETS_MPQ AND NOT SRC_DIST)
     find_program(SMPQ smpq)
   elseif(BUILD_ASSETS_MPQ)

Source/missiles.cpp

@@ -779,36 +779,33 @@ bool IsMissileBlockedByTile(Point tile)
 	return object != nullptr && !object->_oMissFlag;
 }
 
-void GetDamageAmt(SpellID i, int *mind, int *maxd)
+DamageRange GetDamageAmt(SpellID spell, int spellLevel)
 {
 	assert(MyPlayer != nullptr);
-	assert(i >= SpellID::FIRST && i <= SpellID::LAST);
+	assert(spell >= SpellID::FIRST && spell <= SpellID::LAST);
 
 	Player &myPlayer = *MyPlayer;
 
-	const int sl = myPlayer.GetSpellLevel(i);
-
-	switch (i) {
-	case SpellID::Firebolt:
-		*mind = (myPlayer._pMagic / 8) + sl + 1;
-		*maxd = *mind + 9;
-		break;
+	switch (spell) {
+	case SpellID::Firebolt: {
+		const int min = (myPlayer._pMagic / 8) + spellLevel + 1;
+		return { min, min + 9 };
+	}
 	case SpellID::Healing:
 	case SpellID::HealOther:
 		/// BUGFIX: healing calculation is unused
-		*mind = AddClassHealingBonus(myPlayer._pLevel + sl + 1, myPlayer._pClass) - 1;
-		*maxd = AddClassHealingBonus((4 * myPlayer._pLevel) + (6 * sl) + 10, myPlayer._pClass) - 1;
-		break;
+		return {
+			AddClassHealingBonus(myPlayer._pLevel + spellLevel + 1, myPlayer._pClass) - 1,
+			AddClassHealingBonus((4 * myPlayer._pLevel) + (6 * spellLevel) + 10, myPlayer._pClass) - 1
+		};
 	case SpellID::RuneOfLight:
 	case SpellID::Lightning:
-		*mind = 2;
-		*maxd = 2 + myPlayer._pLevel;
-		break;
-	case SpellID::Flash:
-		*mind = ScaleSpellEffect(myPlayer._pLevel, sl);
-		*mind += *mind / 2;
-		*maxd = *mind * 2;
-		break;
+		return { 2, 2 + myPlayer._pLevel };
+	case SpellID::Flash: {
+		int min = ScaleSpellEffect(myPlayer._pLevel, spellLevel);
+		min += min / 2;
+		return { min, min * 2 };
+	};
 	case SpellID::Identify:
 	case SpellID::TownPortal:
 	case SpellID::StoneCurse:
@@ -832,74 +829,67 @@ void GetDamageAmt(SpellID i, int *mind, int *maxd)
 	case SpellID::Berserk:
 	case SpellID::Search:
 	case SpellID::RuneOfStone:
-		*mind = -1;
-		*maxd = -1;
-		break;
+		return { -1, -1 };
 	case SpellID::FireWall:
 	case SpellID::LightningWall:
-	case SpellID::RingOfFire:
-		*mind = 2 * myPlayer._pLevel + 4;
-		*maxd = *mind + 36;
-		break;
+	case SpellID::RingOfFire: {
+		const int min = 2 * myPlayer._pLevel + 4;
+		return { min, min + 36 };
+	}
 	case SpellID::Fireball:
 	case SpellID::RuneOfFire: {
-		int base = (2 * myPlayer._pLevel) + 4;
-		*mind = ScaleSpellEffect(base, sl);
-		*maxd = ScaleSpellEffect(base + 36, sl);
+		const int base = (2 * myPlayer._pLevel) + 4;
+		return {
+			ScaleSpellEffect(base, spellLevel),
+			ScaleSpellEffect(base + 36, spellLevel)
+		};
 	} break;
 	case SpellID::Guardian: {
-		int base = (myPlayer._pLevel / 2) + 1;
-		*mind = ScaleSpellEffect(base, sl);
-		*maxd = ScaleSpellEffect(base + 9, sl);
+		const int base = (myPlayer._pLevel / 2) + 1;
+		return {
+			ScaleSpellEffect(base, spellLevel),
+			ScaleSpellEffect(base + 9, spellLevel)
+		};
 	} break;
 	case SpellID::ChainLightning:
-		*mind = 4;
-		*maxd = 4 + (2 * myPlayer._pLevel);
-		break;
-	case SpellID::FlameWave:
-		*mind = 6 * (myPlayer._pLevel + 1);
-		*maxd = *mind + 54;
-		break;
+		return { 4, 4 + (2 * myPlayer._pLevel) };
+	case SpellID::FlameWave: {
+		const int min = 6 * (myPlayer._pLevel + 1);
+		return { min, min + 54 };
+	}
 	case SpellID::Nova:
 	case SpellID::Immolation:
 	case SpellID::RuneOfImmolation:
 	case SpellID::RuneOfNova:
-		*mind = ScaleSpellEffect((myPlayer._pLevel + 5) / 2, sl) * 5;
-		*maxd = ScaleSpellEffect((myPlayer._pLevel + 30) / 2, sl) * 5;
-		break;
-	case SpellID::Inferno:
-		*mind = 3;
-		*maxd = myPlayer._pLevel + 4;
-		*maxd += *maxd / 2;
-		break;
+		return {
+			ScaleSpellEffect((myPlayer._pLevel + 5) / 2, spellLevel) * 5,
+			ScaleSpellEffect((myPlayer._pLevel + 30) / 2, spellLevel) * 5
+		};
+	case SpellID::Inferno: {
+		int max = myPlayer._pLevel + 4;
+		max += max / 2;
+		return { 3, max };
+	}
 	case SpellID::Golem:
-		*mind = 11;
-		*maxd = 17;
-		break;
+		return { 11, 17 };
 	case SpellID::Apocalypse:
-		*mind = myPlayer._pLevel;
-		*maxd = *mind * 6;
-		break;
+		return { myPlayer._pLevel, myPlayer._pLevel * 6 };
 	case SpellID::Elemental:
-		*mind = ScaleSpellEffect(2 * myPlayer._pLevel + 4, sl);
-		/// BUGFIX: add here '*mind /= 2;'
-		*maxd = ScaleSpellEffect(2 * myPlayer._pLevel + 40, sl);
-		/// BUGFIX: add here '*maxd /= 2;'
-		break;
+		/// BUGFIX: Divide min and max by 2
+		return {
+			ScaleSpellEffect(2 * myPlayer._pLevel + 4, spellLevel),
+			ScaleSpellEffect(2 * myPlayer._pLevel + 40, spellLevel)
+		};
 	case SpellID::ChargedBolt:
-		*mind = 1;
-		*maxd = *mind + (myPlayer._pMagic / 4);
-		break;
+		return { 1, 1 + (myPlayer._pMagic / 4) };
 	case SpellID::HolyBolt:
-		*mind = myPlayer._pLevel + 9;
-		*maxd = *mind + 9;
-		break;
-	case SpellID::BloodStar:
-		*mind = (myPlayer._pMagic / 2) + 3 * sl - (myPlayer._pMagic / 8);
-		*maxd = *mind;
-		break;
+		return { myPlayer._pLevel + 9, myPlayer._pLevel + 18 };
+	case SpellID::BloodStar: {
+		const int min = (myPlayer._pMagic / 2) + 3 * spellLevel - (myPlayer._pMagic / 8);
+		return { min, min };
+	}
 	default:
-		break;
+		return { -1, -1 };
 	}
 }
 

Source/missiles.h

@@ -176,7 +176,11 @@ struct Missile {
 extern std::list<Missile> Missiles;
 extern bool MissilePreFlag;
 
-void GetDamageAmt(SpellID i, int *mind, int *maxd);
+struct DamageRange {
+	int min;
+	int max;
+};
+DamageRange GetDamageAmt(SpellID spell, int spellLevel);
 
 /**
  * @brief Returns the direction a vector from p1(x1, y1) to p2(x2, y2) is pointing to.

Source/panels/spell_book.cpp

@@ -25,12 +25,20 @@ namespace devilution {
 
 namespace {
 
-OptionalOwnedClxSpriteList pSBkBtnCel;
-OptionalOwnedClxSpriteList pSpellBkCel;
+OptionalOwnedClxSpriteList spellBookButtons;
+OptionalOwnedClxSpriteList spellBookBackground;
 
 const size_t SpellBookPages = 6;
 const size_t SpellBookPageEntries = 7;
 
+constexpr uint16_t SpellBookButtonWidthDiablo = 76;
+constexpr uint16_t SpellBookButtonWidthHellfire = 61;
+
+uint16_t SpellBookButtonWidth()
+{
+	return gbIsHellfire ? SpellBookButtonWidthHellfire : SpellBookButtonWidthDiablo;
+}
+
 /** Maps from spellbook page number and position to SpellID. */
 const SpellID SpellPages[SpellBookPages][SpellBookPageEntries] = {
 	{ SpellID::Null, SpellID::Firebolt, SpellID::ChargedBolt, SpellID::HolyBolt, SpellID::Healing, SpellID::HealOther, SpellID::Inferno },
@@ -102,35 +110,52 @@ SpellType GetSBookTrans(SpellID ii, bool townok)
 	return st;
 }
 
+StringOrView GetSpellPowerText(SpellID spell, int spellLevel)
+{
+	if (spellLevel == 0) {
+		return _("Unusable");
+	}
+	if (spell == SpellID::BoneSpirit) {
+		return _(/* TRANSLATORS: UI constraints, keep short please.*/ "Dmg: 1/3 target hp");
+	}
+	const auto [min, max] = GetDamageAmt(spell, spellLevel);
+	if (min == -1) {
+		return StringOrView {};
+	}
+	if (spell == SpellID::Healing || spell == SpellID::HealOther) {
+		return fmt::format(fmt::runtime(_(/* TRANSLATORS: UI constraints, keep short please.*/ "Heals: {:d} - {:d}")), min, max);
+	}
+	return fmt::format(fmt::runtime(_(/* TRANSLATORS: UI constraints, keep short please.*/ "Damage: {:d} - {:d}")), min, max);
+}
+
 } // namespace
 
 void InitSpellBook()
 {
-	pSpellBkCel = LoadCel("data\\spellbk", static_cast<uint16_t>(SidePanelSize.width));
-	pSBkBtnCel = LoadCel("data\\spellbkb", gbIsHellfire ? 61 : 76);
+	spellBookBackground = LoadCel("data\\spellbk", static_cast<uint16_t>(SidePanelSize.width));
+	spellBookButtons = LoadCel("data\\spellbkb", SpellBookButtonWidth());
 	LoadSmallSpellIcons();
 }
 
 void FreeSpellBook()
 {
 	FreeSmallSpellIcons();
-	pSBkBtnCel = std::nullopt;
-	pSpellBkCel = std::nullopt;
+	spellBookButtons = std::nullopt;
+	spellBookBackground = std::nullopt;
 }
 
 void DrawSpellBook(const Surface &out)
 {
-	ClxDraw(out, GetPanelPosition(UiPanels::Spell, { 0, 351 }), (*pSpellBkCel)[0]);
-	if (gbIsHellfire && sbooktab < 5) {
-		ClxDraw(out, GetPanelPosition(UiPanels::Spell, { 61 * sbooktab + 7, 348 }), (*pSBkBtnCel)[sbooktab]);
-	} else {
-		// BUGFIX: rendering of page 3 and page 4 buttons are both off-by-one pixel (fixed).
-		int sx = 76 * sbooktab + 7;
-		if (sbooktab == 2 || sbooktab == 3) {
-			sx++;
-		}
-		ClxDraw(out, GetPanelPosition(UiPanels::Spell, { sx, 348 }), (*pSBkBtnCel)[sbooktab]);
-	}
+	constexpr int SpellBookButtonX = 7;
+	constexpr int SpellBookButtonY = 348;
+	ClxDraw(out, GetPanelPosition(UiPanels::Spell, { 0, 351 }), (*spellBookBackground)[0]);
+	const int buttonX = gbIsHellfire && sbooktab < 5
+	    ? SpellBookButtonWidthHellfire * sbooktab
+	    : SpellBookButtonWidthDiablo * sbooktab
+	        // BUGFIX: rendering of page 3 and page 4 buttons are both off-by-one pixel (fixed).
+	        + (sbooktab == 2 || sbooktab == 3 ? 1 : 0);
+
+	ClxDraw(out, GetPanelPosition(UiPanels::Spell, { SpellBookButtonX + buttonX, SpellBookButtonY }), (*spellBookButtons)[sbooktab]);
 	Player &player = *InspectPlayer;
 	uint64_t spl = player._pMemSpells | player._pISpells | player._pAblSpells;
 
@@ -158,32 +183,17 @@ void DrawSpellBook(const Surface &out)
 				PrintSBookStr(out, line1, _("Skill"));
 				break;
 			case SpellType::Charges: {
-				int charges = player.InvBody[INVLOC_HAND_LEFT]._iCharges;
+				const int charges = player.InvBody[INVLOC_HAND_LEFT]._iCharges;
 				PrintSBookStr(out, line1, fmt::format(fmt::runtime(ngettext("Staff ({:d} charge)", "Staff ({:d} charges)", charges)), charges));
 			} break;
 			default: {
-				int mana = GetManaAmount(player, sn) >> 6;
-				int lvl = player.GetSpellLevel(sn);
+				const int mana = GetManaAmount(player, sn) >> 6;
+				const int lvl = player.GetSpellLevel(sn);
 				PrintSBookStr(out, line0, fmt::format(fmt::runtime(pgettext(/* TRANSLATORS: UI constraints, keep short please.*/ "spellbook", "Level {:d}")), lvl), UiFlags::AlignRight);
-				if (lvl == 0) {
-					PrintSBookStr(out, line1, _("Unusable"), UiFlags::AlignRight);
-				} else {
-					if (sn != SpellID::BoneSpirit) {
-						int min;
-						int max;
-						GetDamageAmt(sn, &min, &max);
-						if (min != -1) {
-							if (sn == SpellID::Healing || sn == SpellID::HealOther) {
-								PrintSBookStr(out, line1, fmt::format(fmt::runtime(_(/* TRANSLATORS: UI constraints, keep short please.*/ "Heals: {:d} - {:d}")), min, max), UiFlags::AlignRight);
-							} else {
-								PrintSBookStr(out, line1, fmt::format(fmt::runtime(_(/* TRANSLATORS: UI constraints, keep short please.*/ "Damage: {:d} - {:d}")), min, max), UiFlags::AlignRight);
-							}
-						}
-					} else {
-						PrintSBookStr(out, line1, _(/* TRANSLATORS: UI constraints, keep short please.*/ "Dmg: 1/3 target hp"), UiFlags::AlignRight);
-					}
-					PrintSBookStr(out, line1, fmt::format(fmt::runtime(pgettext(/* TRANSLATORS: UI constraints, keep short please.*/ "spellbook", "Mana: {:d}")), mana));
+				if (const StringOrView text = GetSpellPowerText(sn, lvl); !text.empty()) {
+					PrintSBookStr(out, line1, text, UiFlags::AlignRight);
 				}
+				PrintSBookStr(out, line1, fmt::format(fmt::runtime(pgettext(/* TRANSLATORS: UI constraints, keep short please.*/ "spellbook", "Mana: {:d}")), mana));
 			} break;
 			}
 		}
@@ -220,17 +230,17 @@ void CheckSBook()
 	// The width of the panel excluding the border is 305 pixels. This does not cleanly divide by 4 meaning Diablo tabs
 	// end up with an extra pixel somewhere around the buttons. Vanilla Diablo had the buttons left-aligned, devilutionX
 	// instead justifies the buttons and puts the gap between buttons 2/3. See DrawSpellBook
-	const int TabWidth = gbIsHellfire ? 61 : 76;
+	const int buttonWidth = SpellBookButtonWidth();
 	// Tabs are drawn in a row near the bottom of the panel
 	Rectangle tabArea = { GetPanelPosition(UiPanels::Spell, { 7, 320 }), Size { 305, 29 } };
 	if (tabArea.contains(MousePosition)) {
 		int hitColumn = MousePosition.x - tabArea.position.x;
 		// Clicking on the gutter currently activates tab 3. Could make it do nothing by checking for == here and return early.
-		if (!gbIsHellfire && hitColumn > TabWidth * 2) {
+		if (!gbIsHellfire && hitColumn > buttonWidth * 2) {
 			// Subtract 1 pixel to account for the gutter between buttons 2/3
 			hitColumn--;
 		}
-		sbooktab = hitColumn / TabWidth;
+		sbooktab = hitColumn / buttonWidth;
 	}
 }