#5919: Fix a crash due to a problem in the destruction order of Entit…

…yNodes.

Any child nodes handled by the ModelKey member might still want to communicate with their parent entity in their destructor, so let's shutdown the ModelKey and attachment list before the EntityNode goes defunct.

radiantcore/entity/EntityNode.cpp

@@ -117,7 +117,13 @@ void EntityNode::constructClone(const EntityNode& original)
 
 void EntityNode::destruct()
 {
-	_modelKey.setActive(false); // disable callbacks during destruction
+    // Disable model key handling, remove child nodes
+	_modelKey.destroy();
+
+    // Remove any attached entities before this EntityNode is defunct
+    // Particle node attachments might want to remove their renderables from
+    // this entity on destruction.
+    _attachedEnts.clear();
 
 	_eclassChangedConn.disconnect();
 

radiantcore/entity/ModelKey.cpp

@@ -2,11 +2,8 @@
 
 #include <functional>
 #include "imodelcache.h"
-#include "ifiletypes.h"
-#include "scene/Node.h"
 #include "ifilter.h"
 #include "modelskin.h"
-#include "itransformable.h"
 #include "string/replace.h"
 #include "scenelib.h"
 
@@ -21,9 +18,13 @@ const scene::INodePtr& ModelKey::getNode() const
 	return _model.node;
 }
 
-void ModelKey::setActive(bool active)
+void ModelKey::destroy()
 {
-	_active = active;
+    detachModelNode();
+
+    _model.node.reset();
+    _model.path.clear();
+    _active = false;
 }
 
 void ModelKey::refreshModel()
@@ -39,7 +40,7 @@ void ModelKey::modelChanged(const std::string& value)
 	if (!_active) return; // deactivated during parent node destruction
 
 	// Sanitise the keyvalue - must use forward slashes
-	std::string newModelName = string::replace_all_copy(value, "\\", "/");
+	auto newModelName = string::replace_all_copy(value, "\\", "/");
 
 	if (newModelName == _model.path)
 	{
@@ -57,18 +58,11 @@ void ModelKey::modelChanged(const std::string& value)
 
 void ModelKey::attachModelNode()
 {
-	// Remove the old model node first
-	if (_model.node != NULL)
-	{
-		_parentNode.removeChildNode(_model.node);
-	}
+	// Remove the old model node first (this also clears the pointer)
+    detachModelNode();
 
-	if (_model.path.empty())
-	{
-		// Empty "model" spawnarg, clear the pointer and exit
-		_model.node = scene::INodePtr();
-		return;
-	}
+	// If the "model" spawnarg is empty, there's nothing to attach
+    if (_model.path.empty()) return;
 
 	// We have a non-empty model key, send the request to
 	// the model cache to acquire a new child node
@@ -94,6 +88,14 @@ void ModelKey::attachModelNode()
 	}
 }
 
+void ModelKey::detachModelNode()
+{
+    if (!_model.node) return; // nothing to do
+
+    _parentNode.removeChildNode(_model.node);
+    _model.node.reset();
+}
+
 void ModelKey::attachModelNodeKeepinSkin()
 {
     if (_model.node)

radiantcore/entity/ModelKey.h

@@ -33,7 +33,11 @@ class ModelKey: public sigc::trackable
 public:
 	ModelKey(scene::INode& parentNode);
 
-	void setActive(bool active);
+    // Remove any model node from the parent entity
+    // used during Entity destruction to remove any child nodes
+    // before the parent entity node is going out of business.
+    // Disables any further behaviour of this instance, it will no longer be functional
+    void destroy();
 
 	// Refreshes the attached model
 	void refreshModel();
@@ -55,6 +59,8 @@ class ModelKey: public sigc::trackable
 	// Loads the model node and attaches it to the parent node
 	void attachModelNode();
 
+    void detachModelNode();
+
     // Attaches a model node, making sure that the skin setting is kept
     void attachModelNodeKeepinSkin();