Hotloading fix after 76acc4e146

Emplace does not replace existing element, insert_or_assign does.

While at it:
 - Clean up the 'outdated' logic and reuse it for actors
 - When an actor fails to load, return a placeholder. This improves
hotloading of broken actors, and makes Cunit behaviour more predictable.
 - Some minor cleanup

Reported by: Stan
Fixes #6157

Differential Revision: https://code.wildfiregames.com/D3882
This was SVN commit r25308.

source/graphics/ObjectBase.cpp

@@ -404,10 +404,9 @@ std::vector<u8> CObjectBase::CalculateVariationKey(const std::vector<const std::
 	// Load each prop, and add their CalculateVariationKey to our key:
 	for (std::multimap<CStr, CStrW>::iterator it = chosenProps.begin(); it != chosenProps.end(); ++it)
 	{
-		CActorDef* prop = m_ObjectManager.FindActorDef(it->second);
-		if (prop)
+		if (auto [success, prop] = m_ObjectManager.FindActorDef(it->second); success)
 		{
-			std::vector<u8> propChoices = prop->GetBase(m_QualityLevel)->CalculateVariationKey(selections);
+			std::vector<u8> propChoices = prop.GetBase(m_QualityLevel)->CalculateVariationKey(selections);
 			choices.insert(choices.end(), propChoices.begin(), propChoices.end());
 		}
 	}
@@ -609,19 +608,18 @@ std::set<CStr> CObjectBase::CalculateRandomRemainingSelections(rng_t& rng, const
 	// Load each prop, and add their required selections to ours:
 	for (std::multimap<CStr, CStrW>::iterator it = chosenProps.begin(); it != chosenProps.end(); ++it)
 	{
-		CActorDef* prop = m_ObjectManager.FindActorDef(it->second);
-		if (prop)
+		if (auto [success, prop] = m_ObjectManager.FindActorDef(it->second); success)
 		{
 			std::vector<std::set<CStr> > propInitialSelections = initialSelections;
 			if (!remainingSelections.empty())
 				propInitialSelections.push_back(remainingSelections);
 
-			std::set<CStr> propRemainingSelections = prop->GetBase(m_QualityLevel)->CalculateRandomRemainingSelections(rng, propInitialSelections);
+			std::set<CStr> propRemainingSelections = prop.GetBase(m_QualityLevel)->CalculateRandomRemainingSelections(rng, propInitialSelections);
 			remainingSelections.insert(propRemainingSelections.begin(), propRemainingSelections.end());
 
 			// Add the prop's used files to our own (recursively) so we can hotload
 			// when any prop is changed
-			m_ActorDef.m_UsedFiles.insert(prop->m_UsedFiles.begin(), prop->m_UsedFiles.end());
+			m_ActorDef.m_UsedFiles.insert(prop.m_UsedFiles.begin(), prop.m_UsedFiles.end());
 		}
 	}
 
@@ -683,14 +681,9 @@ std::vector<std::vector<CStr> > CObjectBase::GetVariantGroups() const
 			{
 				const std::vector<Prop>& props = obj->m_VariantGroups[i][j].m_Props;
 				for (size_t k = 0; k < props.size(); ++k)
-				{
-					if (! props[k].m_ModelName.empty())
-					{
-						CActorDef* prop = m_ObjectManager.FindActorDef(props[k].m_ModelName.c_str());
-						if (prop)
-							objectsQueue.push(prop->GetBase(m_QualityLevel).get());
-					}
-				}
+					if (!props[k].m_ModelName.empty())
+						if (auto [success, prop] = m_ObjectManager.FindActorDef(props[k].m_ModelName.c_str()); success)
+							objectsQueue.push(prop.GetBase(m_QualityLevel).get());
 			}
 		}
 	}
@@ -712,11 +705,11 @@ void CObjectBase::GetQualitySplits(std::vector<u8>& splits) const
 				if (prop.m_ModelName.empty())
 					continue;
 
-				CActorDef* propActor = m_ObjectManager.FindActorDef(prop.m_ModelName.c_str());
-				if (!propActor)
+				auto [success, propActor] = m_ObjectManager.FindActorDef(prop.m_ModelName.c_str());
+				if (!success)
 					continue;
 
-				std::vector<u8> newSplits = propActor->QualityLevels();
+				std::vector<u8> newSplits = propActor.QualityLevels();
 				if (newSplits.size() <= 1)
 					continue;
 
@@ -951,12 +944,16 @@ bool CActorDef::Load(const VfsPath& pathname)
 	return true;
 }
 
-bool CActorDef::Reload()
-{
-	return Load(m_Pathname);
-}
-
 bool CActorDef::UsesFile(const VfsPath& pathname) const
 {
 	return m_UsedFiles.find(pathname) != m_UsedFiles.end();
 }
+
+void CActorDef::LoadErrorPlaceholder(const VfsPath& pathname)
+{
+	m_UsedFiles.clear();
+	m_ObjectBases.clear();
+	m_UsedFiles.emplace(pathname);
+	m_Pathname = pathname;
+	m_ObjectBases.emplace_back(std::make_shared<CObjectBase>(m_ObjectManager, *this, MAX_QUALITY));
+}

source/graphics/ObjectBase.h

@@ -243,10 +243,10 @@ protected:
 	bool Load(const VfsPath& pathname);
 
 	/**
-	 * Reload this object from the file that it was previously loaded from.
-	 * Returns false on error.
+	 * Initialise this object with a default placeholder actor,
+	 * pretending to be the actor at pathname.
 	 */
-	bool Reload();
+	void LoadErrorPlaceholder(const VfsPath& pathname);
 
 	/**
 	 * Returns whether this actor (including any possible props)

source/graphics/ObjectEntry.cpp

@@ -41,7 +41,7 @@
 #include <sstream>
 
 CObjectEntry::CObjectEntry(const std::shared_ptr<CObjectBase>& base, CSimulation2& simulation) :
-	m_Base(base), m_Color(1.0f, 1.0f, 1.0f, 1.0f), m_Model(NULL), m_Outdated(false), m_Simulation(simulation)
+	m_Base(base), m_Color(1.0f, 1.0f, 1.0f, 1.0f), m_Model(NULL), m_Simulation(simulation)
 {
 }
 
@@ -217,8 +217,8 @@ bool CObjectEntry::BuildVariation(const std::vector<const std::set<CStr>*>& comp
 		}
 
 		CObjectEntry* oe = nullptr;
-		if (CActorDef* actorDef = objectManager.FindActorDef(prop.m_ModelName.c_str()); actorDef)
-			oe = objectManager.FindObjectVariation(actorDef->GetBase(m_Base->m_QualityLevel), completeSelections);
+		if (auto [success, actorDef] = objectManager.FindActorDef(prop.m_ModelName.c_str()); success)
+			oe = objectManager.FindObjectVariation(actorDef.GetBase(m_Base->m_QualityLevel), completeSelections);
 
 		if (!oe)
 		{

source/graphics/ObjectEntry.h

@@ -80,10 +80,6 @@ public:
 	// corresponding model
 	CModelAbstract* m_Model;
 
-	// Whether this object is outdated, due to hotloading of its base object.
-	// (If true then CObjectManager won't reuse this object from its cache.)
-	bool m_Outdated;
-
 private:
 
 	CSimulation2& m_Simulation;

source/graphics/ObjectManager.cpp

@@ -66,24 +66,30 @@ CObjectManager::~CObjectManager()
 	UnregisterFileReloadFunc(ReloadChangedFileCB, this);
 }
 
-CActorDef* CObjectManager::FindActorDef(const CStrW& actorName)
+std::pair<bool, CActorDef&> CObjectManager::FindActorDef(const CStrW& actorName)
 {
 	ENSURE(!actorName.empty());
 
 	decltype(m_ActorDefs)::iterator it = m_ActorDefs.find(actorName);
-	if (it != m_ActorDefs.end())
-		return it->second.get();
+	if (it != m_ActorDefs.end() && !it->second.outdated)
+		return { true, *it->second.obj };
 
 	std::unique_ptr<CActorDef> actor = std::make_unique<CActorDef>(*this);
 
 	VfsPath pathname = VfsPath("art/actors/") / actorName;
 
-	if (actor->Load(pathname))
-		return m_ActorDefs.emplace(actorName, std::move(actor)).first->second.get();
+	bool success = true;
+	if (!actor->Load(pathname))
+	{
+		// In case of failure, load a placeholder - we want to have an actor around for hotloading.
+		// (this will leave garbage actors in the object manager if loading files with typos in the name,
+		// but that's unlikely to be a large memory problem).
+		LOGERROR("CObjectManager::FindActorDef(): Cannot find actor '%s'", utf8_from_wstring(actorName));
+		actor->LoadErrorPlaceholder(pathname);
+		success = false;
+	}
 
-	LOGERROR("CObjectManager::FindActorDef(): Cannot find actor '%s'", utf8_from_wstring(actorName));
-
-	return nullptr;
+	return { success, *m_ActorDefs.insert_or_assign(actorName, std::move(actor)).first->second.obj };
 }
 
 CObjectEntry* CObjectManager::FindObjectVariation(const CActorDef* actor, const std::vector<std::set<CStr>>& selections, uint32_t seed)
@@ -114,8 +120,8 @@ CObjectEntry* CObjectManager::FindObjectVariation(const std::shared_ptr<CObjectB
 	std::vector<u8> choices = base->CalculateVariationKey(completeSelections);
 	ObjectKey key (base->GetIdentifier(), choices);
 	decltype(m_Objects)::iterator it = m_Objects.find(key);
-	if (it != m_Objects.end() && !it->second->m_Outdated)
-		return it->second.get();
+	if (it != m_Objects.end() && !it->second.outdated)
+		return it->second.obj.get();
 
 	// If it hasn't been loaded, load it now.
 
@@ -128,7 +134,7 @@ CObjectEntry* CObjectManager::FindObjectVariation(const std::shared_ptr<CObjectB
 	if (!obj->BuildVariation(completeSelections, choices, *this))
 		return nullptr;
 
-	return m_Objects.emplace(key, std::move(obj)).first->second.get();
+	return m_Objects.insert_or_assign(key, std::move(obj)).first->second.obj.get();
 }
 
 CTerrain* CObjectManager::GetTerrain()
@@ -148,25 +154,24 @@ void CObjectManager::UnloadObjects()
 Status CObjectManager::ReloadChangedFile(const VfsPath& path)
 {
 	// Mark old entries as outdated so we don't reload them from the cache
-	for (std::map<ObjectKey, std::unique_ptr<CObjectEntry>>::iterator it = m_Objects.begin(); it != m_Objects.end(); ++it)
-		if (it->second->m_Base->UsesFile(path))
-			it->second->m_Outdated = true;
+	for (std::pair<const ObjectKey, Hotloadable<CObjectEntry>>& object : m_Objects)
+		if (!object.second.outdated && object.second.obj->m_Base->UsesFile(path))
+			object.second.outdated = true;
 
 	const CSimulation2::InterfaceListUnordered& cmps = m_Simulation.GetEntitiesWithInterfaceUnordered(IID_Visual);
 
 	// Reload actors that use a changed object
-	for (std::unordered_map<CStrW, std::unique_ptr<CActorDef>>::iterator it = m_ActorDefs.begin(); it != m_ActorDefs.end(); ++it)
+	for (std::pair<const CStrW, Hotloadable<CActorDef>>& actor : m_ActorDefs)
 	{
-		if (!it->second->UsesFile(path))
-			continue;
-		it->second->Reload();
+		if (!actor.second.outdated && actor.second.obj->UsesFile(path))
+			actor.second.outdated = true;
 
 		// Slightly ugly hack: The graphics system doesn't preserve enough information to regenerate the
 		// object with all correct variations, and we don't want to waste space storing it just for the
 		// rare occurrence of hotloading, so we'll tell the component (which does preserve the information)
 		// to do the reloading itself
 		for (CSimulation2::InterfaceListUnordered::const_iterator eit = cmps.begin(); eit != cmps.end(); ++eit)
-			static_cast<ICmpVisual*>(eit->second)->Hotload(it->first);
+			static_cast<ICmpVisual*>(eit->second)->Hotload(actor.first);
 	}
 	return INFO::OK;
 }

source/graphics/ObjectManager.h

@@ -68,7 +68,12 @@ public:
 
 	void UnloadObjects();
 
-	CActorDef* FindActorDef(const CStrW& actorName);
+	/**
+	 * Get the actor definition for the given path name.
+	 * If the actor cannot be loaded, this will return a placeholder actor.
+	 * @return Success/failure boolean and a valid actor definition.
+	 */
+	std::pair<bool, CActorDef&> FindActorDef(const CStrW& actorName);
 
 	/**
 	 * Get the object entry for a given actor & the given selections list.
@@ -97,7 +102,6 @@ public:
 	 */
 	Status ReloadChangedFile(const VfsPath& path);
 
-
 	/**
 	 * Reload actors that have a quality setting. Used when changing the actor quality.
 	 */
@@ -110,9 +114,17 @@ public:
 	u8 m_QualityLevel = 100;
 	std::unique_ptr<CConfigDBHook> m_QualityHook;
 
+	template<typename T>
+	struct Hotloadable
+	{
+		Hotloadable() = default;
+		Hotloadable(std::unique_ptr<T>&& ptr) : obj(std::move(ptr)) {}
+		bool outdated = false;
+		std::unique_ptr<T> obj;
+	};
 	// TODO: define a hash and switch to unordered_map
-	std::map<ObjectKey, std::unique_ptr<CObjectEntry>> m_Objects;
-	std::unordered_map<CStrW, std::unique_ptr<CActorDef>> m_ActorDefs;
+	std::map<ObjectKey, Hotloadable<CObjectEntry>> m_Objects;
+	std::unordered_map<CStrW, Hotloadable<CActorDef>> m_ActorDefs;
 };
 
 #endif

source/graphics/Unit.cpp

@@ -41,12 +41,9 @@ CUnit::~CUnit()
 
 CUnit* CUnit::Create(const CStrW& actorName, uint32_t seed, const std::set<CStr>& selections, CObjectManager& objectManager)
 {
-	CActorDef* actor = objectManager.FindActorDef(actorName);
+	auto [success, actor] = objectManager.FindActorDef(actorName);
 
-	if (!actor)
-		return nullptr;
-
-	CUnit* unit = new CUnit(objectManager, *actor, seed);
+	CUnit* unit = new CUnit(objectManager, actor, seed);
 	unit->SetActorSelections(selections); // Calls ReloadObject().
 	if (!unit->m_Model)
 	{

source/simulation2/components/CCmpVisualActor.cpp

@@ -753,6 +753,18 @@ void CCmpVisualActor::ReloadActor()
 
 	InitModel();
 
+	if (!m_Unit)
+	{
+		if (m_ModelTag.valid())
+		{
+			CmpPtr<ICmpUnitRenderer> cmpModelRenderer(GetSystemEntity());
+			if (cmpModelRenderer)
+				cmpModelRenderer->RemoveUnit(m_ModelTag);
+			m_ModelTag = ICmpUnitRenderer::tag_t{};
+		}
+		return;
+	}
+
 	ReloadUnitAnimation();
 
 	m_Unit->GetModel().SetShadingColor(shading);