# Bug fixes and cleanup of entity kill code.

Put all entity cleanup code in kill(), so the JS function Kill() can
just call kill(), and made the code more correct in both cases. Also
fixed a bug with auras (a unit kept around auras that had been deleted).
Also fixed up some notifier code that was causing crashes (although that
can still happen in some cases).

Fixes #176.

This was SVN commit r4810.

source/simulation/Aura.cpp

@@ -135,6 +135,9 @@ void CAura::RemoveAll()
 	m_influenced.clear();
 }
 
+// Remove an entity from the aura, but does not remove the aura from its
+// m_aurasInfluencingMe. (Used when the entity is asking to be removed from
+// the aura and will clear its own m_aurasInfluencingMe list afterwards).
 void CAura::Remove( CEntity* ent )
 {
 	jsval rval;

source/simulation/Entity.cpp

@@ -140,6 +140,7 @@ CEntity::~CEntity()
 	entf_set(ENTF_DESTROY_NOTIFIERS);
 	for ( size_t i=0; i<m_listeners.size(); i++ )
 		m_listeners[i].m_sender->DestroyNotifier( this );
+	m_listeners.clear();
 	DestroyAllNotifiers();
 
 	CEntity* remove = this;
@@ -188,16 +189,14 @@ void CEntity::loadBase()
 	}
 
 	// Re-enter all our auras so they can take into account our new traits
-	for( AuraSet::iterator it = m_aurasInfluencingMe.begin(); it != m_aurasInfluencingMe.end(); it++ )
-	{
-		(*it)->Remove( this );
-	}
+	ExitAuras();
 
 	// Resize sectors array	
 	m_sectorValues.resize(m_base->m_sectorDivs);
 	for ( int i=0; i<m_base->m_sectorDivs; ++i )
 		m_sectorValues[i] = false;
 }
+
 void CEntity::initAuraData()
 {
 	if ( m_auras.empty() )
@@ -217,36 +216,71 @@ void CEntity::initAuraData()
 		}
 	}
 }
-void CEntity::kill()
+
+void CEntity::kill(bool keepActor)
 {
-	g_Selection.removeAll( me );
-
-	CEntity* remove = this;
-	g_FormationManager.RemoveUnit(remove);
+	if( entf_get( ENTF_DESTROYED ) )
+	{
+		return;		// We were already killed this frame
+	}
 
+	g_FormationManager.RemoveUnit(this);
+	
 	entf_set(ENTF_DESTROY_NOTIFIERS);
 	for ( size_t i=0; i<m_listeners.size(); i++ )
 		m_listeners[i].m_sender->DestroyNotifier( this );
+	m_listeners.clear();
 	DestroyAllNotifiers();
 
+	for( AuraTable::iterator it = m_auras.begin(); it != m_auras.end(); it++ )
+	{
+		it->second->RemoveAll();
+		delete it->second;
+	}
+	m_auras.clear();
+
+	ExitAuras();
+
+	clearOrders();
+
 	SAFE_DELETE(m_bounds);
 
 	m_extant = false;
 
-	entf_set(ENTF_DESTROYED);
-	g_EntityManager.m_refd[me.m_handle] = false;
-	//Shutdown(); // PT: tentatively removed - this seems to be called by ~CJSComplex, and we don't want to do it twice
+	updateCollisionPatch();
 
-	if( m_actor )
+	g_Selection.removeAll( me );
+
+	// If we have a death animation and want to keep the actor, play that animation
+	if( keepActor && m_actor && 
+		m_actor->GetRandomAnimation( "death" ) != m_actor->GetRandomAnimation( "idle" ) )
+	{
+		// Prevent "wiggling" as we try to interpolate between here and our death position (if we were moving)
+		m_graphics_position = m_position;
+		m_position_previous = m_position;
+		m_graphics_orientation = m_orientation;
+		m_orientation_previous = m_orientation;
+		updateActorTransforms();
+
+		// Play death animation and keep the actor in the game in a dead state 
+		// (TODO: remove the actor after some time through some kind of fading mechanism)
+		m_actor->SetEntitySelection( "death" );
+		m_actor->SetRandomAnimation( "death", true );
+	}
+	else
 	{
 		g_Game->GetWorld()->GetUnitManager().RemoveUnit( m_actor );
 		delete( m_actor );
 		m_actor = NULL;
 	}
 
-	updateCollisionPatch();
+	entf_set(ENTF_DESTROYED);
 
-	me = HEntity(); // will deallocate the entity, assuming nobody else has a reference to it
+	g_EntityManager.m_refd[me.m_handle] = false;
+
+	g_EntityManager.SetDeath(true);		// remember that a unit died this frame
+
+	me = HEntity(); // Will deallocate the entity, assuming nobody else has a reference to it
 }
 
 void CEntity::SetPlayer(CPlayer *pPlayer)
@@ -496,9 +530,8 @@ void CEntity::update( size_t timestep )
 	if( m_lastState != -1 )
 	{
 		PROFILE( "state transition event" );
-		CEntity* d0;
-		CVector3D d1;
-		CEventOrderTransition evt( m_lastState, -1, d0, d1 );
+		CVector3D vec(0, 0, 0);
+		CEventOrderTransition evt( m_lastState, -1, 0, vec );
 		DispatchEvent( &evt );
 
 		m_lastState = -1;
@@ -534,7 +567,7 @@ void CEntity::updateCollisionPatch()
 			}
 		}
 
-		if( m_extant )
+		if( newPatch )
 		{
 			// add ourselves to new patch
 			newPatch->push_back( this );
@@ -745,8 +778,6 @@ struct isListenerSender
 
 int CEntity::DestroyNotifier( CEntity* target )
 {
-	if ( target->m_listeners.empty() )
-		return 0;
 	//Stop listening
 	// (Don't just loop and use 'erase', because modifying the deque while
 	// looping over it is a bit dangerous)
@@ -1005,3 +1036,12 @@ void CEntity::CalculateRegen(float timestep)
 			m_staminaCurr = regen(m_staminaCurr, m_staminaMax, timestep, m_runRegenRate);
 	}
 }
+
+void CEntity::ExitAuras() 
+{
+	for( AuraSet::iterator it = m_aurasInfluencingMe.begin(); it != m_aurasInfluencingMe.end(); it++ )
+	{
+		(*it)->Remove( this );
+	}
+	m_aurasInfluencingMe.clear();
+}

source/simulation/Entity.h

@@ -268,8 +268,14 @@ public:
 	// Updates auras
 	void UpdateAuras( size_t timestep_millis );
 
+	// Exit auras we're currently in (useful for example when we change state)
+	void ExitAuras();
+
 	// Removes entity from the gameworld and deallocates it, but not necessarily immediately.
-	void kill();
+	// The keepActor parameter specifies whether to remove the unit's actor immediately (for
+	// units we want removed immediately, e.g. in Atkas) or to keep it and play the death
+	// animation (for units that die of "natural causes").
+	void kill(bool keepActor = false);
 
 	// Process initialization
 	bool Initialize();

source/simulation/EntityFormation.cpp

@@ -63,7 +63,7 @@ void CEntityFormation::SwitchBase( CFormation*& base )
 	for ( std::vector<CEntity*>::iterator it=copy.begin(); it != copy.end(); it++ )
 		g_FormationManager.AddUnit(*it, m_index);
 }
-bool CEntityFormation::AddUnit( CEntity*& entity )
+bool CEntityFormation::AddUnit( CEntity* entity )
 {
 	debug_assert( entity );
 	//Add the unit to the most appropriate slot
@@ -89,7 +89,7 @@ bool CEntityFormation::AddUnit( CEntity*& entity )
 	}
 	return false;
 }
-void CEntityFormation::RemoveUnit( CEntity*& entity )
+void CEntityFormation::RemoveUnit( CEntity* entity )
 {
 	if ( !(IsValidOrder(entity->m_formationSlot) && entity) )
 		return;

source/simulation/EntityFormation.h

@@ -58,8 +58,8 @@ private:
 	std::vector<bool> m_angleDivs;	//attack direction penalty-true=being attacked from sector
 	std::vector<float> m_angleVals;
 
-	bool AddUnit( CEntity*& entity );
-	void RemoveUnit( CEntity*& entity );
+	bool AddUnit( CEntity* entity );
+	void RemoveUnit( CEntity* entity );
 	bool IsSlotAppropriate( int order, CEntity* entity );   //If empty, can we use this slot?
 	bool IsBetterUnit( int order, CEntity* entity );
 

source/simulation/EntityManager.cpp

@@ -350,8 +350,9 @@ void CEntityManager::destroy( u16 handle )
 	m_reaper.push_back( m_entities[handle].m_entity );
 	
 	//Remove trigger-helper data
-	size_t playerID = (size_t)m_entities[m_nextalloc].m_entity->GetPlayer()->GetPlayerID();
-	CStrW className, classList = m_entities[m_nextalloc].m_entity->m_classes.getMemberList();
+	CEntity* ent = m_entities[handle].m_entity;
+	size_t playerID = (size_t)ent->GetPlayer()->GetPlayerID();
+	CStrW className, classList = ent->m_classes.getMemberList();
 
 	while ( (className = classList.BeforeFirst(L" ")) != classList )
 	{
@@ -360,7 +361,7 @@ void CEntityManager::destroy( u16 handle )
 	}
 	--m_entityClassData[playerID][className];
 
-	m_entities[handle].m_entity->me.m_handle = INVALID_HANDLE;
+	ent->me.m_handle = INVALID_HANDLE;
 }
 
 bool CEntityManager::m_extant = false;

source/simulation/EntityScriptInterface.cpp

@@ -239,10 +239,7 @@ void CEntity::JSI_SetPlayer( jsval val )
 	m_productionQueue->CancelAll();
 
 	// Exit all our auras so we can re-enter them as the new player
-	for( AuraSet::iterator it = m_aurasInfluencingMe.begin(); it != m_aurasInfluencingMe.end(); it++ )
-	{
-		(*it)->Remove( this );
-	}
+	ExitAuras();
 	
 	if( m_actor )
 		m_actor->SetPlayerID( newPlayer->GetPlayerID() ); // calls this->SetPlayer
@@ -357,58 +354,7 @@ bool CEntity::Kill( JSContext* UNUSED(cx), uintN UNUSED(argc), jsval* UNUSED(arg
 	CEventDeath evt;
 	DispatchEvent( &evt );
 
-	for( AuraTable::iterator it = m_auras.begin(); it != m_auras.end(); it++ )
-	{
-		it->second->RemoveAll();
-		delete it->second;
-	}
-	m_auras.clear();
-
-	for( AuraSet::iterator it = m_aurasInfluencingMe.begin(); it != m_aurasInfluencingMe.end(); it++ )
-	{
-		(*it)->Remove( this );
-	}
-	m_aurasInfluencingMe.clear();
-
-	if( m_bounds )
-	{
-		delete( m_bounds );
-		m_bounds = NULL;
-	}
-
-	if( m_extant )
-	{
-		m_extant = false;
-	}
-
-	updateCollisionPatch();
-
-	g_Selection.removeAll( me );
-
-	clearOrders();
-
-	g_EntityManager.SetDeath(true);
-
-	if( m_actor && m_actor->GetRandomAnimation( "death" ) != m_actor->GetRandomAnimation( "idle" ) )
-	{
-		// Prevent "wiggling" as we try to interpolate between here and our death position (if we were moving)
-		m_graphics_position = m_position;
-		m_position_previous = m_position;
-		m_graphics_orientation = m_orientation;
-		m_orientation_previous = m_orientation;
-		updateActorTransforms();
-
-		// Play death animation and keep the actor in the game in a dead state 
-		// (TODO: remove the actor after some time through some kind of fading mechanism)
-		m_actor->SetEntitySelection( "death" );
-		m_actor->SetRandomAnimation( "death", true );
-	}
-	else
-	{
-		g_Game->GetWorld()->GetUnitManager().RemoveUnit( m_actor );
-		delete( m_actor );
-		m_actor = NULL;
-	}
+	kill(true);
 
 	return( true );
 }

source/simulation/EventHandlers.cpp

@@ -88,17 +88,25 @@ CEventPrepareOrder::CEventPrepareOrder( CEntity* target, int orderType, int acti
 	AddLocalProperty( L"notifySource", &m_notifySource );
 }
 
-CEventOrderTransition::CEventOrderTransition( int orderPrevious, int orderCurrent, CEntity*& target, CVector3D& worldPosition ) 
+CEventOrderTransition::CEventOrderTransition( int orderPrevious, int orderCurrent, CEntity* target, CVector3D& worldPosition ) 
 	: CScriptEvent( L"orderTransition", EVENT_ORDER_TRANSITION, true )
 {
 	m_orderPrevious = orderPrevious;
 	m_orderCurrent = orderCurrent;
-	m_target = ⌖
-	m_worldPosition = &worldPosition;
+
+	if(target) {
+		m_target = target->me;
+	}
+	else {
+		m_target = HEntity();
+	}
+
+	m_worldPosition = worldPosition;
+
 	AddLocalProperty( L"orderPrevious", &m_orderPrevious, true );
 	AddLocalProperty( L"orderCurrent", &m_orderCurrent );
-	AddLocalProperty( L"target", m_target );
-	AddLocalProperty( L"position", m_worldPosition );
+	AddLocalProperty( L"target", &m_target );
+	AddLocalProperty( L"position", &m_worldPosition );
 }
 CEventNotification::CEventNotification( CEntityOrder order, int notifyType ) : CScriptEvent( L"notification", EVENT_NOTIFICATION, true )
 {

source/simulation/EventHandlers.h

@@ -107,10 +107,10 @@ class CEventOrderTransition : public CScriptEvent
 {
 	int m_orderPrevious;
 	int m_orderCurrent;
-	CEntity** m_target;
-	CVector3D* m_worldPosition;
+	HEntity m_target;
+	CVector3D m_worldPosition;
 public:
-	CEventOrderTransition( int orderPrevious, int orderCurrent, CEntity*& target, CVector3D& worldPosition );
+	CEventOrderTransition( int orderPrevious, int orderCurrent, CEntity* target, CVector3D& worldPosition );
 };
 class CEventNotification : public CScriptEvent
 {

source/simulation/FormationManager.cpp

@@ -65,7 +65,7 @@ void CFormationManager::DestroyFormation( size_t form )
 	m_formations.erase( it );
 	UpdateIndexes( form );
 }
-bool CFormationManager::AddUnit( CEntity*& entity, int& form )
+bool CFormationManager::AddUnit( CEntity* entity, int& form )
 {
 	if ( !IsValidFormation(form) )
 		return false;
@@ -101,7 +101,7 @@ CEntityList CFormationManager::AddUnitList( CEntityList& entities, int form )
 	}
 	return accepted;
 }
-bool CFormationManager::RemoveUnit( CEntity*& entity )
+bool CFormationManager::RemoveUnit( CEntity* entity )
 {
 	if ( !IsValidFormation(entity->m_formation) )
 		return true;

source/simulation/FormationManager.h

@@ -33,11 +33,11 @@ public:
 	{
 		return ((size_t)index < m_formations.size() && index >= 0);
 	}
-	bool AddUnit( CEntity*& entity, int& form );
+	bool AddUnit( CEntity* entity, int& form );
 	CEntityList AddUnitList( CEntityList& entities, int form );
 
 	//Returns false if the formation is destroyed
-	bool RemoveUnit( CEntity*& entity );
+	bool RemoveUnit( CEntity* entity );
 	bool RemoveUnitList( CEntityList& entities );
 	CEntityFormation* GetFormation(int form);
 	void UpdateIndexes( size_t update );