From a226e6dc10eafc9ede49be5959c8e5b9c996fcb8 Mon Sep 17 00:00:00 2001 From: Chris Beck Date: Thu, 26 Jun 2014 22:17:54 -0400 Subject: [PATCH] fixup halos segfaulting fixes up commit 82c6b98907d9709aef0d23a3846c1e75ac48e1d5 Use smart "handles" for halos which have been added to a halo manager. The handles remember what manager they came from, and delete themselves automatically on destruction. This wasn't an issue when haloes were basically managed by a C library, but if we want to get rid of the static singleton system, the handles need to be smarter than just int's. --- src/display.cpp | 40 +++++++++++----------- src/halo.cpp | 51 ++++++++++++++++++++++------ src/halo.hpp | 57 ++++++++++++++++++++++++++++---- src/overlay.hpp | 8 +++-- src/unit_animation.cpp | 23 ++++--------- src/unit_animation.hpp | 5 +-- src/unit_animation_component.cpp | 8 +---- src/unit_animation_component.hpp | 6 ++-- src/unit_drawer.cpp | 8 ++--- src/unit_frame.cpp | 11 +++--- src/unit_frame.hpp | 3 +- 11 files changed, 138 insertions(+), 82 deletions(-) diff --git a/src/display.cpp b/src/display.cpp index a751c92008fb..0c2e6d5521ff 100644 --- a/src/display.cpp +++ b/src/display.cpp @@ -101,7 +101,7 @@ void display::parse_team_overlays() void display::add_overlay(const map_location& loc, const std::string& img, const std::string& halo,const std::string& team_name, bool visible_under_fog) { if (resources::halo) { - const int halo_handle = resources::halo->add(get_location_x(loc) + hex_size() / 2, + const halo::handle halo_handle = resources::halo->add(get_location_x(loc) + hex_size() / 2, get_location_y(loc) + hex_size() / 2, halo, loc); const overlay item(img, halo, halo_handle, team_name, visible_under_fog); @@ -111,38 +111,36 @@ void display::add_overlay(const map_location& loc, const std::string& img, const void display::remove_overlay(const map_location& loc) { + /* This code no longer needed because of RAII in halo::handles if (resources::halo) { - typedef overlay_map::const_iterator Itor; std::pair itors = overlays_->equal_range(loc); while(itors.first != itors.second) { resources::halo->remove(itors.first->second.halo_handle); ++itors.first; } - - overlays_->erase(loc); } + */ + + overlays_->erase(loc); } void display::remove_single_overlay(const map_location& loc, const std::string& toDelete) { - if (resources::halo) { - - //Iterate through the values with key of loc - typedef overlay_map::iterator Itor; - overlay_map::iterator iteratorCopy; - std::pair itors = overlays_->equal_range(loc); - while(itors.first != itors.second) { - //If image or halo of overlay struct matches toDelete, remove the overlay - if(itors.first->second.image == toDelete || itors.first->second.halo == toDelete) { - iteratorCopy = itors.first; - ++itors.first; - resources::halo->remove(iteratorCopy->second.halo_handle); - overlays_->erase(iteratorCopy); - } - else { - ++itors.first; - } + //Iterate through the values with key of loc + typedef overlay_map::iterator Itor; + overlay_map::iterator iteratorCopy; + std::pair itors = overlays_->equal_range(loc); + while(itors.first != itors.second) { + //If image or halo of overlay struct matches toDelete, remove the overlay + if(itors.first->second.image == toDelete || itors.first->second.halo == toDelete) { + iteratorCopy = itors.first; + ++itors.first; + //Not needed because of RAII -->resources::halo->remove(iteratorCopy->second.halo_handle); + overlays_->erase(iteratorCopy); + } + else { + ++itors.first; } } } diff --git a/src/halo.cpp b/src/halo.cpp index 3d79333fe365..461ef88fca98 100644 --- a/src/halo.cpp +++ b/src/halo.cpp @@ -28,6 +28,8 @@ namespace halo { +const int NO_HALO = 0; + class halo_impl { @@ -465,27 +467,24 @@ void halo_impl::render() manager::manager(display& screen) : impl_(new halo_impl(screen)) {} -manager::~manager() -{ - delete impl_; -} - -int manager::add(int x, int y, const std::string& image, const map_location& loc, +handle manager::add(int x, int y, const std::string& image, const map_location& loc, ORIENTATION orientation, bool infinite) { - return impl_->add(x,y,image, loc, orientation, infinite); + int new_halo = impl_->add(x,y,image, loc, orientation, infinite); + return handle(new halo_record(new_halo, impl_)); } /** Set the position of an existing haloing effect, according to its handle. */ -void manager::set_location(int handle, int x, int y) +void manager::set_location(const handle & h, int x, int y) { - impl_->set_location(handle,x,y); + impl_->set_location(h->id_,x,y); } /** Remove the halo with the given handle. */ -void manager::remove(int handle) +void manager::remove(const handle & h) { - impl_->remove(handle); + impl_->remove(h->id_); + h->id_ = NO_HALO; } /** @@ -506,4 +505,34 @@ void manager::render() // end halo::manager implementation + +/** + * halo::halo_record implementation + */ + +halo_record::halo_record() : + id_(NO_HALO), //halo::NO_HALO + my_manager_() +{} + +halo_record::halo_record(int id, const boost::shared_ptr & my_manager) : + id_(id), + my_manager_(my_manager) +{} + +halo_record::~halo_record() +{ + if (id_ == NO_HALO) return; + if (my_manager_.expired()) return; + boost::shared_ptr man = my_manager_.lock(); + + if (man) { + try { + man->remove(id_); + } catch (std::exception & e) { + std::cerr << "Caught an exception in halo::halo_record destructor: \n" << e.what() << std::endl; + } catch (...) {} + } +} + } //end namespace halo diff --git a/src/halo.hpp b/src/halo.hpp index f52ba090ed66..cff7ce1b5b08 100644 --- a/src/halo.hpp +++ b/src/halo.hpp @@ -21,19 +21,25 @@ class display; #include "map_location.hpp" +#include +#include +#include + namespace halo { class halo_impl; +class halo_record; + +typedef boost::shared_ptr handle; + enum ORIENTATION { NORMAL, HREVERSE, VREVERSE, HVREVERSE }; -const int NO_HALO = 0; class manager { public: manager(display& disp); - ~manager(); /** * Add a haloing effect using 'image centered on (x,y). @@ -44,14 +50,14 @@ class manager * shroud is active. (Note it will be shown with the fog active.) * If it is not attached to an item, the location should be set to -1, -1 */ - int add(int x, int y, const std::string& image, const map_location& loc, + handle add(int x, int y, const std::string& image, const map_location& loc, halo::ORIENTATION orientation=NORMAL, bool infinite=true); /** Set the position of an existing haloing effect, according to its handle. */ - void set_location(int handle, int x, int y); + void set_location(const handle & h, int x, int y); /** Remove the halo with the given handle. */ - void remove(int handle); + void remove(const handle & h); /** * Render and unrender haloes. @@ -63,7 +69,46 @@ class manager void render(); private: - halo_impl * impl_; + boost::shared_ptr impl_; +}; + +/** + * RAII object which manages a halo. When it goes out of scope it removes the corresponding halo entry. + */ +class halo_record : public boost::noncopyable +{ +public: + halo_record(); + halo_record(int id, const boost::shared_ptr & my_manager); + ~halo_record(); + + + friend class manager; +private: + int id_; + boost::weak_ptr my_manager_; + +//Begin safe_bool impl + +#ifndef HAVE_CXX11 + struct safe_bool_impl { void nonnull() {} }; + /** + * Used as t he return type of the conversion operator for boolean contexts. + * Needed, since the compiler would otherwise consider the following + * conversion (C legacy): cfg["abc"] -> "abc"[bool(cfg)] -> 'b' + */ + typedef void (safe_bool_impl::*safe_bool)(); +#endif + +public: +#ifdef HAVE_CXX11 + explicit operator bool() const + { return id_ != 0 && !my_manager_.expired(); } +#else + operator safe_bool() const + { return (id_ != 0 && !my_manager_.expired()) ? &safe_bool_impl::nonnull : NULL; } +#endif + }; } // end namespace halo diff --git a/src/overlay.hpp b/src/overlay.hpp index 046677cbac53..17805ddd5b6e 100644 --- a/src/overlay.hpp +++ b/src/overlay.hpp @@ -15,11 +15,13 @@ #ifndef OVERLAY_INCLUDED #define OVERLAY_INCLUDED +#include "halo.hpp" + struct overlay { overlay(const std::string& img, const std::string& halo_img, - int handle, const std::string& overlay_team_name, const bool fogged) : image(img), halo(halo_img), + halo::handle handle, const std::string& overlay_team_name, const bool fogged) : image(img), halo(halo_img), team_name(overlay_team_name), halo_handle(handle) , visible_in_fog(fogged) {} @@ -27,7 +29,7 @@ struct overlay overlay(const config& cfg) : image(cfg["image"]), halo(cfg["halo"]), team_name(cfg["team_name"]), name(cfg["name"].t_str()), id(cfg["id"]), - halo_handle(-1), visible_in_fog(cfg["visible_in_fog"].to_bool()) + halo_handle(), visible_in_fog(cfg["visible_in_fog"].to_bool()) { } @@ -37,7 +39,7 @@ struct overlay t_string name; std::string id; - int halo_handle; + halo::handle halo_handle; bool visible_in_fog; }; diff --git a/src/unit_animation.cpp b/src/unit_animation.cpp index 15adea3d2d57..f1934020d379 100644 --- a/src/unit_animation.cpp +++ b/src/unit_animation.cpp @@ -874,7 +874,7 @@ unit_animation::particule::particule( animated(), accelerate(true), parameters_(), - halo_id_(0), + halo_id_(), last_frame_begin_time_(0), cycles_(false) { @@ -1226,19 +1226,14 @@ void unit_animation::particule::redraw(const frame_parameters& value,const map_l // for sound frames we want the first time variable set only after the frame has started. if(get_current_frame_begin_time() != last_frame_begin_time_ && animation_time >= get_current_frame_begin_time()) { last_frame_begin_time_ = get_current_frame_begin_time(); - current_frame.redraw(get_current_frame_time(),true,in_scope_of_frame,src,dst,&halo_id_,default_val,value); + current_frame.redraw(get_current_frame_time(),true,in_scope_of_frame,src,dst,halo_id_,default_val,value); } else { - current_frame.redraw(get_current_frame_time(),false,in_scope_of_frame,src,dst,&halo_id_,default_val,value); + current_frame.redraw(get_current_frame_time(),false,in_scope_of_frame,src,dst,halo_id_,default_val,value); } } void unit_animation::particule::clear_halo() { - if(halo_id_ != halo::NO_HALO) { - if (resources::halo) { - resources::halo->remove(halo_id_); - } - halo_id_ = halo::NO_HALO; - } + halo_id_ = halo::handle(); // halo::NO_HALO } std::set unit_animation::particule::get_overlaped_hex(const frame_parameters& value,const map_location &src, const map_location &dst) { @@ -1250,18 +1245,12 @@ std::set unit_animation::particule::get_overlaped_hex(const frame_ unit_animation::particule::~particule() { - if (resources::halo) { - resources::halo->remove(halo_id_); - } - halo_id_ = halo::NO_HALO; + halo_id_ = halo::handle(); // halo::NO_HALO } void unit_animation::particule::start_animation(int start_time) { - if (resources::halo) { - resources::halo->remove(halo_id_); - } - halo_id_ = halo::NO_HALO; + halo_id_ = halo::handle(); // halo::NO_HALO parameters_.override(get_animation_duration()); animated::start_animation(start_time,cycles_); last_frame_begin_time_ = get_begin_time() -1; diff --git a/src/unit_animation.hpp b/src/unit_animation.hpp index 98fb4a00bbe4..678c05b98f1e 100644 --- a/src/unit_animation.hpp +++ b/src/unit_animation.hpp @@ -16,6 +16,7 @@ #include "animated.hpp" #include "config.hpp" +#include "halo.hpp" #include "unit_frame.hpp" #include "unit_ptr.hpp" @@ -89,7 +90,7 @@ class unit_animation animated(start_time), accelerate(true), parameters_(builder), - halo_id_(0), + halo_id_(), last_frame_begin_time_(0), cycles_(false) {} @@ -119,7 +120,7 @@ class unit_animation //animation params that can be locally overridden by frames frame_parsed_parameters parameters_; - int halo_id_; + halo::handle halo_id_; int last_frame_begin_time_; bool cycles_; diff --git a/src/unit_animation_component.cpp b/src/unit_animation_component.cpp index e5358f1dc295..2a6bab76c16b 100644 --- a/src/unit_animation_component.cpp +++ b/src/unit_animation_component.cpp @@ -18,7 +18,6 @@ #include "display.hpp" #include "map.hpp" #include "preferences.hpp" -#include "resources.hpp" //only for halo::manager #include "unit_animation.hpp" #include "unit.hpp" #include "unit_types.hpp" @@ -149,12 +148,7 @@ void unit_animation_component::refresh() void unit_animation_component::clear_haloes () { - if(unit_halo_ != halo::NO_HALO) { - if (resources::halo) { - resources::halo->remove(unit_halo_); - } - unit_halo_ = halo::NO_HALO; - } + unit_halo_ = halo::handle(); //halo::NO_HALO; <-- Removes it from the halo manager automatically. if(anim_ ) anim_->clear_haloes(); } diff --git a/src/unit_animation_component.hpp b/src/unit_animation_component.hpp index d5ab341d79b1..e8b446a71d99 100644 --- a/src/unit_animation_component.hpp +++ b/src/unit_animation_component.hpp @@ -46,7 +46,7 @@ class unit_animation_component frame_begin_time_(0), draw_bars_(false), refreshing_(false), - unit_halo_(halo::NO_HALO) {} + unit_halo_() {} /** Copy construct a unit animation component, for use when copy constructing a unit. */ unit_animation_component(unit & my_unit, const unit_animation_component & o) : @@ -58,7 +58,7 @@ class unit_animation_component frame_begin_time_(o.frame_begin_time_), draw_bars_(o.draw_bars_), refreshing_(o.refreshing_), - unit_halo_(halo::NO_HALO) {} + unit_halo_() {} /** Chooses an appropriate animation from the list of known animations. */ const unit_animation* choose_animation(const display& disp, @@ -123,7 +123,7 @@ class unit_animation_component bool draw_bars_; //!< bool indicating whether to draw bars with the unit bool refreshing_; //!< avoid infinite recursion. flag used for drawing / animation - int unit_halo_; //!< flag used for drawing / animation + halo::handle unit_halo_; //!< handle to the halo of this unit }; #endif diff --git a/src/unit_drawer.cpp b/src/unit_drawer.cpp index cd999b62e4b0..1447c8cea559 100644 --- a/src/unit_drawer.cpp +++ b/src/unit_drawer.cpp @@ -161,13 +161,13 @@ void unit_drawer::redraw_unit (const unit & u) const const int x = static_cast(adjusted_params.offset * xdst + (1.0-adjusted_params.offset) * xsrc) + hex_size_by_2; const int y = static_cast(adjusted_params.offset * ydst + (1.0-adjusted_params.offset) * ysrc) + hex_size_by_2; - if(ac.unit_halo_ == halo::NO_HALO && !u.image_halo().empty()) { + if(!ac.unit_halo_ && !u.image_halo().empty()) { ac.unit_halo_ = halo_man.add(0, 0, u.image_halo()+u.TC_image_mods(), map_location(-1, -1)); } - if(ac.unit_halo_ != halo::NO_HALO && u.image_halo().empty()) { + if(ac.unit_halo_ && u.image_halo().empty()) { halo_man.remove(ac.unit_halo_); - ac.unit_halo_ = halo::NO_HALO; - } else if(ac.unit_halo_ != halo::NO_HALO) { + ac.unit_halo_ = halo::handle(); //halo::NO_HALO; + } else if(ac.unit_halo_) { halo_man.set_location(ac.unit_halo_, x, y - height_adjust); } diff --git a/src/unit_frame.cpp b/src/unit_frame.cpp index b7ce1a8b7272..0bba74b79b1d 100644 --- a/src/unit_frame.cpp +++ b/src/unit_frame.cpp @@ -628,7 +628,7 @@ std::vector frame_parsed_parameters::debug_strings() const { } -void unit_frame::redraw(const int frame_time,bool on_start_time,bool in_scope_of_frame,const map_location & src,const map_location & dst,int*halo_id,const frame_parameters & animation_val,const frame_parameters & engine_val)const +void unit_frame::redraw(const int frame_time,bool on_start_time,bool in_scope_of_frame,const map_location & src,const map_location & dst,halo::handle & halo_id,const frame_parameters & animation_val,const frame_parameters & engine_val)const { const int xsrc = game_display::get_singleton()->get_location_x(src); const int ysrc = game_display::get_singleton()->get_location_y(src); @@ -698,10 +698,7 @@ void unit_frame::redraw(const int frame_time,bool on_start_time,bool in_scope_of ftofxp(current_data.highlight_ratio), current_data.blend_with, current_data.blend_ratio,current_data.submerge,!facing_north); } - if (resources::halo) { - resources::halo->remove(*halo_id); - } - *halo_id = halo::NO_HALO; + halo_id = halo::handle(); //halo::NO_HALO; if (!in_scope_of_frame) { //check after frame as first/last frame image used in defense/attack anims return; @@ -744,13 +741,13 @@ void unit_frame::redraw(const int frame_time,bool on_start_time,bool in_scope_of } if(direction != map_location::SOUTH_WEST && direction != map_location::NORTH_WEST) { - *halo_id = resources::halo->add(static_cast(x+current_data.halo_x* game_display::get_singleton()->get_zoom_factor()), + halo_id = resources::halo->add(static_cast(x+current_data.halo_x* game_display::get_singleton()->get_zoom_factor()), static_cast(y+current_data.halo_y* game_display::get_singleton()->get_zoom_factor()), current_data.halo + current_data.halo_mod, map_location(-1, -1), orientation); } else { - *halo_id = resources::halo->add(static_cast(x-current_data.halo_x* game_display::get_singleton()->get_zoom_factor()), + halo_id = resources::halo->add(static_cast(x-current_data.halo_x* game_display::get_singleton()->get_zoom_factor()), static_cast(y+current_data.halo_y* game_display::get_singleton()->get_zoom_factor()), current_data.halo + current_data.halo_mod, map_location(-1, -1), diff --git a/src/unit_frame.hpp b/src/unit_frame.hpp index 9230cd1261c1..4914347f8207 100644 --- a/src/unit_frame.hpp +++ b/src/unit_frame.hpp @@ -20,6 +20,7 @@ #ifndef UNIT_FRAME_H_INCLUDED #define UNIT_FRAME_H_INCLUDED +#include "halo.hpp" #include "image.hpp" class config; @@ -204,7 +205,7 @@ class unit_frame { public: // Constructors unit_frame(const frame_builder& builder=frame_builder()):builder_(builder){} - void redraw(const int frame_time,bool on_start_time,bool in_scope_of_frame,const map_location & src,const map_location & dst,int*halo_id,const frame_parameters & animation_val,const frame_parameters & engine_val)const; + void redraw(const int frame_time,bool on_start_time,bool in_scope_of_frame,const map_location & src,const map_location & dst,halo::handle & halo_id,const frame_parameters & animation_val,const frame_parameters & engine_val)const; const frame_parameters merge_parameters(int current_time,const frame_parameters & animation_val,const frame_parameters & engine_val=frame_parameters()) const; const frame_parameters parameters(int current_time) const {return builder_.parameters(current_time);} const frame_parameters end_parameters() const {return builder_.parameters(duration());}