Adjust and clean up

Author: AJenbo

Source/lua/lua_global.cpp

@@ -334,6 +334,8 @@ void LuaShutdown()
 #ifdef _DEBUG
 	LuaReplShutdown();
 #endif
+	// Must clear before destroying the Lua state: registered callbacks
+	// capture sol::function handles that reference CurrentLuaState.
 	ClearTownerDialogOptions();
 	CurrentLuaState = std::nullopt;
 }

Source/stores.cpp

@@ -7,9 +7,10 @@
 
 #include <algorithm>
 #include <cstdint>
+#include <optional>
 #include <string>
 #include <string_view>
-#include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include <fmt/format.h>
@@ -35,6 +36,7 @@
 #include "towners.h"
 #include "utils/format_int.hpp"
 #include "utils/language.h"
+#include "utils/log.hpp"
 #include "utils/str_cat.hpp"
 #include "utils/utf8.hpp"
 
@@ -71,41 +73,69 @@ TalkID OldActiveStore;
 /** Temporary item used to hold the item being traded */
 Item TempItem;
 
-std::unordered_map<std::string, std::vector<TownerDialogOption>> ExtraTownerOptions;
+std::vector<std::pair<std::string, std::vector<TownerDialogOption>>> ExtraTownerOptions;
 
 const char *TownerNameForTalkID(TalkID s)
 {
-	const auto lookup = [](const _talker_id id) -> const char * {
-		auto it = TownerShortNames.find(id);
-		return it != TownerShortNames.end() ? it->second : nullptr;
-	};
 	switch (s) {
-	case TalkID::Smith: return lookup(TOWN_SMITH);
-	case TalkID::Witch: return lookup(TOWN_WITCH);
-	case TalkID::Boy: return lookup(TOWN_PEGBOY);
-	case TalkID::Healer: return lookup(TOWN_HEALER);
-	case TalkID::Storyteller: return lookup(TOWN_STORY);
-	case TalkID::Tavern: return lookup(TOWN_TAVERN);
-	case TalkID::Drunk: return lookup(TOWN_DRUNK);
-	case TalkID::Barmaid: return lookup(TOWN_BMAID);
+	case TalkID::Smith: return "griswold";
+	case TalkID::Witch: return "adria";
+	case TalkID::Boy: return "wirt";
+	case TalkID::Healer: return "pepin";
+	case TalkID::Storyteller: return "cain";
+	case TalkID::Tavern: return "ogden";
+	case TalkID::Drunk: return "farnham";
+	case TalkID::Barmaid: return "gillian";
 	default: return nullptr;
 	}
 }
 
+/** Finds the entry for a towner in ExtraTownerOptions, or nullptr if none. */
+static std::vector<TownerDialogOption> *FindExtraTownerOptions(std::string_view townerName)
+{
+	for (auto &[name, opts] : ExtraTownerOptions) {
+		if (name == townerName)
+			return &opts;
+	}
+	return nullptr;
+}
+
 void RegisterTownerDialogOption(std::string_view townerName,
     std::function<std::string()> getLabel,
     std::function<void()> onSelect)
 {
-	ExtraTownerOptions[std::string(townerName)].push_back({ std::move(getLabel), std::move(onSelect) });
+	// Validate that the towner name is known.
+	bool found = false;
+	for (const auto &[id, shortName] : TownerShortNames) {
+		if (shortName == townerName) {
+			found = true;
+			break;
+		}
+	}
+	if (!found) {
+		LogWarn("RegisterTownerDialogOption: unknown towner name \"{}\"", townerName);
+	}
+
+	if (auto *opts = FindExtraTownerOptions(townerName); opts != nullptr) {
+		opts->push_back({ std::move(getLabel), std::move(onSelect) });
+	} else {
+		std::vector<TownerDialogOption> newOpts;
+		newOpts.push_back({ std::move(getLabel), std::move(onSelect) });
+		ExtraTownerOptions.emplace_back(std::string(townerName), std::move(newOpts));
+	}
 }
 
-/** Maps dialog line number to ExtraTownerOptions index for options visible in the current dialog session */
-static std::unordered_map<int, size_t> CurrentExtraOptionIndices;
+/**
+ * Maps dialog line number to ExtraTownerOptions vector index for
+ * options visible in the current dialog session.
+ * Indexed by text line number (0..NumStoreLines-1).
+ */
+static std::optional<size_t> CurrentExtraOptionIndices[NumStoreLines];
 
 void ClearTownerDialogOptions()
 {
 	ExtraTownerOptions.clear();
-	CurrentExtraOptionIndices.clear();
+	std::fill(std::begin(CurrentExtraOptionIndices), std::end(CurrentExtraOptionIndices), std::nullopt);
 }
 
 namespace {
@@ -2345,19 +2375,33 @@ void StartStore(TalkID s)
 		break;
 	}
 
-	CurrentExtraOptionIndices.clear();
+	std::fill(std::begin(CurrentExtraOptionIndices), std::end(CurrentExtraOptionIndices), std::nullopt);
 	if (const char *extraTownerName = TownerNameForTalkID(s); extraTownerName != nullptr) {
-		if (auto extraIt = ExtraTownerOptions.find(extraTownerName); extraIt != ExtraTownerOptions.end()) {
+		if (auto *extraOpts = FindExtraTownerOptions(extraTownerName); extraOpts != nullptr) {
+			// Find the last selectable line (the "leave"/"say goodbye" option).
+			int lastSelectableLine = -1;
+			for (int i = NumStoreLines - 1; i >= 0; --i) {
+				if (TextLine[i].isSelectable()) {
+					lastSelectableLine = i;
+					break;
+				}
+			}
+
+			// Insert extra options into empty even-numbered lines before the leave option.
 			size_t optIdx = 0;
-			for (int line = 14; line < 18 && optIdx < extraIt->second.size(); line += 2) {
-				if (TextLine[line].hasText()) break;
-				std::string label = extraIt->second[optIdx].getLabel();
+			for (int line = 10; line < lastSelectableLine && optIdx < extraOpts->size(); line += 2) {
+				if (TextLine[line].hasText()) continue;
+				std::string label = (*extraOpts)[optIdx].getLabel();
 				if (!label.empty()) {
 					AddSText(0, line, label, UiFlags::ColorWhite | UiFlags::AlignCenter, true);
 					CurrentExtraOptionIndices[line] = optIdx;
 				}
 				++optIdx;
 			}
+			if (optIdx < extraOpts->size()) {
+				LogWarn("Towner \"{}\" dialog: {} extra option(s) could not be placed (no empty lines)",
+				    extraTownerName, extraOpts->size() - optIdx);
+			}
 		}
 	}
 
@@ -2626,10 +2670,15 @@ void StoreEnter()
 
 	PlaySFX(SfxID::MenuSelect);
 
-	if (auto extraOptIt = CurrentExtraOptionIndices.find(CurrentTextLine); extraOptIt != CurrentExtraOptionIndices.end()) {
+	if (CurrentTextLine >= 0 && CurrentTextLine < NumStoreLines && CurrentExtraOptionIndices[CurrentTextLine].has_value()) {
+		size_t optIdx = *CurrentExtraOptionIndices[CurrentTextLine];
 		if (const char *townerName = TownerNameForTalkID(ActiveStore); townerName != nullptr) {
-			if (auto it = ExtraTownerOptions.find(townerName); it != ExtraTownerOptions.end() && extraOptIt->second < it->second.size()) {
-				it->second[extraOptIt->second].onSelect();
+			if (auto *extraOpts = FindExtraTownerOptions(townerName); extraOpts != nullptr && optIdx < extraOpts->size()) {
+				ActiveStore = TalkID::None;
+				(*extraOpts)[optIdx].onSelect();
+				// If onSelect() set ActiveStore (e.g. to open a sub-dialog), preserve it.
+				// Otherwise it stays TalkID::None (dialog closed).
+				return;
 			}
 		}
 		ActiveStore = TalkID::None;

Source/stores.h

@@ -10,7 +10,7 @@
 #include <optional>
 #include <string>
 #include <string_view>
-#include <unordered_map>
+#include <utility>
 #include <vector>
 
 #include "DiabloUI/ui_flags.hpp"
@@ -117,17 +117,29 @@ struct TownerDialogOption {
 };
 
 /** Extra dialog options injected by mods, keyed by towner short name. */
-extern DVL_API_FOR_TEST std::unordered_map<std::string, std::vector<TownerDialogOption>> ExtraTownerOptions;
+extern DVL_API_FOR_TEST std::vector<std::pair<std::string, std::vector<TownerDialogOption>>> ExtraTownerOptions;
 
 /**
  * @brief Returns the towner short name for a top-level TalkID, or nullptr if not a towner store.
+ *
+ * Only maps top-level store entries (Smith, Witch, Boy, Healer, Storyteller, Tavern, Drunk, Barmaid).
+ * Sub-pages (SmithBuy, WitchSell, etc.) return nullptr.
  */
 DVL_API_FOR_TEST const char *TownerNameForTalkID(TalkID s);
 
 /**
  * @brief Registers a dynamic dialog option for a towner's talk menu.
  *
- * @param townerName   Short name of the towner (e.g. "farnham").
+ * Options are inserted into empty even-numbered lines before the towner's "leave" option.
+ * If no empty lines are available the option is silently skipped for that dialog session
+ * and a warning is logged.
+ *
+ * When the player selects a mod option, onSelect() is called. By default the dialog is
+ * closed afterwards. If onSelect() sets ActiveStore to a value other than TalkID::None,
+ * that value is preserved (e.g. to open a sub-dialog).
+ *
+ * @param townerName   Short name of the towner (e.g. "farnham"). Must match one of the
+ *                     names in TownerShortNames; a warning is logged for unknown names.
  * @param getLabel     Called when the dialog is built; return a non-empty string to show the
  *                     option, or an empty string to hide it.
  * @param onSelect     Called when the player chooses this option.
@@ -136,7 +148,12 @@ void RegisterTownerDialogOption(std::string_view townerName,
     std::function<std::string()> getLabel,
     std::function<void()> onSelect);
 
-/** Clears all mod-registered towner dialog options (e.g. before mod reload / Lua shutdown). */
+/**
+ * @brief Clears all mod-registered towner dialog options.
+ *
+ * Must be called before the Lua state is destroyed, since registered callbacks
+ * capture sol::function handles that reference the Lua state.
+ */
 void ClearTownerDialogOptions();
 
 void AddStoreHoldRepair(Item *itm, int8_t i);

test/stores_test.cpp

@@ -6,6 +6,16 @@ using namespace devilution;
 
 namespace {
 
+/** Helper to find the options vector for a towner by short name, or nullptr. */
+std::vector<TownerDialogOption> *FindOptions(std::string_view name)
+{
+	for (auto &[key, opts] : ExtraTownerOptions) {
+		if (key == name)
+			return &opts;
+	}
+	return nullptr;
+}
+
 TEST(Stores, AddStoreHoldRepair_magic)
 {
 	devilution::Item *item;
@@ -113,8 +123,10 @@ TEST(Stores, RegisterTownerDialogOption_afterClearDoesNotAccumulate)
 	ClearTownerDialogOptions();
 	RegisterTownerDialogOption("farnham", []() { return std::string("Second"); }, []() {});
 
-	ASSERT_EQ(ExtraTownerOptions.at("farnham").size(), 1u);
-	EXPECT_EQ(ExtraTownerOptions.at("farnham")[0].getLabel(), "Second");
+	auto *opts = FindOptions("farnham");
+	ASSERT_NE(opts, nullptr);
+	ASSERT_EQ(opts->size(), 1u);
+	EXPECT_EQ((*opts)[0].getLabel(), "Second");
 
 	ClearTownerDialogOptions();
 }
@@ -125,9 +137,10 @@ TEST(Stores, RegisterTownerDialogOption_storesOption)
 
 	RegisterTownerDialogOption("farnham", []() { return std::string("Go to Tiny Town"); }, []() {});
 
-	ASSERT_EQ(ExtraTownerOptions.count("farnham"), 1u);
-	ASSERT_EQ(ExtraTownerOptions.at("farnham").size(), 1u);
-	EXPECT_EQ(ExtraTownerOptions.at("farnham")[0].getLabel(), "Go to Tiny Town");
+	auto *opts = FindOptions("farnham");
+	ASSERT_NE(opts, nullptr);
+	ASSERT_EQ(opts->size(), 1u);
+	EXPECT_EQ((*opts)[0].getLabel(), "Go to Tiny Town");
 
 	ClearTownerDialogOptions();
 }
@@ -139,7 +152,10 @@ TEST(Stores, RegisterTownerDialogOption_callsOnSelect)
 	bool called = false;
 	RegisterTownerDialogOption("farnham", []() { return std::string("Travel"); }, [&called]() { called = true; });
 
-	ExtraTownerOptions.at("farnham")[0].onSelect();
+	auto *opts = FindOptions("farnham");
+	ASSERT_NE(opts, nullptr);
+	ASSERT_EQ(opts->size(), 1u);
+	(*opts)[0].onSelect();
 	EXPECT_TRUE(called);
 
 	ClearTownerDialogOptions();
@@ -151,8 +167,10 @@ TEST(Stores, RegisterTownerDialogOption_emptyLabelHidesOption)
 
 	RegisterTownerDialogOption("farnham", []() { return std::string(""); }, []() {});
 
-	ASSERT_EQ(ExtraTownerOptions.at("farnham").size(), 1u);
-	EXPECT_TRUE(ExtraTownerOptions.at("farnham")[0].getLabel().empty());
+	auto *opts = FindOptions("farnham");
+	ASSERT_NE(opts, nullptr);
+	ASSERT_EQ(opts->size(), 1u);
+	EXPECT_TRUE((*opts)[0].getLabel().empty());
 
 	ClearTownerDialogOptions();
 }
@@ -164,10 +182,14 @@ TEST(Stores, RegisterTownerDialogOption_multipleTowners)
 	RegisterTownerDialogOption("farnham", []() { return std::string("A"); }, []() {});
 	RegisterTownerDialogOption("griswold", []() { return std::string("B"); }, []() {});
 
-	EXPECT_EQ(ExtraTownerOptions.at("farnham").size(), 1u);
-	EXPECT_EQ(ExtraTownerOptions.at("griswold").size(), 1u);
-	EXPECT_EQ(ExtraTownerOptions.at("farnham")[0].getLabel(), "A");
-	EXPECT_EQ(ExtraTownerOptions.at("griswold")[0].getLabel(), "B");
+	auto *farnhamOpts = FindOptions("farnham");
+	auto *griswoldOpts = FindOptions("griswold");
+	ASSERT_NE(farnhamOpts, nullptr);
+	ASSERT_NE(griswoldOpts, nullptr);
+	EXPECT_EQ(farnhamOpts->size(), 1u);
+	EXPECT_EQ(griswoldOpts->size(), 1u);
+	EXPECT_EQ((*farnhamOpts)[0].getLabel(), "A");
+	EXPECT_EQ((*griswoldOpts)[0].getLabel(), "B");
 
 	ClearTownerDialogOptions();
 }