fixup halos segfaulting

fixes up commit 82c6b98

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,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<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);
 	}
+	*/
+
+	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<Itor,Itor> 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<Itor,Itor> 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;
 		}
 	}
 }

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<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 @@ 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<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,19 +15,21 @@
 #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)
 	{}
 
 
 	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) {
-		if (resources::halo) {
-			resources::halo->remove(halo_id_);
-		}
-		halo_id_ = halo::NO_HALO;
-	}
+	halo_id_ = halo::handle(); // 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) {
-		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<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) {
-		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();
 }