From 199108ba649bbd4e232ef13018797799a690bf40 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 | 19 ++++++++++++-------
 Source/Core/Map/MapSet.cs          | 20 ++++++++++++++++++++
 Source/Core/Map/Thing.cs           | 10 +++++++---
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/Source/Core/Editing/ClassicMode.cs b/Source/Core/Editing/ClassicMode.cs
index b78a978..f21699c 100644
--- a/Source/Core/Editing/ClassicMode.cs
+++ b/Source/Core/Editing/ClassicMode.cs
@@ -699,6 +699,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 vindow!");
@@ -748,15 +750,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;
 					}
@@ -776,7 +779,9 @@ namespace CodeImp.DoomBuilder.Editing
 				playerStartPosition = start.Position;
 
 				//everything should be valid, let's move player start here
-				start.Move(new Vector3D(mousemappos.x, mousemappos.y, 0));
+				start.Move(new Vector3D(mousemappos.x, mousemappos.y, General.Map.SRB2 ? 0 : s.FloorHeight));
+
+				General.Map.UndoRedo.IgnorePropChanges = oldignorepropchanges;
 			}
 
 			return true;
@@ -788,7 +793,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 3227400..27813a4 100644
--- a/Source/Core/Map/MapSet.cs
+++ b/Source/Core/Map/MapSet.cs
@@ -679,6 +679,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 deefdb9..953b2c2 100644
--- a/Source/Core/Map/Thing.cs
+++ b/Source/Core/Map/Thing.cs
@@ -91,6 +91,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;
 
@@ -142,7 +145,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
@@ -153,8 +156,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
@@ -167,7 +171,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