Streamline KeyObserver attachment to EntityNode

- EntityNode has a new observeKey() method which adds a simple callback
  function to observe a particular key, rather than requiring a full
  KeyObserver object owned and managed by the calling code.
- KeyObserverMap now stores the KeyObservers as shared_ptrs rather than raw
  pointers. If the legacy addKeyObserver() method is called, the observer is
  wrapped in a shared_ptr with a null deleter. If the new observeKey() method
  is called, a new KeyObserver is constructed to wrap the callback function.
- EntityKeyValue::detach() has a new parameter to disable sending the final
  empty value when the observer is detached. This avoids feedback loops and
  crashes when EntityNode (and its contained KeyObserverMap) is destroyed.
- All KeyObserverDelegates are now removed from Doom3GroupNode, in favour of
  the simpler observeKey() interface.

include/ientity.h

@@ -19,6 +19,8 @@ typedef std::shared_ptr<const IEntityClass> IEntityClassConstPtr;
 class KeyObserver
 {
 public:
+    using Ptr = std::shared_ptr<KeyObserver>;
+
     virtual ~KeyObserver() {}
 
     /**
@@ -28,24 +30,40 @@ class KeyObserver
     virtual void onKeyValueChanged(const std::string& newValue) = 0;
 };
 
-class EntityKeyValue :
-    public NameObserver
+/**
+ * @brief Object representing a single keyvalue (spawnarg) on an entity.
+ *
+ * This class exists so that each spawnarg can have its own independent set of
+ * KeyObservers responding to changes in its value. For most purposes it is
+ * simpler to use Entity::Observer::onKeyChange, Entity::setKeyValue and
+ * Entity::getKeyValue to interact with key values.
+ */
+class EntityKeyValue: public NameObserver
 {
 public:
     virtual ~EntityKeyValue() {}
-    /** greebo: Retrieves the actual value of this key
-     */
+
+    /// Retrieves the actual value of this key
     virtual const std::string& get() const = 0;
 
-    /** greebo: Sets the value of this key
-     */
+    /// Sets the value of this key
     virtual void assign(const std::string& other) = 0;
 
-    /** greebo: Attaches/detaches a callback to get notified about
-     *          the key change.
-     */
+    /// Attaches a callback to get notified about the key change.
     virtual void attach(KeyObserver& observer) = 0;
-    virtual void detach(KeyObserver& observer) = 0;
+
+    /**
+     * @brief Detach the given observer from this key value.
+     *
+     * @param observer
+     * Observer to detach. No action will be taken if this observer is not
+     * already attached.
+     *
+     * @param sendEmptyValue
+     * If true (the default), the observer will be invoked with an empty value
+     * before being detached. If false, no final value will be sent.
+     */
+    virtual void detach(KeyObserver& observer, bool sendEmptyValue = true) = 0;
 };
 typedef std::shared_ptr<EntityKeyValue> EntityKeyValuePtr;
 
@@ -326,7 +344,7 @@ class IEntitySelection
     // Iterates over each selected entity, invoking the given functor
     virtual void foreachEntity(const std::function<void(Entity*)>& functor) = 0;
 
-    // Returns the key value shared by all entities in this set, or an empty string 
+    // Returns the key value shared by all entities in this set, or an empty string
     // if there is no such value.
     virtual std::string getSharedKeyValue(const std::string& key) = 0;
 };

radiantcore/entity/KeyObserverDelegate.h

@@ -1,5 +1,4 @@
-#ifndef _KEY_OBSERVER_DELEGATE_H_
-#define _KEY_OBSERVER_DELEGATE_H_
+#pragma once
 
 #include "ientity.h"
 #include <functional>
@@ -45,6 +44,4 @@ class KeyObserverDelegate :
 	{}
 };
 
-} // namespace entity
-
-#endif /* _KEY_OBSERVER_DELEGATE_H_ */
+} // namespace entity

radiantcore/entity/KeyObserverMap.h

@@ -31,17 +31,40 @@ Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
 namespace entity
 {
 
+/**
+ * @brief Map of key observers associated with a particular entity.
+ *
+ * This is used internally by EntityNode to keep track of the KeyObserver
+ * classes which are observing particular spawnargs.
+ */
 class KeyObserverMap :
 	public Entity::Observer,
     public sigc::trackable
 {
 	// A map using case-insensitive comparison
-	typedef std::multimap<std::string, KeyObserver*, string::ILess> KeyObservers;
-	KeyObservers _keyObservers;
+    typedef std::multimap<std::string, KeyObserver::Ptr, string::ILess> KeyObservers;
+    KeyObservers _keyObservers;
 
 	// The observed entity
 	SpawnArgs& _entity;
 
+    void attachObserver(const std::string& key, KeyObserver& observer)
+    {
+		// Attach immediately if the entity already has such a (non-inherited) spawnarg
+		if (EntityKeyValuePtr keyValue = _entity.getEntityKeyValue(key); keyValue) {
+			keyValue->attach(observer);
+		}
+
+		// Call the observer right now with the current keyvalue as argument
+		observer.onKeyValueChanged(_entity.getKeyValue(key));
+    }
+
+    void detachObserver(const std::string& key, KeyObserver& observer, bool sendEmptyValue)
+    {
+        if (EntityKeyValuePtr kvp = _entity.getEntityKeyValue(key); kvp)
+            kvp->detach(observer, sendEmptyValue);
+    }
+
 public:
 	KeyObserverMap(SpawnArgs& entity) :
 		_entity(entity)
@@ -52,50 +75,85 @@ class KeyObserverMap :
 
 	~KeyObserverMap()
 	{
+        // Detach each individual KeyObserver from its EntityKeyValue, to avoid
+        // dangling pointers if KeyObservers are destroyed.
+        for (auto& [key, observer]: _keyObservers)
+            detachObserver(key, *observer, false /* don't send final value change */);
+
+        // All observers are detached, clear them out
+        _keyObservers.clear();
+
+        // Remove ourselves as an Entity::Observer (onKeyInsert and onKeyErase)
 		_entity.detachObserver(this);
 	}
 
-	/**
-	 * greebo: This registers a key for observation. As soon as the key gets inserted in the
-	 * entity's spawnargs, the given observer is attached to the entity's keyvalue.
-	 */
+    /**
+     * @brief Add an observer function for the specified key.
+     *
+     * The observer function will be wrapped in a KeyObserver interface object
+     * owned and deleted by the KeyObserverMap. This allows calling code to
+     * attach a callback function without worrying about maintaining a
+     * KeyObserver object.
+     *
+     * There is currently no way to manually delete an observer function added
+     * in this way. All observer functions will be removed on destruction.
+     *
+     * @param key
+     * Key to observe.
+     *
+     * @param func
+     * Callback function to be invoked when the key value changes. It will also
+     * be invoked immediately with the key's current value, or an empty string
+     * if the key does not currently exist.
+     */
+    void observeKey(const std::string& key, KeyObserverFunc func)
+    {
+        auto iter = _keyObservers.insert(
+            {key, std::make_shared<KeyObserverDelegate>(func)}
+        );
+        assert(iter != _keyObservers.end());
+        attachObserver(key, *iter->second);
+    }
+
+    /**
+     * @brief Add an observer object for the specified key.
+     *
+     * Multiple observers can be attached to the same key. It is the calling
+     * code's responsibility to ensure that the KeyObserver exists for the
+     * lifetime of the attachment; undefined behaviour will result if a
+     * KeyObserver is destroyed while still attached to the entity.
+     *
+     * @param key
+     * Key to observe.
+     *
+     * @param observer
+     * Observer which will be invoked when the key value changes. The observer
+     * will also be invoked immediately with the key's current value, which may
+     * be an empty string if the key does not exist.
+     */
 	void insert(const std::string& key, KeyObserver& observer)
 	{
-		_keyObservers.insert(KeyObservers::value_type(key, &observer));
+        // Wrap observer in a shared_ptr with a NOP deleter, since we don't own it
+		_keyObservers.insert({key, KeyObserver::Ptr(&observer, [](KeyObserver*) {})});
 
-		// Check if the entity already has such a (non-inherited) spawnarg
-		EntityKeyValuePtr keyValue = _entity.getEntityKeyValue(key);
-
-		if (keyValue != NULL)
-		{
-			// Attach an observer to a the given KeyValue
-			keyValue->attach(observer);
-		}
-
-		// Call the observer right now with the current keyvalue as argument
-		observer.onKeyValueChanged(_entity.getKeyValue(key));
+        attachObserver(key, observer);
 	}
 
 	void erase(const std::string& key, KeyObserver& observer)
 	{
-		for (KeyObservers::iterator i = _keyObservers.find(key);
-			 i != _keyObservers.upper_bound(key) && i != _keyObservers.end();
+        const auto upperBound = _keyObservers.upper_bound(key);
+		for (auto i = _keyObservers.find(key);
+			 i != upperBound && i != _keyObservers.end();
 			 /* in-loop increment */)
 		{
-			if (i->second == &observer)
-			{
-				EntityKeyValuePtr keyValue = _entity.getEntityKeyValue(key);
-
-				if (keyValue != NULL)
-				{
-					// Detach the observer from the actual keyvalue
-					keyValue->detach(observer);
-				}
+			if (i->second.get() == &observer) {
+                // Detach the observer from the actual keyvalue, if it exists
+                detachObserver(key, observer, true /* send final empty value */);
 
+                // Order is important: first increment, then erase the non-incremented value
 				_keyObservers.erase(i++);
 			}
-			else
-			{
+			else {
 				++i;
 			}
 		}

radiantcore/entity/KeyValue.cpp

@@ -3,14 +3,14 @@
 #include <functional>
 #include "iundo.h"
 
-namespace entity 
+namespace entity
 {
 
-KeyValue::KeyValue(const std::string& value, const std::string& empty, 
+KeyValue::KeyValue(const std::string& value, const std::string& empty,
                    const std::function<void(const std::string&)>& valueChanged) :
     _value(value),
     _emptyValue(empty),
-    _undo(_value, std::bind(&KeyValue::importState, this, std::placeholders::_1), 
+    _undo(_value, std::bind(&KeyValue::importState, this, std::placeholders::_1),
         std::bind(&KeyValue::onUndoRedoOperationFinished, this), "KeyValue"),
     _valueChanged(valueChanged)
 {}
@@ -39,16 +39,16 @@ void KeyValue::attach(KeyObserver& observer)
 	observer.onKeyValueChanged(get());
 }
 
-void KeyValue::detach(KeyObserver& observer)
+void KeyValue::detach(KeyObserver& observer, bool sendEmptyValue)
 {
-	observer.onKeyValueChanged(_emptyValue);
+    // Send final empty value if requested
+    if (sendEmptyValue)
+        observer.onKeyValueChanged(_emptyValue);
 
+    // Remove the observer if present
 	auto found = std::find(_observers.begin(), _observers.end(), &observer);
-
 	if (found != _observers.end())
-    {
 		_observers.erase(found);
-	}
 }
 
 const std::string& KeyValue::get() const
@@ -81,7 +81,7 @@ void KeyValue::notify()
 	}
 }
 
-void KeyValue::importState(const std::string& string) 
+void KeyValue::importState(const std::string& string)
 {
 	// We notify our observers after the entire undo rollback is done
 	_value = string;

radiantcore/entity/KeyValue.h

@@ -10,13 +10,12 @@ namespace entity
 
 class SpawnArgs;
 
-/// \brief 
+/// \brief
 /// Represents the string value of an entity key.
 ///
 /// - Notifies observers when value changes - value changes to "" on destruction.
 /// - Provides undo support through the map's undo system.
-class KeyValue final :
-	public EntityKeyValue
+class KeyValue final: public EntityKeyValue
 {
 private:
 	typedef std::vector<KeyObserver*> KeyObservers;
@@ -40,13 +39,11 @@ class KeyValue final :
     void connectUndoSystem(IUndoSystem& undoSystem);
     void disconnectUndoSystem(IUndoSystem& undoSystem);
 
-	void attach(KeyObserver& observer);
-	void detach(KeyObserver& observer);
-
-	// Accessor method, retrieve the actual value
-	const std::string& get() const;
-
-	void assign(const std::string& other);
+    // EntityKeyValue implementation
+	void attach(KeyObserver& observer) override;
+	void detach(KeyObserver& observer, bool sendEmptyValue) override;
+	const std::string& get() const override;
+	void assign(const std::string& other) override;
 
 	void notify();
 

radiantcore/entity/doom3group/Doom3GroupNode.cpp

@@ -68,16 +68,17 @@ void Doom3GroupNode::construct()
 {
     EntityNode::construct();
 
-	_angleObserver.setCallback(std::bind(&RotationKey::angleChanged, &m_rotationKey, std::placeholders::_1));
-	_rotationObserver.setCallback(std::bind(&RotationKey::rotationChanged, &m_rotationKey, std::placeholders::_1));
-	_nameObserver.setCallback(std::bind(&Doom3GroupNode::nameChanged, this, std::placeholders::_1));
-
 	m_rotation.setIdentity();
 
+    // Observe relevant spawnarg changes
 	addKeyObserver("origin", m_originKey);
-	addKeyObserver("angle", _angleObserver);
-	addKeyObserver("rotation", _rotationObserver);
-	addKeyObserver("name", _nameObserver);
+    _keyObservers.observeKey(
+        "angle", [=](const std::string& val) { m_rotationKey.angleChanged(val); }
+    );
+    _keyObservers.observeKey(
+        "rotation", [=](const std::string& val) { m_rotationKey.rotationChanged(val); }
+    );
+    _keyObservers.observeKey("name", [=](const std::string& val) { nameChanged(val); });
 	addKeyObserver(curve_Nurbs, m_curveNURBS);
 	addKeyObserver(curve_CatmullRomSpline, m_curveCatmullRom);
 
@@ -601,9 +602,6 @@ void Doom3GroupNode::destroy()
 	modelChanged("");
 
 	removeKeyObserver("origin", m_originKey);
-	removeKeyObserver("angle", _angleObserver);
-	removeKeyObserver("rotation", _rotationObserver);
-	removeKeyObserver("name", _nameObserver);
 	removeKeyObserver(curve_Nurbs, m_curveNURBS);
 	removeKeyObserver(curve_CatmullRomSpline, m_curveCatmullRom);
 }

radiantcore/entity/doom3group/Doom3GroupNode.h

@@ -56,10 +56,6 @@ class Doom3GroupNode :
 	// brushes).
 	bool m_isModel;
 
-	KeyObserverDelegate _rotationObserver;
-	KeyObserverDelegate _angleObserver;
-	KeyObserverDelegate _nameObserver;
-
 	CurveNURBS m_curveNURBS;
 	std::size_t m_curveNURBSChanged;
 	CurveCatmullRom m_curveCatmullRom;