Skip to content

Commit

Permalink
Game Events: refactored event handler storage
Browse files Browse the repository at this point in the history 
This throws out the custom smart_list class in favor of a plain std::list. It also greatly simplifies
a few things. First, event handlers no longer remove themselves from the main list in event_handlers.
Now they just flag themselves as disabled (which means they will never execute once marked) and cleaned
up later in a newly added cleanup stage. This means a handler no longer needs to keep its index in the
active handler vector.

This removal of reliance on indices also means I could add the aforementioned cleanup stage. With the
smart_list code, event handlers were never actually removed from the active vector, nor any weak_ptrs
pointing to them removed either. This wasn't exactly a problem, since the handlers were stored via
shared_ptrs which would then simply be null after one deleted itself. Still, it's cleaner to drop any
invalid ones (and unlockable weak_ptrs) from any relevant containers. I've opted to do this in
manager::execute_on_events. Seems a good enough place as any.

The net result of this is the code is much cleaner. We're able to get rid of a bunch of unnecessary
feelers into various classes. This also makes the manager::iteration dereference code a lot easier
to understand. There certainly could be further refactoring, but I think this is a good start.
  • Loading branch information
@Vultraz @GregoryLundberg
Vultraz authored and GregoryLundberg committed Nov 30, 2017
1 parent bd14e5f commit 15d3b4a
Show file tree
Hide file tree
Showing 8 changed files with 104 additions and 1,056 deletions.
74 changes: 12 additions & 62 deletions src/game_events/handlers.cpp
View file
Expand Up @@ -45,90 +45,40 @@ static lg::log_domain log_event_handler("event_handler");
// This file is in the game_events namespace.
namespace game_events
{
/* ** handler_list::iterator ** */

/**
* Dereference.
* If the current element has become invalid, we will increment first.
*/
handler_ptr handler_list::iterator::operator*()
{
// Check for an available handler.
while(iter_.derefable()) {
// Handler still accessible?
if(handler_ptr lock = iter_->lock()) {
return lock;
} else {
// Remove the now-defunct entry.
iter_ = list_t::erase(iter_);
}
}

// End of the list.
return handler_ptr();
}

/* ** event_handler ** */

event_handler::event_handler(const config& cfg, bool imi, handler_vec::size_type index, manager& man)
event_handler::event_handler(const config& cfg, bool imi)
: first_time_only_(cfg["first_time_only"].to_bool(true))
, is_menu_item_(imi)
, index_(index)
, man_(&man)
, disabled_(false)
, cfg_(cfg)
{
}

/**
* Disables *this, removing it from the game.
* (Technically, the handler is only removed once no one is hanging on to a
* handler_ptr to *this. So be careful how long they persist.)
*
* WARNING: *this may be destroyed at the end of this call, unless
* the caller maintains a handler_ptr to this.
*/
void event_handler::disable()
{
assert(man_);
assert(man_->event_handlers_);

// Handlers must have an index after they're created.
assert(index_ < man_->event_handlers_->size());

// Disable this handler.
(*man_->event_handlers_)[index_].reset();
assert(!disabled_);;
disabled_ = true;
}

/**
* Handles the queued event, according to our WML instructions.
* WARNING: *this may be destroyed at the end of this call, unless
* the caller maintains a handler_ptr to this.
*
* @param[in] event_info Information about the event that needs handling.
* @param[in,out] handler_p The caller's smart pointer to *this. It may be
* reset() during processing.
*/
void event_handler::handle_event(const queued_event& event_info, handler_ptr& handler_p, game_lua_kernel& lk)

void event_handler::handle_event(const queued_event& event_info, game_lua_kernel& lk)
{
// We will need our config after possibly self-destructing. Make a copy now.
// TODO: instead of copying possibly huge config objects we should use shared things and only increase a refcount
// here.
vconfig vcfg(cfg_, true);
// If this even is disabled, do nothing.
if(disabled_) {
return;
}

if(is_menu_item_) {
DBG_NG << cfg_["name"] << " will now invoke the following command(s):\n" << cfg_;
}

// Disable this handler if it's a one-time event.
if(first_time_only_) {
// Disable this handler.
disable();

// Also remove our caller's hold on us.
handler_p.reset();
}
// *WARNING*: At this point, dereferencing this could be a memory violation!

lk.run_wml_action("command", vcfg, event_info);
lk.run_wml_action("command", vconfig(cfg_, false), event_info);
sound::commit_music_changes();
}

Expand Down
130 changes: 16 additions & 114 deletions src/game_events/handlers.hpp
View file
Expand Up @@ -24,8 +24,8 @@
#pragma once

#include "config.hpp"
#include "utils/smart_list.hpp"

#include <list>
#include <memory>
#include <set>
#include <string>
Expand All @@ -39,21 +39,18 @@ struct queued_event;
class event_handler; // Defined a few lines down.
class manager;

/// Shared pointer to handler objects.
typedef std::shared_ptr<event_handler> handler_ptr;

/// Storage of event handlers.
typedef std::vector<handler_ptr> handler_vec;
using handler_ptr = std::shared_ptr<event_handler>;
using weak_handler_ptr = std::weak_ptr<event_handler>;
using handler_list = std::list<weak_handler_ptr>;

class event_handler
{
public:
event_handler(const config& cfg, bool is_menu_item, handler_vec::size_type index, manager&);
event_handler(const config& cfg, bool is_menu_item);

/** The index of *this should only be of interest when controlling iterations. */
handler_vec::size_type index() const
bool disabled() const
{
return index_;
return disabled_;
}

bool matches_name(const std::string& name, const game_data* data) const;
Expand All @@ -63,9 +60,15 @@ class event_handler
return is_menu_item_;
}

/** Disables *this, removing it from the game. */
/** Flag this handler as disabled. */
void disable();
void handle_event(const queued_event& event_info, handler_ptr& handler_p, game_lua_kernel&);

/**
* Handles the queued event, according to our WML instructions.
*
* @param[in] event_info Information about the event that needs handling.
*/
void handle_event(const queued_event& event_info, game_lua_kernel&);

const config& get_config() const
{
Expand All @@ -75,109 +78,8 @@ class event_handler
private:
bool first_time_only_;
bool is_menu_item_;
handler_vec::size_type index_;
manager* man_;
bool disabled_;
config cfg_;
};

/**
* This is a wrapper for a list of weak pointers to handlers. It allows forward
* iterations of the list, with each element returned as a shared pointer.
* (Weak pointers that fail to lock are silently removed from the list.) These
* iterations can be used recursively, even when the innermost iteration might
* erase arbitrary elements from the list.
*
* The interface is not the standard list interface because that would be
* inconvenient. The functionality implemented is that required by Wesnoth.
*/
class handler_list
{
/// The weak pointers that are used internally.
typedef std::weak_ptr<event_handler> internal_ptr;

/// The underlying list.
typedef utils::smart_list<internal_ptr> list_t;

public:
/**
* Handler list iterators are rather limited. They can be constructed
* from a reference iterator (not default constructed), incremented,
* and dereferenced. Consecutive dereferences are not guaranteed to
* return the same element (if the list mutates between them, the next
* element might be returned). An increment guarantees that the next
* dereference will differ from the previous (unless at the end of the
* list). The end of the list is indicated by dereferencing to a null
* pointer.
*/
class iterator
{
/// The current element.
list_t::iterator iter_;

public:
/// Initialized constructor (to be called by handler_list).
explicit iterator(const list_t::iterator& base_iter)
: iter_(base_iter)
{
}

/// Increment.
iterator& operator++()
{
++iter_;
return *this;
}
/// Dereference.
handler_ptr operator*();
};
friend class iterator;
typedef iterator const_iterator;

public:
/**
* Default constructor.
* Note: This explicit definition is required (by the more pedantic
* compilers) in order to declare a default-constructed, static,
* and const variable in event_handlers::get(), in handlers.cpp.
*/
handler_list()
: data_()
{
}

const_iterator begin() const
{
return iterator(const_cast<list_t&>(data_).begin());
}

// The above const_cast is so the iterator can remove obsolete entries.
const_iterator end() const
{
return iterator(const_cast<list_t&>(data_).end());
}

// push_front() is probably unneeded, but I'll leave the code here, just in case.
// (These lists must be maintained in index order, which means pushing to the back.)
void push_front(const handler_ptr& p)
{
data_.push_front(internal_ptr(p));
}

void push_back(const handler_ptr& p)
{
data_.push_back(internal_ptr(p));
}

void clear()
{
data_.clear();
}

private:
/// No implementation of operator=() since smart_list does not support it.
handler_list& operator=(const handler_list&);

/// The actual list.
list_t data_;
};
}
43 changes: 30 additions & 13 deletions src/game_events/manager.cpp
View file
Expand Up @@ -44,7 +44,7 @@ namespace game_events
/** Create an event handler. */
void manager::add_event_handler(const config& handler, bool is_menu_item)
{
event_handlers_->add_event_handler(handler, *this, is_menu_item);
event_handlers_->add_event_handler(handler, is_menu_item);
}

/** Removes an event handler. */
Expand Down Expand Up @@ -101,7 +101,6 @@ manager::iteration::iteration(const std::string& event_name, manager& man)
: main_list_(man.event_handlers_->get(event_name))
, var_list_(man.event_handlers_->get_dynamic())
, event_name_(event_name)
, end_(man.event_handlers_->size())
, current_is_known_(false)
, main_is_current_(false)
, main_it_(main_list_.begin())
Expand All @@ -113,7 +112,7 @@ manager::iteration::iteration(const std::string& event_name, manager& man)
/**
* Increment
* Incrementing guarantees that the next dereference will differ from the
* previous derference (unless the iteration is exhausted). However, multiple
* previous dereference (unless the iteration is exhausted). However, multiple
* increments between dereferences are allowed to have the same effect as a
* single increment.
*/
Expand All @@ -139,29 +138,44 @@ manager::iteration& manager::iteration::operator++()
return *this;
}

// Small helper function to ensure we don't try to dereference an invalid iterator.
static handler_ptr lock_ptr(const handler_list& list, handler_list::iterator iter)
{
if(iter != list.end()) {
if(handler_ptr ptr = iter->lock()) {
return ptr;
} else {
assert(false && "Found null handler in handler list!");
}
}

return nullptr;
}

/**
* Dereference
* Will return a null pointer when the end of the iteration is reached.
*/
handler_ptr manager::iteration::operator*()
{
// Get the candidate for the current element from the main list.
handler_ptr main_ptr = *main_it_;
handler_vec::size_type main_index = ptr_index(main_ptr);
handler_ptr main_ptr = lock_ptr(main_list_, main_it_);

// Get the candidate for the current element from the var list.
handler_ptr var_ptr = *var_it_;
handler_ptr var_ptr = lock_ptr(var_list_, var_it_);

// (Loop while var_ptr would be chosen over main_ptr, but the name does not match.)
while(var_ptr && var_ptr->index() < main_index && !var_ptr->matches_name(event_name_, gamedata_)) {
var_ptr = *++var_it_;
// If we have a variable-name event but its name doesn't match event_name_,
// keep iterating until we find a match. If we reach var_list_ end var_ptr
// will be nullptr.
while(var_ptr && !var_ptr->matches_name(event_name_, gamedata_)) {
var_ptr = lock_ptr(var_list_, ++var_it_);
}

handler_vec::size_type var_index = ptr_index(var_ptr);
// Are either of the handler ptrs valid?
current_is_known_ = main_ptr != nullptr || var_ptr != nullptr;

// Which list? (Index ties go to the main list.)
current_is_known_ = main_index < end_ || var_index < end_;
main_is_current_ = main_index <= var_index;
// If var_ptr is invalid, we use the ptr from the main list.
main_is_current_ = var_ptr == nullptr;

if(!current_is_known_) {
return nullptr; // End of list; return a null pointer.
Expand Down Expand Up @@ -212,6 +226,9 @@ void manager::execute_on_events(const std::string& event_id, manager::event_func
func(*this, hand);
++iter;
}

// Clean up expired ptrs. This saves us effort later since it ensures every ptr is valid.
event_handlers_->clean_up_expired_handlers(event_id);
}

game_events::wml_event_pump& manager::pump()
Expand Down
14 changes: 2 additions & 12 deletions src/game_events/manager.hpp
View file
Expand Up @@ -69,23 +69,13 @@ class manager
// Dereference:
handler_ptr operator*();

private:
/// Gets the index from a pointer, capped at end_.
handler_vec::size_type ptr_index(const handler_ptr& ptr) const
{
return !bool(ptr) ? end_ : std::min(ptr->index(), end_);
}

private:
/// The fixed-name event handlers for this iteration.
const handler_list& main_list_;
handler_list& main_list_;
/// The varying-name event handlers for this iteration.
const handler_list& var_list_;
handler_list& var_list_;
/// The event name for this iteration.
const std::string event_name_;
/// The end of this iteration. We intentionally exclude handlers
/// added after *this is constructed.
const handler_vec::size_type end_;

/// Set to true upon dereferencing.
bool current_is_known_;
Expand Down

0 comments on commit 15d3b4a

Please sign in to comment.