Old prefab undo bug postponed until improved prefabs.

Hi!

Back in April of 2017, I reported a bug regarding undoing prefab instances. Here's the repro code:

Code (csharp):

1.  
2. [Serializable]
3. public class Foo
4. {
5.     public int i;
6. }
7.  
8. public class Test : MonoBehaviour
9. {
10.     public List<Foo> foos = new List<Foo>();
11. }
12.  
13. [CustomEditor(typeof(Test))]
14. public class TestEditor : Editor
15. {
16.     private Test script;
17.  
18.     void OnEnable()
19.     {
20.         script = (Test) target;
21.     }
22.  
23.     public override void OnInspectorGUI()
24.     {
25.         base.OnInspectorGUI();
26.  
27.         if (GUILayout.Button("Add a foo!"))
28.         {
29.             Undo.RecordObject(script, "adding foo");
30.             script.foos.Add(new Foo {i = script.foos.Count});
31.         }
32.     }
33. }
34.  

The repro steps are:

- create a GameObject, add a Foo component.
- create a prefab from the GameObject.
- select the prefab instance (important!)
- click the "add a foo" button. Note that new Foo elements are added, with incremental i values:



- enter play mode:



Bye, bye, values! But the array size change is saved! what gives?

The issue here is that Undo.RecordObject doesn't record prefab instance changes properly. To fix this, the user has to call PrefabUtility.RecordPrefabInstancePropertyModifications on the target object:

Code (csharp):

1. Undo.RecordObject(script, "adding foo");
2. script.foos.Add(new Foo {i = script.foos.Count});
3. PrefabUtility.RecordPrefabInstancePropertyModifications(script);

When I reported this as a bug (956330), this was reported as intended behavior (?!?!). Upon making the point that it's utter insanity that Undo.RecordObject not recording the object is intended behavior, the reply was:

I was hoping that the new prefab stuff would have cleaned this up as a side-effect. That's not the case. It doesn't even manage to save the array size:



Which is, I guess, more streamlined, but in no way better.

In addition, we now clearly see that the overrides are not registered:



Now, this is kinda bad. Undo.RecordObject should not fail to record changes because the target object happens to be a prefab instance.


Is there any chance that this bug will be fixed now? It's been bothering me for a while that it just got brushed off with "might get fixed when we ship a new system" one and a half year before that system's due to be shipped. The fix for RecordObject should just be:

Code (csharp):

1. if(PrefabUtility.IsPartOfAnyPrefab(target))
2.     PrefabUtility.RecordPrefabInstancePropertyModifications(target);

Though of course I realize that there might be unforeseen side-effects I'm not realizing.


Anyway, I hope that this can get an internal fix now that the new prefab stuff is getting pushed out. Having to remember to do a PrefabUtility.RecordEtc. after every editor script isn't very ideal.

 

Hi,

I understand your frustration. Addressing this design issue in the undo system is not something we in Prefabs Team can prioritize at the moment, unfortunately. I understand it may have sounded like it would be addressed with the new Prefabs features, but in fact the person you were in contact with didn't know and just stated it might be a possibility. As it happens though, the new features did not touch upon that issue.

We are in contact with the people in the company maintaining and evolving the Undo system. Perhaps something fruitful will come out of that. For now though, the status is unchanged: While the design is not ideal, the issue you describe is by design and the way to deal with it is, as you know, to either call RecordPrefabInstancePropertyModifications, or - better yet - use SerializedObject and SerializedProperties.

 

Hey, thanks for the reply. I wasn't really expecting this to be something that you could fix now, but since this very much relates to prefabs, there was a chance that some change in the backend would relate to this.

The big issue isn't that I can't solve it - this goes into the list about weird Unity corner cases that I just know. The problem is that this a) isn't documented, b) is really hard to figure out and c) is a very sneaky bug - serialized data not sticking is hard to debug. Altogether this means that a bunch of other people, who didn't read this specific thread, is going to run into this issue and maybe not be able to deal with it.



The SerializedObject/Property interface is what I usually use. But the way that interfaces with arrays is the worst API I think I've seen in my life, so I tend to avoid it in all array-based work:

Code (csharp):

1. // non-serialized property:
2. myList.Add(new Foo { a = 1; b = 2; c = 3; });
3.  
4. // serialized property:
5. var index = myListProp.arraySize - 1;
6. myListProp.InsertArrayElementAtIndex();
7. var addedProp = myListProp.GetArrayElementAtIndex(index);
8. addedProp.FindPropertyRelative("a").intValue = 1;
9. addedProp.FindPropertyRelative("b").intValue = 2;
10. addedProp.FindPropertyRelative("c").intValue = 3;

That just hurts. I also don't want to force my coworkers to have to read throgh that.

 

Planned changes? Sounds interesting! Any information available about what those are?

Thanks!

 

YES. Document everything for the love of god! I cant believe it remained undocumented. Surely everything should be documented as its released?

I understand not touching on older features, but as new things are released why are they not also being documented in tandem?

Its the biggest frustration with unity, navigating documentation or rather working out which is information, which is disinformation, and what forum has the true info.