Undo Support about node_editor_framework

[Keerpich]

I'm going to give this a shot. Will make a PR later and modify where it's needed

[Seneral]

Hi! Thanks @TwoOfDiamonds, very thankful for that. Might be a daunting task though. If you feel you need more control than the default undo system we could consider using UndoPro in order to make use of undo actions instead of the preset commands of unity.
Would be better without to avoid unneeded scripts, though.

[Keerpich]

I was thinking about adding 2 stacks to the canvas and have one for the Undo action and one for the Redo action (Ctrl+Z and Ctrl+Y). I would add the performed actions to the Undo stack when they are done. Like adding, removing, moving nodes, etc.
The only problem with this approach is that it will change code in various places throught the scripts in order to add the action to the stack when they are executed.

[Seneral]

Not a good idea, better to use standard undo system of Unity. Atleast for the editor, that is. Because the big problem would be that you can not use the shortcuts Ctrl+Z and Ctrl+Y that everyone is used to, and you would also loose all other interfaces.
So standard Undo is the way to go, atleast until you find it would be impossible with the basic commands that system provides, in which case we'd probably make use of UndoPro, which is a tool to add action commands to the standard undo system to perform complicated tasks:)

[Keerpich]

I will try with the Unity Undo system and if it works for the editor I will be happy

[Julien-B]

Hi, We're trying to use something based on UndoPro for our implementation of the Node Editor (to create behaviour trees). We're ran into a big issue though : say you modify a value in a node, you'll get an Inspector entry in the Undo stack. If you then delete the node, the first Undo will work and get it back, but it broke the reference of the node and the second Undo doesn't restore the former value.

Is there a way around that ?

[Seneral]

I'll try to explain whats going on...
When creating a record using anonymous actions (which you probably did), UndoPro serializes some context data which you're referencing along the record. Now if you just do

int oldValue = node.value; // This will ensure it saves the current value, not node.value
UndoProManager.RecordOperation (()=>node.value = newValue, ()=>node.value = oldValue, "Changed Value");

Then these following variables are saved along the record:
node, newValue, oldValue
And if node is then deleted and recreated (f.E. through a different record), the reference is lost.

A way around this is replacing the direct reference to node by some other value to identify the node, like an ID.
So lets assume you have that ID in Node as a field called uniqueID and a method called GetNodeByID on NodeCanvas to get a node by ID. The code could now look like this:

int oldValue = node.value; // This will ensure it saves the current value
string nodeID = node.uniqueID; // This will ensure it saves the actual ID
UndoProManager.RecordOperation (()=>nodeCanvas.GetNodeByID(nodeID).value = newValue, ()=>nodeCanvas.GetNodeByID(nodeID).value = oldValue, "Changed Value");

Which would cause these variables to be serialized along the record:
nodeID, newValue, oldValue
And thus when you destroy and recreate the node the ID will still uniquely identify the node:)

Currently, there's nothing like a unique node ID implemented in the framework, the ID you can access on the node is type-specific. So you'd need to ensure yourself that each node has it's unique ID (add it into Node.InitBase). Actually, you could make a PR for that if you want or I can do it myself because that is necessary for other things as well, like XML serialization.

Well, long read, hope you understood it:)
Would very much like to see how it turned out for you, and if you don't mind, please create a PR;)

Btw, I think most undo parts can be done with the default undo system, especially related to the GUI for each node. Maybe it's better to try relying on that wherever possible, but I can imagine that quite some things are very tricky with it, hence I built UndoPro;)

[Julien-B]

We already have an ID in every node for our Undo in the Pro records stack, but modifications in the inspector add an 'Inspector' entry in the default undo stack, not our custom stack, and we have no control over it. Same for 'Selection change' entries.

Theses entries are generic, they work for selection changes or modifications in the inspector anywhere, not just in the node canvas (ie. selecting a script in the project window). But when they apply to a node that was deleted/recreated they break.

Edit: just tried creating a 3d sphere, changing stuff, deleting it and Undo. The sphere is created again and the Inspector entry works. So Unity can do it. But, how? And can it apply to something other than a GameObject (our Nodes are ScriptableObjects), or outside of the Scene?

[Seneral]

Oups, misunderstood you, thought you were using UndoPro for both the 'value changed' and 'deleted' entries...
Can I see your lines of code you used in both deleting and value changed scenarios?

I just tested, the default undo system does not work correctly with assets, but in the editor using the cache we are directly working on assets, so using the default system is not applicable here:(
Using UndoPro records for deleting and creating nodes and when changing values works flawlessly.
Only problem is that when using UndoPro records for creating a node, and recording changes of that node using the normal undo system, the normal record is invalid once the UndoPro record has been undone and redone (node is created completely new) as the reference of the default undo system to that node is broken, I assume. I hope that is clear;)
So currently we can only use that recreation using UndoPro records with other UndoPro records. BUT we do need normal records aswell, as we don't want the node to manually watch each property in the node but we only want a Undo.RecordObject wrapped around Node.NodeGUI...
So we need a different solution...

Here's a test snippet on a sample Node asset showing what I said above (GUI part only):

GUILayout.Label ("Node '" + (node==null? "null" : node.name) + "' rect '" + (node==null? "null" : node.rect.ToString ()) + "'");

if (GUILayout.Button ("Test"))
{
	node = null;
	// Does not work:
	// Undo.RegisterCreatedObjectUndo (node, "Created Node asset");
	// UndoPro works here:
	UndoPro.UndoProManager.RecordOperationAndPerform (
		()=> {
			node = CreateInstance<NodeEditorFramework.Standard.CalcNode> ();
			AssetDatabase.CreateAsset (node, "Assets/Test.asset");
			Debug.Log ("REDO PRO: Node is " + (node==null? "null" : node.name));
		}, ()=> {
			ScriptableObject.DestroyImmediate (node, true);
			AssetDatabase.DeleteAsset ("Assets/Test.asset");
			Debug.Log ("UNDO PRO: Node is " + (node==null? "null" : node.name));
		}, "Created Node asset PRO");

	// Make sure to copy values! And do NOT resuse the variable or it will overwrite the record variable!
	Rect newRect = new Rect (0, 0, 100, 100);
	Rect prevRect = node.rect;
	Debug.Log ("Now changing rect from " + prevRect.ToString () + " to " + newRect.ToString () + "!");
	UndoPro.UndoProManager.RecordOperationAndPerform (
		()=> {
			node.rect = newRect;
			Debug.Log ("REDO PRO: Node Rect is " + (node==null? "null" : node.rect.ToString ()));
		}, ()=> {
			node.rect = prevRect;
			Debug.Log ("UNDO PRO: Node Rect is " + (node==null? "null" : node.rect.ToString ()));
		}, "Changed node rect PRO");
	
	// Now using the default undo system. Copying is not needed
	Rect newRect2 = new Rect (100, -100, 200, 50);
	Debug.Log ("Now changing rect to " + newRect2.ToString () + "!");
	Undo.RegisterCompleteObjectUndo (this.node, "Changed node rect NORMAL");
	node.rect = newRect2;
	Debug.Log ("AFTER: Node Rect is " + (node==null? "null" : node.rect.ToString ()));
}

I'll report once I have a correct 'rig' supporting both UndoPro and normal records on asset objects along each other:)

[Seneral]

Ok so for now I actually can't see a way of implementing Undo, UNLESS we completely disable the current cache system, which directly works on the asset objects.
But Undo.RegisterCreatedObjectUndo and Undo.DestroyObjectImmediate and probably the other undo methods, too, simply do not work at all with asset objects. We also cannot use UndoPro instead for creating and deleting objects, because that would break all references to them, including those from default records made of these objects. That means we could not use the default Undo system on these objects.
But we do need the default system for nodes, for example to record changes made in Node.NodeGUI, which UndoPro cannot do by default due to it's command-pattern nature.

Hope you understand, and please tell me if you found a way around it.
An idea I have are to rewrite the cache system (or replace it) so it does not work on assets, which would also have the benefit of less lag when creating and deleting nodes... but that would mean a higher chance of lost progress when the editor crashes. Then, Undo should be quite easy to add I imagine.
That is something to sincerely consider because Undo is vital, the cache system not.
What's your opinion on this?

[Julien-B]

Well we don't know much about Unity's undo system apart from the fact that we need it to put our behaviour tree editor on the store. The solution for our implementation would be to do every edits (child priority, method choice, etc..) from custom controls on the nodes instead of the inspector. That way we have full control and can use IDs like we do for other actions.

But I don't think it's viable for a general use as we heavily modified the node editor to fit our project and others might still need to be able to edit properties from the inspector ?

[Seneral]

Ok, so I think I will be developing a different (simpler) cache system in the future that does not work on assets directly and as such is faster and allows for undo at the cost of a bit of safety incase of Unity crashing...

[Seneral]

Yes, using something like UndoPro with explicit records is not really applicable for the node GUI, not only because of the inspector, but I don't want to care about Undo when I'm simply creating a node with standard controls...

So I think I'll just create a different, much simpler cache system that does not work directly on assets, which has the benefit of significantly less lag when creating nodes and a possibility to add Undo but with the risk of loosing progress when Unity crashes. But I think we can accept that, don't you think?

[Julien-B]

How much progress are we talking about ? The entire session ?

The undo is a requirement for the Unity store, if it doesn't work with assets, I don't think we have a choice.

[Seneral]

We could auto-save every 5 or 2 mins of course. So it's not automatically saved while it is created as it is right now, but saved at intervals and of course when closing the window.

Of course you're right about Undo... Do you have a release date set? I'll try to do at least the cache over the holidays, then Undo should be possible:)

[Seneral]

Implemented second cache system that should enable Undo support in 50b28a3
Now, canvas elements are not assets anymore and thus the standard Undo methods apply.
If you want to give it a try, go ahead! :)

[Julien-B]

We'll have a look at it, thanks !

[Votrubec]

Great Work!!

[Seneral]

Oh hold on, this should be closed! Implemented long ago in 0f69501