Fixes to UniqueID loading

Author: Causeless

Source/Entities/MovableObject.cpp

@@ -266,10 +266,9 @@ int MovableObject::Create(const MovableObject& reference) {
 	m_NumberValueMap = reference.m_NumberValueMap;
 	m_ObjectValueMap = reference.m_ObjectValueMap;
 
-	// If we have a -1 unique ID, then we're currently performing a game save or load
-	// In this case, we actually want to persist our existing IDs
+	// If we're currently performing a game save or load we want to persist our existing IDs
 	if (g_MovableMan.ShouldPersistUniqueIDs()) {
-		m_UniqueID = reference.m_UniqueID;
+		m_UniqueID = reference.m_UniqueID == 0 ? g_MovableMan.GetNextUniqueID() : reference.m_UniqueID;
 		m_AgeTimer = reference.m_AgeTimer;
 	} else {
 		// Otherwise we're copying from a preset normally and ought to create a new object
@@ -384,11 +383,7 @@ int MovableObject::ReadProperty(const std::string_view& propName, Reader& reader
 	MatchProperty("SimUpdatesBetweenScriptedUpdates", { reader >> m_SimUpdatesBetweenScriptedUpdates; });
 	MatchProperty("AddCustomValue", { ReadCustomValueProperty(reader); });
 	MatchProperty("ForceIntoMasterLuaState", { reader >> m_ForceIntoMasterLuaState; });
-	MatchProperty("SpecialBehaviour_SetUniqueID", {
-		long oldID = m_UniqueID;
-		reader >> m_UniqueID;
-		g_MovableMan.ReregisterObjectIfApplicable(this, oldID);
-	});
+	MatchProperty("SpecialBehaviour_SetUniqueID", { reader >> m_UniqueID; });
 
 	EndPropertyList;
 }

Source/Entities/MovableObject.h

@@ -979,6 +979,9 @@ namespace RTE {
 		/// @param newRestThreshold New rest threshold value
 		void SetRestThreshold(int newRestThreshold) { m_RestThreshold = newRestThreshold; }
 
+		/// Resets this MO's unique persistent ID. Unsafe, only for internal use!
+		void ResetUniqueID() { m_UniqueID = 0; }
+
 		/// Returns this MO's unique persistent ID
 		/// @return Returns this MO's unique persistent ID
 		long GetUniqueID() const { return m_UniqueID; }

Source/Entities/Scene.cpp

@@ -1167,12 +1167,12 @@ void Scene::SaveSceneObject(Writer& writer, const SceneObject* sceneObjectToSave
 	}
 
 	if (const MovableObject* movableObjectToSave = dynamic_cast<const MovableObject*>(sceneObjectToSave); movableObjectToSave && saveFullData) {
+		writer.NewPropertyWithValue("SpecialBehaviour_SetUniqueID", movableObjectToSave->GetUniqueID());
 		writer.NewPropertyWithValue("HUDVisible", movableObjectToSave->GetHUDVisible());
 		writer.NewPropertyWithValue("Velocity", movableObjectToSave->GetVel());
 		writer.NewPropertyWithValue("LifeTime", movableObjectToSave->GetLifetime());
 		writer.NewPropertyWithValue("Age", movableObjectToSave->GetAge());
 		writer.NewPropertyWithValue("PinStrength", movableObjectToSave->GetPinStrength());
-		writer.NewPropertyWithValue("SpecialBehaviour_SetUniqueID", movableObjectToSave->GetUniqueID());
 	}
 
 	if (const MOSprite* moSpriteToSave = dynamic_cast<const MOSprite*>(sceneObjectToSave)) {

Source/Managers/ActivityMan.cpp

@@ -128,12 +128,12 @@ bool ActivityMan::SaveCurrentGame(const std::string& fileName) {
 		}
 	}
 
+	writer->NewPropertyWithValue("MaxUniqueID", currentMaxID);
+	writer->NewPropertyWithValue("CurrentSimTicks", g_TimerMan.GetSimTickCount());
 	writer->NewPropertyWithValue("OriginalScenePresetName", scene->GetPresetName());
 	writer->NewPropertyWithValue("PlaceObjectsIfSceneIsRestarted", g_SceneMan.GetPlaceObjectsOnLoad());
 	writer->NewPropertyWithValue("PlaceUnitsIfSceneIsRestarted", g_SceneMan.GetPlaceUnitsOnLoad());
 	writer->NewPropertyWithValue("Scene", modifiableScene.get());
-	writer->NewPropertyWithValue("MaxUniqueID", currentMaxID);
-	writer->NewPropertyWithValue("CurrentSimTicks", g_TimerMan.GetSimTickCount());
 
 	auto saveWriterData = [](Writer* writerToSave) {
 		writerToSave->EndWrite();
@@ -170,14 +170,22 @@ bool ActivityMan::LoadAndLaunchGame(const std::string& fileName) {
 	std::unique_ptr<Scene> scene(std::make_unique<Scene>());
 	std::unique_ptr<GAScripted> activity(std::make_unique<GAScripted>());
 
-	long maxUniqueID = 0;
+	g_MovableMan.ClearKnownObjects();
+
+	g_MovableMan.SetMaxUniqueID(std::numeric_limits<long>::min());
+
+	long maxUniqueID = 1;
 	long long simTimeTicks = 0;
 	std::string originalScenePresetName = fileName;
 	bool placeObjectsIfSceneIsRestarted = true;
 	bool placeUnitsIfSceneIsRestarted = true;
 	while (reader.NextProperty()) {
 		std::string propName = reader.ReadPropName();
-		if (propName == "Activity") {
+		if (propName == "MaxUniqueID") {
+			reader >> maxUniqueID;
+		} else if (propName == "CurrentSimTicks") {
+			reader >> simTimeTicks;
+		} else if (propName == "Activity") {
 			reader >> activity.get();
 		} else if (propName == "OriginalScenePresetName") {
 			reader >> originalScenePresetName;
@@ -187,14 +195,11 @@ bool ActivityMan::LoadAndLaunchGame(const std::string& fileName) {
 			reader >> placeUnitsIfSceneIsRestarted;
 		} else if (propName == "Scene") {
 			reader >> scene.get();
-		} else if (propName == "MaxUniqueID") {
-			reader >> maxUniqueID;
-			g_MovableMan.SetShouldPersistUniqueIDs(true);
-		} else if (propName == "CurrentSimTicks") {
-			reader >> simTimeTicks;
 		}
 	}
 
+	g_MovableMan.SetShouldPersistUniqueIDs(true);
+
 	// SetSceneToLoad() doesn't Clone(), but when the Activity starts, it will eventually call LoadScene(), which does a Clone() of scene internally.
 	g_SceneMan.SetSceneToLoad(scene.get(), true, true);
 	// Saved Scenes get their presetname set to their filename to ensure they're separate from the preset Scene they're based off of.
@@ -203,18 +208,18 @@ bool ActivityMan::LoadAndLaunchGame(const std::string& fileName) {
 	// For starting Activity, we need to directly clone the Activity we want to start.
 	StartActivity(dynamic_cast<GAScripted*>(activity->Clone()));
 
+	g_MovableMan.SetShouldPersistUniqueIDs(false);
+
 	// Set the max unique ID to our loaded maximum so we don't stomp over any existing ones
-	if (maxUniqueID != 0) {
-		g_MovableMan.SetMaxUniqueID(maxUniqueID);
-	}
+	g_MovableMan.SetMaxUniqueID(maxUniqueID);
 
-	g_MovableMan.SetShouldPersistUniqueIDs(false);
 	//g_TimerMan.SetSimTickCount(simTimeTicks); // Don't do for now... causes issues with negative times in places
 
 	// When this method exits, our Scene object will be destroyed, which will cause problems if you try to restart it. To avoid this, set the Scene to load to the preset object with the same name.
 	g_SceneMan.SetSceneToLoad(originalScenePresetName, placeObjectsIfSceneIsRestarted, placeUnitsIfSceneIsRestarted);
 
 	g_ConsoleMan.PrintString("SYSTEM: Game \"" + fileName + "\" loaded!");
+
 	return true;
 }
 

Source/Managers/MovableMan.cpp

@@ -166,7 +166,7 @@ MOID MovableMan::GetMOIDPixel(int pixelX, int pixelY, const std::vector<int>& mo
 }
 
 void MovableMan::RegisterObject(MovableObject* mo) {
-	if (!mo) {
+	if (!mo || mo->GetUniqueID() <= 0) {
 		return;
 	}
 
@@ -183,15 +183,12 @@ void MovableMan::UnregisterObject(MovableObject* mo) {
 	m_KnownObjects.erase(mo->GetUniqueID());
 }
 
-void MovableMan::ReregisterObjectIfApplicable(MovableObject* mo, long oldUniqueId) {
-	if (mo == nullptr || m_KnownObjects.find(oldUniqueId) == m_KnownObjects.end()) {
-		// Old ID was never registered, don't need to do anything
-		return;
+void RTE::MovableMan::ClearKnownObjects() {
+	for (auto& pair : m_KnownObjects) {
+		pair.second->ResetUniqueID();
 	}
 
-	std::lock_guard<std::mutex> guard(m_ObjectRegisteredMutex);
-	m_KnownObjects[oldUniqueId] = nullptr;
-	m_KnownObjects[mo->GetUniqueID()] = mo;
+	m_KnownObjects.clear();
 }
 
 const std::vector<MovableObject*>* MovableMan::GetMOsInBox(const Box& box, int ignoreTeam, bool getsHitByMOsOnly) const {

Source/Managers/MovableMan.h

@@ -482,19 +482,9 @@ namespace RTE {
 		/// @param mo MO to remove.
 		void UnregisterObject(MovableObject* mo);
 
-		/// Reregisters an object in a global Map collection to handle if it's unique ID changes
-		/// @param mo MO to reregister if necessary.
-		/// @param oldUniqueID The MO's old UniqueID.
-		void ReregisterObjectIfApplicable(MovableObject* mo, long oldUniqueId);
-
 		/// Returns the next unique id for MO's and increments unique ID counter
 		/// @return Returns the next unique id.
-		long GetNextUniqueID() {
-			if (m_ShouldPersistUniqueIDs) {
-				return 0;
-			}
-			return ++m_UniqueIDCounter;
-		}
+		long GetNextUniqueID() { return ++m_UniqueIDCounter; }
 
 		/// Returns the max unique id for MO's
 		/// @return Returns the current max unique id.
@@ -512,6 +502,9 @@ namespace RTE {
 		/// @param newValue Whether we should be persisting uniqueIDs.
 		void SetShouldPersistUniqueIDs(bool newValue) { m_ShouldPersistUniqueIDs = newValue; }
 
+		/// Clears all known objects. Should only be used for internal usage, it's unsafe!
+		void ClearKnownObjects();
+
 		/// Uses a global lookup map to find an object by it's unique id.
 		/// @param id Unique Id to look for.
 		/// @return Object found or 0 if not found any.