Further reworking of event iteration interface

Fixes #2312. Turns out constructing a list of matching handlers breaks some undocumented
behavior, namely events being executed in the order they're added (usually the order in
which they appear in the WML). It also broke certain variable substitution by moving variable
interpolation wholly before any event execution, as opposed to between each event.

This commit implements an even simpler method: simply iterating through every registered
handler in order, checking for a name match on each one, and if it passes, immediately
executing the provided function. Cleanup will only occur once the toplevel call to
execute_on_events is completed.

This commit also removes the immediately cleanup upon removing a handler. This should
ensure (I didn't test explicitly, but from other incidental issues I'm sure this is the
case) the size of the main vector doesn't change during iteration.

I also changed the pre-add standardization code to only affect event names without variables.
Variable names are standardized later after variable substitution.

src/game_events/handlers.cpp

@@ -43,11 +43,12 @@ namespace game_events
 {
 /* ** event_handler ** */
 
-event_handler::event_handler(config&& cfg, bool imi)
+event_handler::event_handler(config&& cfg, bool imi, const std::vector<std::string>& types)
 	: first_time_only_(cfg["first_time_only"].to_bool(true))
 	, is_menu_item_(imi)
 	, disabled_(false)
 	, cfg_(cfg)
+	, types_(types)
 {
 }
 
@@ -75,70 +76,4 @@ void event_handler::handle_event(const queued_event& event_info, game_lua_kernel
 	sound::commit_music_changes();
 }
 
-bool event_handler::matches_name(const std::string& name, const game_data* gd) const
-{
-	const std::string my_names = !gd
-		? cfg_["name"].str()
-		: utils::interpolate_variables_into_string(cfg_["name"], *gd);
-
-	std::string::const_iterator
-		itor, it_begin = my_names.begin(),
-		it_end         = my_names.end(),
-		match_it       = name.begin(),
-		match_begin    = name.begin(),
-		match_end      = name.end();
-
-	int skip_count = 0;
-	for(itor = it_begin; itor != it_end; ++itor) {
-		bool do_eat = false, do_skip = false;
-
-		switch(*itor) {
-		case ',':
-			if(itor - it_begin - skip_count == match_it - match_begin && match_it == match_end) {
-				return true;
-			}
-			it_begin = itor + 1;
-			match_it = match_begin;
-			skip_count = 0;
-			continue;
-		case '\f':
-		case '\n':
-		case '\r':
-		case '\t':
-		case '\v':
-			do_skip = (match_it == match_begin || match_it == match_end);
-			break;
-		case ' ':
-			do_skip = (match_it == match_begin || match_it == match_end);
-			FALLTHROUGH;
-		case '_':
-			do_eat = (match_it != match_end && (*match_it == ' ' || *match_it == '_'));
-			break;
-		default:
-			do_eat = (match_it != match_end && *match_it == *itor);
-			break;
-		}
-
-		if(do_eat) {
-			++match_it;
-		} else if(do_skip) {
-			++skip_count;
-		} else {
-			itor = std::find(itor, it_end, ',');
-			if(itor == it_end) {
-				return false;
-			}
-			it_begin = itor + 1;
-			match_it = match_begin;
-			skip_count = 0;
-		}
-	}
-
-	if(itor - it_begin - skip_count == match_it - match_begin && match_it == match_end) {
-		return true;
-	}
-
-	return false;
-}
-
 } // end namespace game_events

src/game_events/handlers.hpp

@@ -37,15 +37,18 @@ struct queued_event;
 class event_handler
 {
 public:
-	event_handler(config&& cfg, bool is_menu_item);
+	event_handler(config&& cfg, bool is_menu_item, const std::vector<std::string>& types);
+
+	const std::vector<std::string>& names() const
+	{
+		return types_;
+	}
 
 	bool disabled() const
 	{
 		return disabled_;
 	}
 
-	bool matches_name(const std::string& name, const game_data* data) const;
-
 	bool is_menu_item() const
 	{
 		return is_menu_item_;
@@ -71,6 +74,7 @@ class event_handler
 	bool is_menu_item_;
 	bool disabled_;
 	config cfg_;
+	std::vector<std::string> types_;
 };
 
 }

src/game_events/manager.cpp

@@ -19,6 +19,8 @@
 #include "game_events/menu_item.hpp"
 #include "game_events/pump.hpp"
 
+#include "formula/string_utils.hpp"
+#include "game_data.hpp"
 #include "log.hpp"
 #include "resources.hpp"
 #include "serialization/string_utils.hpp"
@@ -119,26 +121,65 @@ void manager::write_events(config& cfg) const
 
 void manager::execute_on_events(const std::string& event_id, manager::event_func_t func)
 {
-	// Start with a list of weak_ptrs to all matching event names (ie, "moveto").
-	handler_list matching_events = event_handlers_->get(event_id);
-
-	// Then check events with variables in their names for a match.
-	for(const auto& handler : event_handlers_->get_dynamic()) {
-		if(handler_ptr ptr = handler.lock()) {
-			if(ptr->matches_name(event_id, resources::gamedata)) {
-				matching_events.push_back(handler);
-			}
+	const std::string standardized_event_id = event_handlers::standardize_name(event_id);
+	const game_data* gd = resources::gamedata;
+	auto& active_handlers = event_handlers_->get_active();
+
+	// Save the end outside the loop so the end point remains constant,
+	// even if new events are added to the queue.
+	const unsigned saved_end = active_handlers.size();
+
+	// It's possible for this function to call itself again. If list cleanup occurred
+	// after each recursive call, the saved_end variable in the call above it would no
+	// longer be a valid end index. To work around that, we delay cleanup until the
+	// toplevel call is done running through the events (ie, when recursion_depth is 0).
+	static unsigned recursion_depth = 0;
+	++recursion_depth;
+
+	for(unsigned i = 0; i < saved_end; ++i) {
+		handler_ptr handler = nullptr;
+
+		try {
+			handler = active_handlers.at(i);
+		} catch(const std::out_of_range&) {
+			continue;
 		}
-	}
 
-	for(auto& handler : matching_events) {
-		if(handler_ptr ptr = handler.lock()) {
-			func(*this, ptr);
+		// Shouldn't happen, but we're just being safe.
+		if(!handler || handler->disabled()) {
+			continue;
+		}
+
+		// Could be more than one.
+		for(const std::string& name : handler->names()) {
+			bool matches = false;
+
+			if(utils::might_contain_variables(name)) {
+				// If we don't have gamedata, we can't interpolate variables, so there's
+				// no way the name will match. Move on to the next one in that case.
+				if(!gd) {
+					continue;
+				}
+
+				matches = standardized_event_id ==
+					event_handlers::standardize_name(utils::interpolate_variables_into_string(name, *gd));
+			} else {
+				matches = standardized_event_id == name;
+			}
+
+			if(matches) {
+				func(*this, handler);
+				break;
+			}
 		}
 	}
 
+	--recursion_depth;
+
 	// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
-	event_handlers_->clean_up_expired_handlers(event_id);
+	if(recursion_depth == 0) {
+		event_handlers_->clean_up_expired_handlers(standardized_event_id);
+	}
 }
 
 game_events::wml_event_pump& manager::pump()

src/game_events/manager_impl.cpp

@@ -97,6 +97,7 @@ void event_handlers::add_event_handler(const config& cfg, bool is_menu_item)
 	if(!id.empty()) {
 		// Ignore this handler if there is already one with this ID.
 		auto find_it = id_map_.find(id);
+
 		if(find_it != id_map_.end() && !find_it->second.expired()) {
 			DBG_EH << "ignoring event handler for name='" << name << "' with id '" << id << "'\n";
 			return;
@@ -112,9 +113,13 @@ void event_handlers::add_event_handler(const config& cfg, bool is_menu_item)
 
 	// ...and standardize each one individually. This ensures they're all valid for by-name lookup.
 	for(std::string& single_name : standardized_names) {
-		single_name = standardize_name(single_name);
+		if(!utils::might_contain_variables(single_name)) {
+			single_name = standardize_name(single_name);
+		}
 	}
 
+	assert(standardized_names.size() != 0);
+
 	// Write the new name back to the config.
 	name = utils::join(standardized_names);
 	event_cfg["name"] = name;
@@ -123,7 +128,7 @@ void event_handlers::add_event_handler(const config& cfg, bool is_menu_item)
 	// Do note active_ holds the main shared_ptr, and the other three containers
 	// construct weak_ptrs from the shared one.
 	DBG_EH << "inserting event handler for name=" << name << " with id=" << id << "\n";
-	active_.emplace_back(new event_handler(std::move(event_cfg), is_menu_item));
+	active_.emplace_back(new event_handler(std::move(event_cfg), is_menu_item, standardized_names));
 
 	//
 	//  !! event_cfg is invalid past this point! DO NOT USE!
@@ -170,14 +175,10 @@ void event_handlers::remove_event_handler(const std::string& id)
 		// Do this even if the lock failed.
 		id_map_.erase(find_it);
 
-		// Remove handler from other lists if we got a lock. If we didn't, the handler
-		// was already from the main list previously, so any other weak_ptrs to it in
-		// the by-name or with-vars list should already have been dropped. If for some
-		// reason they haven't, no problem. They don't do anything and will be dropped
-		// next cleanup.
-		if(handler) {
-			clean_up_expired_handlers(handler->get_config()["name"]);
-		}
+		// We don't delete the handler from the other lists just yet. This is to ensure
+		// the main handler list's size doesn't change when iterating over the handlers.
+		// Any disabled handlers don't get executed, and will be removed during the next
+		// cleanup pass.
 	}
 
 	log_handlers();

src/game_events/manager_impl.hpp

@@ -47,10 +47,10 @@ class event_handlers
 
 	void log_handlers();
 
+public:
 	/** Utility to standardize the event names used in by_name_. */
 	static std::string standardize_name(const std::string& name);
 
-public:
 	event_handlers()
 		: active_()
 		, by_name_()
@@ -71,6 +71,11 @@ class event_handlers
 		return active_;
 	}
 
+	handler_vec_t& get_active()
+	{
+		return active_;
+	}
+
 	/** Access to the handlers with fixed event names, by event name. */
 	handler_list& get(const std::string& name);