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

@@ -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,23 +111,22 @@ 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<Itor,Itor> itors = overlays_->equal_range(loc);
 		while(itors.first != itors.second) {
 			resources::halo->remove(itors.first->second.halo_handle);
 			++itors.first;
 		}
+	}
+	*/
 
 	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;
@@ -137,7 +136,7 @@ void display::remove_single_overlay(const map_location& loc, const std::string&
 		if(itors.first->second.image == toDelete || itors.first->second.halo == toDelete) {
 			iteratorCopy = itors.first;
 			++itors.first;
-				resources::halo->remove(iteratorCopy->second.halo_handle);
+			//Not needed because of RAII -->resources::halo->remove(iteratorCopy->second.halo_handle);
 			overlays_->erase(iteratorCopy);
 		}
 		else {
@@ -145,7 +144,6 @@ void display::remove_single_overlay(const map_location& loc, const std::string&
 		}
 	}
 }
-}
 
 
 

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()
+handle manager::add(int x, int y, const std::string& image, const map_location& loc,
-{
-	delete impl_;
-}
-
-int 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<halo_impl> & 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<halo_impl> 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

src/halo.hpp

@@ -21,19 +21,25 @@ class display;
 
 #include "map_location.hpp"
 
+#include <boost/noncopyable.hpp>
+#include <boost/shared_ptr.hpp>
+#include <boost/weak_ptr.hpp>
+
 namespace halo
 {
 
 class halo_impl;
 
+class halo_record;
+
+typedef boost::shared_ptr<halo_record> 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 @@ public:
 	 * 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 @@ public:
 	void render();
 
 private:
-	halo_impl * impl_;
+	boost::shared_ptr<halo_impl> 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<halo_impl> & my_manager);
+	~halo_record();
+
+
+	friend class manager;
+private:
+	int id_;
+	boost::weak_ptr<halo_impl> 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

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;
 
 };

src/unit_animation.cpp

@@ -874,7 +874,7 @@ unit_animation::particule::particule(
 		animated<unit_frame>(),
 		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) {
+	halo_id_ = halo::handle(); // halo::NO_HALO
-		if (resources::halo) {
-			resources::halo->remove(halo_id_);
-		}
-		halo_id_ = halo::NO_HALO;
-	}
 }
 std::set<map_location> unit_animation::particule::get_overlaped_hex(const frame_parameters& value,const map_location &src, const map_location &dst)
 {
@@ -1250,18 +1245,12 @@ std::set<map_location> unit_animation::particule::get_overlaped_hex(const frame_
 
 unit_animation::particule::~particule()
 {
-	if (resources::halo) {
+	halo_id_ = halo::handle(); // halo::NO_HALO
-		resources::halo->remove(halo_id_);
-	}
-	halo_id_ = halo::NO_HALO;
 }
 
 void unit_animation::particule::start_animation(int start_time)
 {
-	if (resources::halo) {
+	halo_id_ = halo::handle(); // halo::NO_HALO
-		resources::halo->remove(halo_id_);
-	}
-	halo_id_ = halo::NO_HALO;
 	parameters_.override(get_animation_duration());
 	animated<unit_frame>::start_animation(start_time,cycles_);
 	last_frame_begin_time_ = get_begin_time() -1;

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<unit_frame>(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_;
 

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) {
+	unit_halo_ = halo::handle(); //halo::NO_HALO; <-- Removes it from the halo manager automatically.
-		if (resources::halo) {
-			resources::halo->remove(unit_halo_);
-		}
-		unit_halo_ = halo::NO_HALO;
-	}
 	if(anim_ ) anim_->clear_haloes();
 }
 

src/unit_animation_component.hpp

@@ -46,7 +46,7 @@ public:
 		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 @@ public:
 		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 @@ private:
 	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

src/unit_drawer.cpp

@@ -161,13 +161,13 @@ void unit_drawer::redraw_unit (const unit & u) const
 	const int x = static_cast<int>(adjusted_params.offset * xdst + (1.0-adjusted_params.offset) * xsrc) + hex_size_by_2;
 	const int y = static_cast<int>(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;
+		ac.unit_halo_ = halo::handle(); //halo::NO_HALO;
-	} else if(ac.unit_halo_ != halo::NO_HALO) {
+	} else if(ac.unit_halo_) {
 		halo_man.set_location(ac.unit_halo_, x, y - height_adjust);
 	}
 

src/unit_frame.cpp

@@ -628,7 +628,7 @@ std::vector<std::string> 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) {
+	halo_id = halo::handle(); //halo::NO_HALO;
-		resources::halo->remove(*halo_id);
-	}
-	*halo_id = 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<int>(x+current_data.halo_x* game_display::get_singleton()->get_zoom_factor()),
+			halo_id = resources::halo->add(static_cast<int>(x+current_data.halo_x* game_display::get_singleton()->get_zoom_factor()),
 					static_cast<int>(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<int>(x-current_data.halo_x* game_display::get_singleton()->get_zoom_factor()),
+			halo_id = resources::halo->add(static_cast<int>(x-current_data.halo_x* game_display::get_singleton()->get_zoom_factor()),
 					static_cast<int>(y+current_data.halo_y* game_display::get_singleton()->get_zoom_factor()),
 					current_data.halo + current_data.halo_mod,
 					map_location(-1, -1),

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());}