Undo WIP

[monkeez]

Hey after talking about undo in the other PR I was messing around trying to get undo to work for the nicer syntax. I've come up with something that seems to work, but there's probably some huge flaw that I'm missing. Take a look and see if you think it's worth pursuing.

Undo without using modifiers directly

node = cmdx.createNode("transform")
node in cmdx.ls()
# Result: True # 

node["ty"] = 10
node["ty"]
# Result: 10.0 # 

cmds.undo()
node["ty"]
# Result: 0.0 #

cmds.undo()
node in cmdx.ls()
# Result: False #

Using modifiers to group commands for undo

nodeA = cmdx.createNode("transform")
nodeB = cmdx.createNode("transform")

with cmdx.DGModifier():
    nodeA["tx"] = 5
    nodeA["tx"] >> nodeB["tx"]

nodeB["tx"]
# Result: 5.0 # 

cmds.undo()
nodeB["tx"].connected
# Result: False # 
nodeA["tx"]
# Result: 0.0 #

Using modifiers the old way works too

with cmdx.DagModifier() as mod:
    node = mod.createNode("transform")
    mod.setAttr(node["tx"], 5)

node["tx"]
# Result: 5.0 # 
cmds.undo()
node in cmdx.ls()
# Result: False # 

How it works

Commands that edit the DG are wrapped by the @use_modifier decorator. This decorator gets the active modifier (DGModifier or DagModifier) if there is one, otherwise creates a new one for the operation. It then inserts the modifier into the _mod keyword argument, the function can then call modifier methods via _mod.

Limitations

Querying attrs after setting them inside a modifier won't give you up to date information (just like before when using a modifier).

with cmdx.DagModifier():
    node = createNode("transform")
    node["tx"] = 20
    value = node["tx"].read()

value
# Result: 0.0 #

Undo is more expensive than no undo 😄, the main cost comes from having to create new instances of DGModifier or DagModifier. The cost of this makes creating new nodes more in line with the speed of cmds.createNode.

cmdx.createNode("transform") # new DagModifier
cmdx.createNode("transform") # new DagModifier
cmdx.createNode("transform") # new DagModifier
cmdx.createNode("transform") # new DagModifier

However, most of the cost can be avoided by calling from within a DagModifier or DGModifier block.

with DagModifier(): # uses this instance for all nodes created within
    cmdx.createNode("transform")
    cmdx.createNode("transform")
    cmdx.createNode("transform")
    cmdx.createNode("transform")

If you take a look at the test_performance.py file I've wrapped the test in a modifier, and you can see that the performance tests still pass without changing the comparison value to cmds or maya.api.OpenMaya.

[mottosso]

Nice one. :) Yes, you've identified the only two paths I can think of, one of which was recently part of cmdx but got removed for a tirelessly complex reason.

DagModifier per call

There at least 4 reasons this isn't going to work.

1. Functionality MDagModifier simply can't do everything cmdx can do, so it becomes misleadning. You can't be sure what's actually undoable, and what is not. E.g. MFnTransform.setRotatePivot is that undoable? (No)
2. Plug-ins These need to implement undo their way, as an internal block of code. Which means none of the native API calls like createNode can't (shouldn't) undo themselves.
3. MDGModifier This is poor design IMO, you cannot create DAG nodes like transform using the DGModifier, and you cannot create DG nodes like multMatrix with the DagModifier. And you can't know which node type belongs where until you try. Especially for non-native plug-in types. Which means you now need to (1) instantiate one modifier, (2) test it and (3) instantiate another one. Triple the performance overhead, or more.
4. Performance I can't remember the numbers off-hand, but somewhere around a 10-100x slowdown?

In my experience, (1) is of most importance. Being confident that what you perform is both undoable, and is going to be undone in the exact same order. As soon as you mess up the order, such as involving calls to cmds, you not only risk breaking undo, and risk putting Maya in an unstable state (e.g. nodes in the outliner don't actually exist) but also risk a Maya crash. Too much of my life has been spent learning this the hard way haha. Undo/redo is a hard problem that is too easy to take for granted.

Implicit MDagModifier

I did this too, and used it for the first 3 years of cmdx. It was great, until it wasn't. The 2 reasons are similar to above.

1. Predictability Namely, not knowing what within that modifier block is actually undoable.
2. Memory management This one was the straw that broke the camels back.

For (1) like with embedding a modifier with native calls like cmdx.createNode, having what appears to be an undo block distills a false sense of security. If only one of the commands you use isn't undoable, you won't know until Maya crashes without telling you why and where.

For (2) it may still be solvable, I tried and failed many times but did not explore every venue.

The way I did it was to embed the MDagModifier with the node being created, such that any call such as ["attr"] = 5 would first check whether one was embedded, and whether it was still active (whether doIt had been called or not). And it worked great! Until you also consider that one of the main appeals of cmdx is the persistence of nodes. That myNode is the same node even when it changes name or moves in a hierarchy. Even when the variable is discarded and is recovered via e.g. cmdx.ls().

That means the DagModifier is also maintained, even after having been doIt() which means its internal state is maintained which means Maya cannot safely destroy the node.

Even when the node is removed from the user-side, a node lives in Maya memory until all memory of it is released. Maya will not tell you whether the memory is released or not, and will happily crash or not crash at the least convenient time.

This is the PR where all of that got stripped in favour of the current system.

• https://github.com/mottosso/cmdx/pull/56

Next Step

So! The challenge then is to overcome these two major concerns. I would still like to believe it's possible! But if there is one main advantage of explicitly calling mod.something() it is that predictability. Knowing that anything mod. can do is undoable. And anything you do outside of it is at-your-own-risk.

[monkeez]

Haha that is a lot of drawbacks!

For (1) Yeah that is annoying not knowing whether something will be undoable or not. Is it even possible to solve this one?

For (2) I've implemented it in a different way, but I haven't tested it enough to know if it faces the same memory issues.

[mottosso]

The safest way to test memory is to create a node from a plug-in, and try and unload that plug-in. The unload mechanism knows exactly what's going on and will tell you whether something is holding onto anything. I've only experimented with a custom plug-it, but I expect something like matrixNodes.mll to also complain about it.

[mottosso]

Is it even possible to solve this one?

Well, it is, yes. There's no great mystery to undo, the way cmdx implements it is by piggybacking on the modifier and calling undoIt at an appropriate time. But undoIt is nothing other than calling the opposite of whatever was done.

1. createNode was called on doIt, so deleteNode is called to undo
2. connect was called on doIt, so disconnect is called on undo

We could put a decorator on every single function in cmdx that includes the opposite of that call. The way you pass this to the undo stack currenctly is by calling cmdx.commit(undo, redo). So even MFnTransform.rotatePivot could technically also become undoable this way.

def myCustomRedoIt():
   cmdx.createNode()
   cmdx.setAttr()
   cmdx.connect()
   # blabla

def myCustomUndoIt():
  cmdx.disconnect()  # Must happen before we delete!
  # cmdx.setAttr() # Doesn't need to be undone since we're deleting the node (in this case!)
  cmdx.delete()  # Order is important

myCustomRedo()
cmdx.commit(myCustomUndoIt, myCustomRedoIt)

Anything could be put in these two calls, this is all of what undo/redo is. Nothing is truly "undone". Every command simply has an opposite. For setting attributes, the challenge is remembering the previous value, somehing the modifiers do a good job at and is easy to take for granted.

Is it worth it? Maaaybe.. You tell me.

[monkeez]

Is it worth it? Maaaybe.. You tell me.

My initial thought is probably no.. but I think I'll poke around some more.

[mottosso]

I can somewhat imagine a future where cmdx could on the front page state "Everything is undoable".

The number of calls to account for is large, but not infinite. If that could be made possible without breaking the readability of the code - like decorators is a good idea - and with it being explicit and optional, such that we could still write plug-ins using cmdx, it might be worth it. If it's explicit, it could potentially be made available piecewise, before everything is actually done.

Off the top of my head, if there was a..

with cmdx.UndoChunk():
  cmdx.createNode()  # Ok
  cmdx.setAttr()  # ERROR: Not yet implemented

That would probably be OK. So long as we can trust it.

Another advantage of the modifiers is just that, chunks of commands are made explicit. It means you can currently do things like..

with cmdx.DagModifer() as mod:
  mod.createNode()
  mod.connect(badNode, otherBadNode)

And it'll error out on that second call, before actually calling the first. Meaning Maya can never (theoretically) end up in a bad state, where a function fails half-way through leaving a big old mess behind. It'll either succeed completely, or do nothing and tell you why.

[mottosso]

Actually continuing on this, one thing I would like to encapsulate like this is also evaluation.

with cmdx.DagModifier() as mod:
   node = mod.createNode("transform")
   mod.setAttr(node["tx"], otherNode["tx"].read())

Currently, that call to .read() will trigger an evaluation of whatever otherNode needs in order to provide "tx". This is one of my current headaches since there can be calls upstream that effect evaluation in unexpected ways. Like, maybe otherNode is parented to another node which is parent constrained to yet another node which is like a follicle, which is driven by a mesh somewhere and the mesh is computed by whatever else. Suddenly, the command behaves unpredictably depending on the frame you are on, or whether there is an unrelated cycle, and depending on whatever else happens within that mod block.

My current workaround is something like this.

# Somewhere upstream
cache = {otherNode: {"tx": otherNode["tx"].read()}}
...
do_the_thing(node, otherNode, cache)

Where do_the_thing isn't calling otherNode["tx"] but rather cache[otherNode]["tx"].

If this could be encapsulated also, I would be a very happy man. Outside the scope of undo of course, just making note.

[mottosso]

Closing this for now. If you or anyone else find yourself poking around some more, this is still an interesting area of research and new PRs are welcome.