Bug Editor Undo API Works but Corrupts Scene on Scene Reload

I've been having trouble implementing UndoRedo for a custom editor tool. I've managed to get it to work perfectly, however if I perform a undo and then a redo and then save the scene and reload it or hit the play button, the scene heirarchy is broken and stays broken (as if the scene file was incorrectly modified).

I have reduced my problem to a simple reproduce-able test case.

I have the following Scene Hierarchy with a "TestObject" that has a TestMap component attached which only has a public List<MapPiece> exposed:


Here are the two monobehaviours I am using in the example:

Code (CSharp):

1.  
2. public class MapPiece : MonoBehaviour
3. {
4.     public TestMap testMap;
5.     public Vector3 vec;
6.     public int number = 0;
7. }
8.  
9. public class TestMap : MonoBehaviour
10. {
11.     public List<MapPiece> pieces = new List<MapPiece>();
12. }
13.  

TestMap is attached to the "TestObject" gameobject (as shown above).
MapPiece is attached to the root of a simple prefab created by creating a simple "primitive Cube" gameobject using the built-in gameobject context menu (Right Click > 3D Object > Cube). This prefab has a "primitive Sphere" gameobject as a child object.


Now, for the test code :

Code (CSharp):

1.  
2. public class ExampleMenuItem : MonoBehaviour
3. {
4.     [MenuItem("Test/ClearUndoRedoStack")]
5.     static void ClearUndoRedoStack()
6.     {
7.         Undo.ClearAll();
8.     }
9.  
10.     [MenuItem("Test/UndoExample")]
11.     static void UndoExample()
12.     {
13.         GameObject prefab = AssetDatabase.LoadAssetAtPath<GameObject>("Assets/gitignore/UndoTest/cube.prefab");
14.         GameObject testContainer = GameObject.Find("TestContainer");
15.         TestMap testMap = testContainer.transform.parent.GetComponent<TestMap>();
16.  
17.         for (int i = 0; i < 3; i++)
18.         {
19.             GameObject prefabInstance = (GameObject)PrefabUtility.InstantiatePrefab(prefab, testContainer.transform);
20.  
21.             Undo.RegisterCreatedObjectUndo(prefabInstance, "");
22.  
23.             PrefabUtility.UnpackPrefabInstance(prefabInstance, PrefabUnpackMode.Completely, InteractionMode.UserAction);
24.  
25.             MapPiece mapPiece = prefabInstance.GetComponent<MapPiece>();
26.  
27.             Undo.RecordObjects(new UnityEngine.Object[] { mapPiece, testMap }, "");
28.  
29.             mapPiece.testMap = testMap;
30.  
31.             testMap.pieces.Add(mapPiece);
32.  
33.             mapPiece.transform.position = new Vector3(i, mapPiece.transform.position.y, mapPiece.transform.position.z);
34.  
35.             mapPiece.vec = new Vector3(i, i, i);
36.             mapPiece.number = i;
37.  
38.             EditorUtility.SetDirty(testMap);
39.  
40.             Undo.FlushUndoRecordObjects();
41.             Undo.CollapseUndoOperations(Undo.GetCurrentGroup());
42.         }
43.     }
44. }
45.  

Reproduction steps:
1. Click `Test > UndoExample` from the top menu bar.
Result :


2. Press `Ctrl + Z` to trigger Undo.
Result : Scene returns to normal.

3. Press `Ctrl + Y` to trigger Redo.
Result: Scene returns to the exact same state as in step 1 (as expected!) but don't celebrate just yet...

4. Save the scene. Then either press the play button (reloads the scene) or reload the current scene in the editor.
Result : Scene is broken if reloaded in the editor. Unity crashes if you press the play button (you will have to manually delete the scene file using your file explorer in order to open Unity again without crashing).



Why does this happen? Am I not using the Undo API correctly?
Should I file a bug report with Unity?
Any help is much appreciated here. Thanks.

Unity version: 2021.3.14f1 Personal <DX11>
Platform: Windows 10 64bit

 

I am going to file a bug report for this since I've had no response. I'll post any updates from the bug report here.

 

Yeah, I've got nothing... you might have a Unity bug, but on the other hand you are in some pretty sticky complicated code with the unpacking prefabs stuff.

It appears you are undo-saving the right stuff, but I might suggest recording the mapPiece.gameObject instead, or otherwise trying combinations of other objects.

I would need to kinda diagram the object creation and mutation you got going to reason about if it is capturing everything it needs to or not.

 

When I do an Undoable sequence of multiple steps, I follow very clear steps as if they were independently-invokable commands. Your code doesn't really read like there's a clear bracketing of stuff to be done, at least not like I've seen before.

Code (CSharp):

1.         Undo.IncrementCurrentGroup();
2.         Undo.SetCurrentGroupName("Do Things");
3.         int undoID = Undo.GetCurrentGroup();
4.         foreach (var gobj in gobjsToBeDone)
5.         {
6.             MyComp comp = gobj.GetComponent<MyComp>();
7.             Undo.RecordObject(comp, "Do One Thing");
8.             comp.DoThing();
9.         }
10.         Undo.CollapseUndoOperations(undoID);
11.  

 

@halley Thanks for the reply. I want to make multiple changes to multiple things and collapse it into a single action. I record all steps for each action individually and collapse them into the current group. I should then increment the group index but I didn't bother as this is a minimal test example that should run once and work (plus I have a test context menu action that clears the undoredo stack).

As for my logic not having a "bracketing of stuff" (I'm not quite sure what you mean by this) but I think the logic makes sense and follows a simple set of steps:

1. Do the following 3 times:
A. Instantiate prefab
B. Since I created a gameobject in the scene, I register its creation via RegisterCreatedObjectUndo
C. Unpack the prefab, passing `InteractionMode.UserAction` as a parameter which registers this action with the undo system.
D. Start recording MapPiece object attached to the newly created instance and the testMap object that is in the scene
E. Make changes to the MapPiece object (assign testMap reference)
F. Make changes to the testMap object (assign mapPiece reference)
G. Make changes to the MapPiece object (modify various values)
H. Mark Map as dirty so the user has to save the scene\
I. Ensure objects recorded using RecordObjects are registered as an undoable action.
F. Collapse all undo operation up to group index together into one step

Perhaps I have to add the mapPiece Transform component to the list of objects being recorded?
Edit: nope, I added mapPiece.transform to the array of Objects passed to recordObject() but I still get the same result.

 

So after some experimentation it seems that commenting out this line causes no errors:

Code (CSharp):

1. PrefabUtility.UnpackPrefabInstance(prefabInstance, PrefabUnpackMode.Completely, InteractionMode.UserAction);

However, I want to unpack the prefab instance. What should I do to have the unpacking of the prefab registered with the same undo command along with all the other modifications? Any Ideas?

I feel like my logic is correct and that this is a bug with the Undo system which is why it would be great if a Unity dev can take a look at this and tell me if I did things correctly and this is indeed a bug or I messed up and in this case what are the correct steps to achieve the desired result?

Just to note, I've tried passing InteractionMode.AutomatedAction instead which doesn't break the scene file but I get incorrect behavior now when doing ctrl+z then ctrl+y which is expected since the unpacking of the prefab is not being recorded if I flag the action as "Automated".

For reference, here is the source for UnpackPrefabInstance : https://github.com/Unity-Technologi...no/Prefabs/PrefabUtility.cs#L2776C28-L2776C48

which is slightly different than my version that I see in VS:

Code (CSharp):

1. public static void UnpackPrefabInstance(GameObject instanceRoot, PrefabUnpackMode unpackMode, InteractionMode action)
2. {
3.     if (!IsPartOfNonAssetPrefabInstance(instanceRoot))
4.     {
5.         throw new ArgumentException("UnpackPrefabInstance must be called with a Prefab instance.");
6.     }
7.  
8.     if (!IsOutermostPrefabInstanceRoot(instanceRoot))
9.     {
10.         throw new ArgumentException("UnpackPrefabInstance must be called with a root Prefab instance GameObject.");
11.     }
12.  
13.     if (action == InteractionMode.UserAction)
14.     {
15.         string name = "Unpack Prefab instance";
16.         Undo.RegisterFullObjectHierarchyUndo(instanceRoot, name);
17.         GameObject[] array = UnpackPrefabInstanceAndReturnNewOutermostRoots(instanceRoot, unpackMode);
18.         GameObject[] array2 = array;
19.         foreach (GameObject instanceComponentOrGameObject in array2)
20.         {
21.             UnityEngine.Object prefabInstanceHandle = GetPrefabInstanceHandle(instanceComponentOrGameObject);
22.             if ((bool)prefabInstanceHandle)
23.             {
24.                 Undo.RegisterCreatedObjectUndo(prefabInstanceHandle, name);
25.             }
26.         }
27.     }
28.     else
29.     {
30.         UnpackPrefabInstanceAndReturnNewOutermostRoots(instanceRoot, unpackMode);
31.     }
32.  
33.     PrefabUtility.prefabInstanceUnpacked?.Invoke(instanceRoot);
34. }