search

GafferHQ / gaffer

Gaffer is a node-based application for lookdev, lighting and automation
http://www.gafferhq.org
BSD 3-Clause "New" or "Revised" License
963 stars 207 forks source link

Moving nodes in the NodeGraph should be undoable #423

Closed johnhaddon closed 4 years ago

johnhaddon commented 11 years ago

This will probably require #422, so that transitioning between movement in one axis and movement in two axes will still merge into a single undo event appropriately. While doing this, test with a very large number of nodes to determine the performance implications of creating and merging a huge number of undo events.

Alternatively we could cheat and just make a single undoable event at drag end.

andrewkaufman commented 11 years ago

Are we sure moving nodes should be undoable? I'd be tempted to leave it out personally, at least until some serious user testing shows it to be necessary. I get annoyed in other packages when trying to undo actual operations, and my queue is full of nonsense node shifts that I did without thinking.

johnhaddon commented 11 years ago

I've found myself being annoyed that it wasn't undoable on occasion. It does seem like a pretty solid general principle that anything the user does to their script directly should be undoable and should set the unsavedChanges plug - currently (with #419) moving nodes does neither. Maybe you need to stop making nonsense node shifts without thinking?

andrewkaufman commented 11 years ago

They're nonsense in that I don't care about them being undone. They're legitimate in that I like to have a nice pretty node layout, and my opinion of what that is shifts depending on my mood. If Ben feels strongly, I won't oppose it. Just saying that in other software that does this, I find it quite annoying / unnecessary for the type of work I do.

johnhaddon commented 11 years ago

I'm not in any rush to do this one so we can wait for some more input. I just wanted to make a note of it (and the relation to #422) while it was fresh on my mind from the other undo work.

danieldresser commented 11 years ago

I'll just duck in here to state my opinion:

+1 for undoable node moves +1 for Andrew should stop making nonsense node shifts

bentoogood commented 11 years ago

I was just showing John one of my shader graphs this morning, moved some nodes off to the side to clarify the connections, then pressed ctrl-z to move them back. was a little surprised to see nothing happen, seems like a very natural thing to expect. so another +1 for undoable node moves

johnhaddon commented 11 years ago

Just noting that when we come to do this, we also need to make Backdrop resizing undoable.

ivanimanishi commented 10 years ago

I'm not that interested in the undo functionality itself, but I do think that changes to node positions should mark the "unsavedChanges" plug to True. It is quite reasonable to expect that a user would spend a whole session just re-organizing the graph without creating new nodes or changing plug values, and still have something that should be considered "unsavedChanges". In particular, I was trying to add functionality that would check for unsaved changes to either prevent unnecessary saves, or suggest a save before closing the application, or opening another script. And right now I can't go ahead with that as I may miss the kind of changes described above.

Not a critical issue right now, but I'd like to add my vote in favor of addressing this issue.

Chadt54 commented 5 years ago

Did you guys ever address this issue? It seems like resizing backdrops is undoable, but moving nodes is not. At least in the version I'm using (gaffer-0.52.3.1-linux)

johnhaddon commented 5 years ago

Nope - ticket is still open...