From 88b0fe59536b3976ea2894cee17ca3773d0f8300 Mon Sep 17 00:00:00 2001 From: Charles Dang Date: Thu, 28 Dec 2017 23:12:41 -0500 Subject: [PATCH] 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 | 69 +------------------------------- src/game_events/handlers.hpp | 10 +++-- src/game_events/manager.cpp | 69 +++++++++++++++++++++++++------- src/game_events/manager_impl.cpp | 21 +++++----- src/game_events/manager_impl.hpp | 7 +++- 5 files changed, 81 insertions(+), 95 deletions(-) diff --git a/src/game_events/handlers.cpp b/src/game_events/handlers.cpp index c676d26a2e8f..d1b0890d0bbd 100644 --- a/src/game_events/handlers.cpp +++ b/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& 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 diff --git a/src/game_events/handlers.hpp b/src/game_events/handlers.hpp index f2bb4cd2821c..f6525ec1ce08 100644 --- a/src/game_events/handlers.hpp +++ b/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& types); + + const std::vector& 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 types_; }; } diff --git a/src/game_events/manager.cpp b/src/game_events/manager.cpp index 042dffc77aa7..9c170561c453 100644 --- a/src/game_events/manager.cpp +++ b/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() diff --git a/src/game_events/manager_impl.cpp b/src/game_events/manager_impl.cpp index 80a4face5585..047107fcc87f 100644 --- a/src/game_events/manager_impl.cpp +++ b/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(); diff --git a/src/game_events/manager_impl.hpp b/src/game_events/manager_impl.hpp index 3a37c043a0c8..eb9d810a13f5 100644 --- a/src/game_events/manager_impl.hpp +++ b/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);