Undo in the editor

[Simon]

Undo in the Lix editor will happen.

This is long overdue. It's hard to implement on top of an editor that doesn't already support it, but it doesn't matter, it must happen. It's fine if it takes several weeks, as long as I focus on it whenever I develop.

Reason: Yesterday, I trashed 20 minutes of unsaved work and raged.



There are more severe usability issues in the editor. Giga's rant is still 80 % accurate after two years, mainly about how disorienting the editor is, and how bad/long the feedback loops are.

It's glaring when one hasn't build levels with the Lix editor for a year, and then comes back to it.

-- Simon

[namida]

In cases where I've had to retroactively implement Undo - and even in some cases where it isn't retroactive - I've achieved this simply by having a List<MemoryStream>, and each time a change occurs to the level, the level is saved (exactly the same way it might be saved to a file) to a MemoryStream which is then added to this List<>. When the user hits Undo, load the previous stream. Have an int variable keeping track of the current position in the Undo list; if the user is not at the end of the list and makes a change, nuke all MemoryStreams that are newer than the current position. (Not nuking them until this allows for a Redo function to exist.) You may need to skip certain aspects of loading (for example, you probably don't want to re-center the screen just in response to Undo) when loading from the undo list.

Probably a bit memory-hungry compared to a delta-based implementation, but also likely to be less bug-prone (especially in terms of bugs that cause data loss rather than just annoy the user) and far easier to implement when the system wasn't designed from the ground up with Undo support. I'd also guess that IRS levels are generally not complex enough for this kind of Undo to become a problem memory-wise on modern PCs.

[Ramon]

Very nice! An undo function will definitely improve workflow during level design sessions. It is not rare to do some unwanted button- or keypress that, while not terribly wiping all the work put in, is sometimes annoying to manually revert.

(On second read that sentence sounds really weird.)

[Simon]

Thanks. Yes, if I can't get anything else to work properly, I will savestate the entire level.

First, I will attempt a delta-based solution, with the OO pattern of doable/undoable command objects. The undoable actions will be really low level: For a rotation of 7 selected tiles around their averaged center, the undoable action is a 14-fold composite action of 7 individual undoable rotations and 7 individual undoable moves.

I had a similar idea 3 years ago, but wanted really high-level undoables. It was hard though to revert the 7-tile rotation exactly: There are rounding errors when computing the centers and the 7 tiles' new integer positions, with hackish special-case code to kill the rounding errors. This is a nightmare to revert high-level, but will be much easier low-level.

Ramond: Yes, the mistake is usually annoying to revert, but it was rarely annoying enough to push undo. The way I want everything, it will be a large-scale refactor of the editor, and I had been too scared to attempt it seriously for years. >_>

There will be many undoables, you can change the level in so many different ways. Maybe I'll only implement undoable deletion first, and clear the undo stack whenever a not-yet-undaoable command comes. Mis-deletions and mis-moves are the most annoying mistakes.

-- Simon

[ccexplore]

It was hard though to revert the 7-tile rotation exactly: There are rounding errors when computing the centers and the 7 tiles' new integer positions, with hackish special-case code to kill the rounding errors. This is a nightmare to revert high-level, but will be much easier low-level.

Interesting.  Does that mean it would've been difficult for a user to manually revert too?  It sounds like the user couldn't just rotate it back the other direction by the same amount and always expect to get back exactly to the original setup.

It's probably easier to implement undo of movement of a single tile by simply remembering the old position/orientation, and just restore them directly upon request to undo, instead of trying to honor it via an "opposite" movement.

[Simon]

rounding errors when computing the centers and the 7 tiles' new integer positions, with hackish special-case code to kill the rounding errors. This is a nightmare to revert high-level, but will be much easier low-level.

Does that mean it would've been difficult for a user to manually revert too?

Yes, exactly, this is a problem.

The special cases fix only the most common case when all these apply:

• You have one or more terrain tiles selected,
  
• you have no gadgets selected, and
  
• the encompassing rectangle of your selection has no sides that exceed wrapping dimensions of the map.
  

In this good case, if you rotate your selection 4 times, it should be back where it started.

Selected gadgets don't rotate when we tell them to rotate, this is the OO fruit basket problem with the fat-interface solution. Thus, gadgets in the selection break the assumption of the anti-rounding-error code that the (encompassing rectangle of the pieces that rotated in unison) is, per dimension, at most 1 pixel off the (theoretical rotation of the original encompassing rectangle).

Torus dimensions mod the tile coordinates after every rotation, this can squish the selection together too early, e.g., when you press rotate-quarter-turn twice to rotate half a turn.

remembering the old position/orientation, and just restore them directly upon request to undo, instead of trying to honor it via an "opposite" movement.

This is smart. My hunch would have been to separate UndoableMove : Undoable and UndoableRotation : Undoable. But your approach sounds even more robust and reasonable.

-- Simon

[Forestidia86]

Do you plan undo and redo then for grouping of tiles? There is an issue (#280) that ungrouped tiles go into their original position instead of the one that they were in when they were grouped (Edit: remembered that wrongly, the issue only occurs when rotating/mirroring the tile group, the tiles go then in the original grouping position). How does that interact/relate then? Does undo put them then in the grouping position instead of the original?

[Simon]

Undo a grouping: Yes, this should be undoable. Whenever the most recent action has been a grouping, undoing that grouping can't run into issue #280 because the group is in its original orientation.

Issue #280 is interesting and needs more work. I feel like issue #280 is rarely a problem, but when it hits, it feels so odd, indeed. I haven't invested enough time to think about a nice solution to #280, maybe it's easier than I think.

-- Simon

[Forestidia86]

Ah, I understand, I had the issue wrongly remembered.

[Simon]

After heavy refactoring, this is the status quo of unreleased development lix-unstable, branch editor-undo:

• Editor has undo/redo buttons.
  
• Inserting a tile from the tile browser is undoable. This is good as-is, although rarely useful.
  
• Deleting an arbitrary selection is undoable as a unit. Undo re-adds the tiles. They aren't selected after getting added. Should they be selected? This will be a doable but nontrivial task; ideally, we'll decide later.
  
• Copying an arbitrary selection is buggy at the moment, this must be fixed, and this should also be undoable as a unit.
  
• Moving an arbitrary selection is undoable, but in a terrible way: While the mouse isn't still, each individual moving tile generates 60 undoable commands per second only for this single tile. These must certainly be grouped. But how? Complex scenario: You drag 3 tiles, and mid-motion, hit the hotkey to rotate them. Should this generate 1 undoable, or 3 undoables (move, rotate, move)? I'm leaning towards 3.
  
• Rotation/mirroring/dark isn't undoable yet.
  
• Changing level properties via dialogs isn't yet undoable. This is the most boring and I have 100 % compassion for NL to have had a bug there.
  
• When stuff happens that isn't yet undoable, but we undo the most recent undoable, consistency checks fire and crash the app with an error.
  
• When the undo stack is empty, the undo/redo buttons are shown anyway. This is a general problem in the editor, it has clickable buttons that do nothing, e.g., rotate button when you have nothing selected.
  

Current no-undo Lix version is 0.9.31. Maybe I'll have enough undo in shape for 0.9.32, maybe not. Anyway, the first version with undo will probably get this:

• Fix copying tiles. Then all adding/removing will be undoable. This will be useful already: Undoing deletions is the important use case.
  
• Make the tile moving non-undoable, i.e., cut this half-baked feature. Undoable moving can happen in a future version. I haven't released in months, it's time to release something.
  
• Empty the undo stack whenever we do something yet non-undoable in the editor. This will keep the undoing always consistent. No more crashes from detected inconsistency.
  
• Show/hide the undo buttons when the stack is empty.
  

In the yet-distant future, everything should be undoable. The undo stack should never be emptied during normal work.

-- Simon

[Proxima]

Changing level properties via dialogs isn't yet undoable.

Does it have to be? If you want to change the level properties, even if it's reverting a change, you are more likely to go into the dialogue again so as to make sure the values are correct, since most of them are not visible from the main editor screen. Conversely, making this kind of change undoable carries a high risk of the user accidentally undoing and not realising something is wrong.

I guess the one time you would want to undo a dialogue change would be if you know you want the old value back but don't remember exactly what it was.

[Simon]

Right -- skills, name, number of lix etc. don't have to be undoable. I was unsure with this too. I suspected that it should be undoable for consistency with everything else, but good usability here will be delicate and costly, right.

Map dimensions and wrapping: When this changes, tiles may move/wrap. This seems much harder to keep orthogonal to what happens in the undo stack. Likely, I'll have to make this undoable.

-- Simon

[namida]

Deleting an arbitrary selection is undoable as a unit. Undo re-adds the tiles. They aren't selected after getting added. Should they be selected? This will be a doable but nontrivial task; ideally, we'll decide later.

I would personally think it desirable but non-critical for them to be selected.

But how? Complex scenario: You drag 3 tiles, and mid-motion, hit the hotkey to rotate them. Should this generate 1 undoable, or 3 undoables (move, rotate, move)? I'm leaning towards 3.

I would not have expected this situation to be possible in the first place - I would expect the rotate either terminates, or is ignored during, the drag. To be fair, I wouldn't even attempt it in the first place.

I should note that I tend to follow the opposite workflow of this, though. I would likely be using the clickable buttons to rotate / etc, while using the keyboard to move pieces. This is due to the keyboard movement being more precise. I may occasionally move a piece a long distance with the mouse; I would not try to rotate / etc it while doing this (I'd most likely do so afterwards, if I were going to do so at all, though I'm not 100% sure it would never be "before" - I am sure it would not be "during"), and would almost certianly follow up with some keyboard movement afterwards to get it to the exact desired spot.

Make the tile moving non-undoable, i.e., cut this half-baked feature. Undoable moving can happen in a future version. I haven't released in months, it's time to release something.

I would consider undoing a move to be almost as important, perhaps even more so, than undoing a delete. Accidental moves, or accidentally including something in a move, etc, are far easier to do than an accidental delete, in my experience.

Empty the undo stack whenever we do something yet non-undoable in the editor. This will keep the undoing always consistent. No more crashes from detected inconsistency.

Unsure about this. Perhaps valid if the intent is "this will never be undoable", though perhaps preferable there is to make the thing in question outside of Undo / Redo altogether, rather than making it clear the stack. For things that will be undoable in the future, perhaps instead of clearing the stack, it should give a warning when the user attempts to undo, then allow them to continue undoing from the last undoable action?

[Simon]

Thanks, several good ideas.

You drag some tiles, and mid-motion, hit the hotkey to rotate them. Should this generate 1 undoable, or 3 undoables (move, rotate, move)? I'm leaning towards 3.

I would not have expected this situation to be possible in the first place

I think I do it because I know how it's implemented. The mouse dragging moves the tiles 60 times per second; these moves (in 0.9.31 = current no-undo Lix) are committed instantly to level every time, thus the intermixed rotation is meaningful and uninterruptive.

The downside of this frequent committing is that it's more work now to merge the entire dragging into a single undoable.

This frequent committing isn't standard in GUI. Other programs will let user drag a shadow or an outline of the dragee. I'm really 50:50 on this. The rotation on the fly is cool and matches how we move real-life household objects.

clickable buttons to rotate / etc, while using the keyboard to move pieces.

follow up with some keyboard movement afterwards to get it to the exact desired spot.

Right, this tile nudging via keyboard is also extremely common. The keyboard is more precise than the mouse when grid alignment won't suffice.

When the mouse isn't dragging, and the key is merely tapped, not held, then I'll make the tap generate a single standalone undoable for its effect: moving all selected pieces, rotating selected pieces, deleting selected pieces, ... When we tap 5 times in quick succession to nudge selection rightwards by single pixel each, this generates 5 undoables that I won't merge. Sounds OK.

When a directional key is held, not merely tapped, the selection moves once, then waits (threshold), then moves 60 times per second. I would like the entire keyholding to generate only a single undoable.

There is more to reply. Until soon!

-- Simon

[Simon]

I would consider undoing a move to be almost as important, perhaps even more so, than undoing a delete. Accidental moves, or accidentally including something in a move, etc, are far easier to do than an accidental delete, in my experience.

Undo of tile moves is now implemented! Moving multiple tiles generates only a single undoable, and (continuous dragging) or (directonal key holding) also generate only a single undoable. Comfortable.

Thanks for the push! It was interesting to design the two possibilitis of merging moves: Several tiles moving the same distance as the first tile, or the same tiles move further than they moved originally while dragging or key-holding.

I agree that moving is the most common action. Even though a single wrong move is easier to correct manually than a wrong deletion, it's likely possible that wrong moving generates the most frustration overall.



Thus, tile moves and insertions/deletions are already fully undoable in the unstable repo.

What's left:

• Duplicating tiles must be undoable. The most likely candidate for this is a compound undoable of an insertion followed by a move. (A proper copy buffer that doesn't immediately paste would also be nice, e.g., to copy between levels, but that's a separate feature.)
• Rotations and mirroring must be undoable.
• Resizing of the map must be undoable, because it prompts tiles to move, and activating wrap (torus) imposes new restrictions on allowed tile coordinates. If we don't make resizing undoable, then it might violate the consistency checks of existing undoables on the undo stack that move to/from now-forbidden coordinates.

Editing the name/skills/numbers of levels is completely orthogonal to everything on the undo stack, and we can easily leave it out.

-- Simon