From 102e251fda6bb675b1e13612ba84f081ef99e5d0 Mon Sep 17 00:00:00 2001
From: biwa <6475593+biwa@users.noreply.github.com>
Date: Thu, 3 Jun 2021 12:02:51 +0200
Subject: [PATCH] Improved solution for issue #573 to not destroy the undo
 history when no player start is present

---
 Source/Core/Editing/ClassicMode.cs | 17 +++++++++++------
 Source/Core/Map/MapSet.cs          | 20 ++++++++++++++++++++
 Source/Core/Map/Thing.cs           | 10 +++++++---
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/Source/Core/Editing/ClassicMode.cs b/Source/Core/Editing/ClassicMode.cs
index e6b59318..97217c3e 100755
--- a/Source/Core/Editing/ClassicMode.cs
+++ b/Source/Core/Editing/ClassicMode.cs
@@ -704,6 +704,8 @@ namespace CodeImp.DoomBuilder.Editing
 		{
 			if(testFromCurrentPosition) 
 			{
+				bool oldignorepropchanges = General.Map.UndoRedo.IgnorePropChanges;
+
 				if (!mouseinside)
 				{
 					General.MainWindow.DisplayStatus(StatusType.Warning, "Can't test from current position: mouse is outside editing window!");
@@ -753,15 +755,16 @@ namespace CodeImp.DoomBuilder.Editing
 
 				if(start == null) // biwa. If there's no existing valid player start create one
 				{
-					// Create an undo snapshot that can will be revoked in OnMapTestEnd to remove the temporary player start
-					// This has to be done because just creating and deleting the thing will screw with the undo/redo. See https://github.com/jewalky/UltimateDoomBuilder/issues/573
-					General.Map.UndoRedo.CreateUndo("Create temporary player thing");
-
 					playerStartIsTempThing = true;
-					start = General.Map.Map.CreateThing();
+
+					// We have to circumvent the undo manager when creating the temporary player start, otherwise undoing
+					// stuff will crash: https://github.com/jewalky/UltimateDoomBuilder/issues/573
+					start = General.Map.Map.CreateTempThing();
 
 					if (start != null)
 					{
+						// We have to ignore property changes, otherwise the undo manager will try to record the changes
+						General.Map.UndoRedo.IgnorePropChanges = true;
 						General.Settings.ApplyDefaultThingSettings(start);
 						start.Type = 1;
 					}
@@ -782,6 +785,8 @@ namespace CodeImp.DoomBuilder.Editing
 
 				//everything should be valid, let's move player start here
 				start.Move(new Vector3D(mousemappos.x, mousemappos.y, s.FloorHeight));
+
+				General.Map.UndoRedo.IgnorePropChanges = oldignorepropchanges;
 			}
 
 			return true;
@@ -793,7 +798,7 @@ namespace CodeImp.DoomBuilder.Editing
 			{
 				if (playerStartIsTempThing) // biwa
 				{
-					General.Map.UndoRedo.WithdrawUndo();
+					General.Map.Map.RemoveThing(playerStart.Index);
 				}
 				else
 				{
diff --git a/Source/Core/Map/MapSet.cs b/Source/Core/Map/MapSet.cs
index 661a952d..1b37ec71 100755
--- a/Source/Core/Map/MapSet.cs
+++ b/Source/Core/Map/MapSet.cs
@@ -678,6 +678,26 @@ namespace CodeImp.DoomBuilder.Map
 			return t;
 		}
 
+		/// <summary>
+		/// This creates a temporary thing. The difference to normal thing creation is that the creation will not be recorded by the undo manager.
+		/// This should only be used in very specific cases, like creating a temporary player start for the "test from current position" action
+		/// </summary>
+		/// <returns>The new thing</returns>
+		internal Thing CreateTempThing()
+		{
+			if (numthings == General.Map.FormatInterface.MaxThings)
+			{
+				General.Interface.DisplayStatus(StatusType.Warning, "Failed to complete operation: maximum number of things reached.");
+				return null;
+			}
+
+			// Make the thing
+			Thing t = new Thing(this, numthings, false);
+			AddItem(t, ref things, numthings, ref numthings);
+			return t;
+
+		}
+
 		// This increases the size of the array to add an item
 		private void AddItem<T>(T item, ref T[] array, int index, ref int counter) where T: MapElement
 		{
diff --git a/Source/Core/Map/Thing.cs b/Source/Core/Map/Thing.cs
index fd37bf52..ecd8c524 100755
--- a/Source/Core/Map/Thing.cs
+++ b/Source/Core/Map/Thing.cs
@@ -86,6 +86,9 @@ namespace CodeImp.DoomBuilder.Map
 		private bool fixedsize;
 		private bool directional; //mxd. If true, we need to render an arrow
 
+		// biwa. This should only ever be used for temporary player starts for the "test from current position" action
+		private bool recordundo;
+
         // Rendering
         private int lastProcessed;
 
@@ -126,7 +129,7 @@ namespace CodeImp.DoomBuilder.Map
         #region ================== Constructor / Disposer
 
         // Constructor
-        internal Thing(MapSet map, int listindex)
+        internal Thing(MapSet map, int listindex, bool recordundo = true)
 		{
 			// Initialize
 			this.elementtype = MapElementType.THING; //mxd
@@ -137,8 +140,9 @@ namespace CodeImp.DoomBuilder.Map
 			this.scaleX = 1.0f;
 			this.scaleY = 1.0f;
 			this.spritescale = new SizeF(1.0f, 1.0f);
+			this.recordundo = recordundo;
 			
-			if(map == General.Map.Map)
+			if(map == General.Map.Map && recordundo)
 				General.Map.UndoRedo.RecAddThing(this);
 			
 			// We have no destructor
@@ -151,7 +155,7 @@ namespace CodeImp.DoomBuilder.Map
 			// Not already disposed?
 			if(!isdisposed)
 			{
-				if(map == General.Map.Map)
+				if(map == General.Map.Map && recordundo)
 					General.Map.UndoRedo.RecRemThing(this);
 
 				// Remove from main list