Undo in the editor

[Simon]

I have undoable duplication of tiles (copy-paste without a copy buffer). There is a regression left behind: The newly inserted tiles aren't yet selected. The old selection remains, which is practically never what you want.

New to-do list:

• Enrich all Undoables to suggest which tiles should be selected after apply/undo. Main reason are copy-pasting Undoables, but it will be handy for insertions and undone deletions.
• Rotations and mirroring must become undoable.
• Z-ordering (move to foreground/background) must become undoable.
• Hotkeys Z and Y will bind to undo/redo by default. Both are still free in the default editor layout.
• Resizing of the map must become undoable, or clear the undo stack if we want to rush the feature's release.

There will also be lots of lovely refactoring for cleanup, but that's not necessary to release the undo feature. Since the Undoables do the bulk of the work, I will be able to ditch the Hover class hierarchy, which is parallel to the tile hierachy and has bugged me since I made the editor.



-- Simon

[Simon]

Spent all Saturday on Undo. Progress:

• Hotkeys Z and Y are bound to undo/redo by default. You can change it in Lix's options menu.
  
• All Undoables will now suggest what tiles shall be selected after apply/undo. In particular:
  When you undo a deletion, the undeleted tiles will be selected.
  When you duplicate tiles, the newly inserted tiles will be selected.
  
• Lots of refactoring towards immutability of the involved classes. But the asserts in the area where I considered triple dispatch have begun to fire when I duplicate a large selection of both gadgets and terrain tiles.
  

New to-do list:

• Fix above bug in the undoable copy-paste.
  
• Rotations and mirroring must become undoable.
  
• Z-ordering (move to foreground/background) must become undoable.
  
• Resizing of the map must become undoable, or clear the undo stack if we want to rush the feature's release.
  

Cleanup work that isn't necessary to release undo, but that I would like to tackle:

• Ditch the Hover class hierarchy in the editor. This is a parallel class hierarchy that wrap a selecte tile. Work more directly with the raw selection of tiles.
  
• Open a pull request against Phobos, the D standard library, to allow RedBlackTrees with immutable classes as payload.
  
• Open another pull request against Phobos to make SortedRange const-correct. I would have preferred SortedRange over RedBlackTree for the Lix code from the beginning, but RedBlackTree had fewer roadblock bugs.
  
• Attempt to fix a compiler bug: 21321: Class with unimplemented interface method compiles, links, then segfaults, if inherited through abstract base class. I ran into this yesterday. It would be the first time that I seriously look at the D compiler's source, it's a large and scary codebase.
  

^ Illustration of a pull request. You must be careful, watch for feedback, and don't mind the time spent. I had one pull already merged into Phobos a month ago.

In the end, running into 4 library/compiler issues within one big Lix feature shows what parts of D are rarely treaded: Heavy object-orientation, exhausting and combining the OO features of D, making stuff transitively immutable where possible, and using the more obscure areas of the standard library.

Crane would be delighted, if it weren't all so template-heavy.

-- Simon

[Simon]

Lots of bugfixing this evening. Copypasting does the right thing now in all cases, I don't believe I have introduced other bugs yet. This resolves the bug in yesterday's to-do list.

I've begun with the class that applies/undoes tile rotation/mirroring/darkening. This will be exactly how ccexplore suggested I should implement tile moving. The exception is that the existing TileMove objects merge with one another if they're the same tiles that get dragged further during the same mouse dragging, or if they're simultaneously selected tiles that move the same distance. Rotating/mirroring/darkening will never merge with subsequent editing; it will only form a CompoundUndoable with the simultaneously selected tiles.



Thus, very successful evening. Now time for bed.

-- Simon

[WillLem]

I'm enjoying all these cute gifs

[Simon]

Grouping/Ungrouping is undoable.

Small issue left behind during Ungrouping: If you have more than one group selected and click Ungroup, only one of the groups will ungroup. I assume that this is unimportant.

Darkening (turn terrain piece into eraser piece) is undoable.

To-do:

• Rotations and mirroring must become undoable.
  
• Z-ordering (move to foreground/background) must become undoable.
  
• Resizing of the map must become undoable, or clear the undo stack if we want to rush the feature's release.
  

-- Simon

[namida]

If you have more than one group selected and click Ungroup, only one of the groups will ungroup. I assume that this is unimportant.

If I specifically thought about this / tried it out, I would expect the behaviour to be that it ungroups all. But, I probably would never find this unless I was specifically thinking of edge cases to try, in the first place.

[Simon]

expect the behaviour to be that it ungroups all.

Right, that is what I would expect, too, and this is how it has worked before undo.

The new limitation (only ungroup one group, keep all others grouped) comes from the strong encouragement (by the undo design) to describe the entire modification to the level before committing anything to the level.

After one group tile has been deleted and its parts have been inserted in the level, the next group tile will potentially sit at a different z-order (counted from the front of the terrain list), and this messes with the iteration if we commit the first ungrouping to the level. If we don't commit the first ungrouping yet, then the iteration continues well, but I save absolute positions (index numbers within the terrain list) in the Undoables, and the future absolute positions for the next ungrouping will be computed wrongly.

There are hack solutions around this: Create ungrouping Undoable, commit to level, create next ungrouping Undoable, commit to level, ..., then undo all these Undoables, create a CompoundUndoable of them, and recommit this CompoundUndoable. Or track meticulously all the index shifts in some complex data structure.

But ungrouping is already very rare, and, as you say, normallly used on single groups. Hacking or complex counting code doesn't seem appropriate, even though it would be technically correct.

We could also limit this command to work only on single selections, i.e., offer it only in the menu when a single group tile is selected. Generally, in the UI, I should offer only the buttons that are meaningful for the current selection, and hide/grey out the others. This will be a nice long-term improvement for the editor.

-- Simon

[Simon]

Mirroring of tiles and rotating of tiles are now undoable.

The Hover parallel class hierarchy is ditched. The editor is refactored to work directly with sets of iterators to tile occurrences. This wasn't strictly necessary, but I've looked forward to this since the beginning of undo.

These were two wholesome evenings with solid progress. Now it's time for early sleeping.



To do:

• It must become undoable to move baby anteater on top of mom, thereby moving mom anteater below baby, but ideally without having to assume in the usercode that they're anteaters.
  

Nice to have:

• Resizing of the map must become undoable. In a pinch, we can clear the undo stack if we want to rush the feature's release, but it's better if the resizing is undoable.
  
• Clicking the in-editor button for new level: This may or may not be undoable, would be nice-to-have. Epecially if we make resizing undoable, this shouldn't be hard to make undoable, too.
  

-- Simon

[Simon]

Started with undoable resizing, because it's easier than z-ordering tiles. But haven't used this newly written Undoable yet.

Mouse hovering has new bugs since ditching the Hover class hierarchy. The editor will offer the wrong tile during mouse hovering if several tiles overlap. Mouse hovering is a nontrivial mixture of checking which tiles' bounding rectangles are under the cursor and which tiles' opaque pixels are under the cursor. The code before was complicated, a rewrite might have been good anyway, but I was too aggressive.

-- Simon

[namida]

Is the opaque pixel check necessary? As far as I'm aware, neither NL editor has had this check, and it has not been too difficult to deal with. I will note that the current one at least (not sure about the old one) has a priority invert key.

[Simon]

From experience, both checks are necessary for the user experience; I'm really annoyed about the mishovering that this bug reintroduced.

When the mouse is on a solid pixel, this pixel's tile should be selected. Reason: If we went by the tiles' bounding boxes, we would select a very large and thin tile instead, e.g., we select a Crystal-mesh that overlays filled smaller tiles, even though the mouse points through the holes of the mesh to the filled tiles behind.

When the mouse points to air, we select the tiles by bounding box. Reason: If the thin mesh sits all alone in space, pointing somewhere within its bounding box will unambiguously mean the mesh.

During priority invert, I don't have such strong opinions what's best. I think I want to select the tile that produces the hindmost solid pixel (that we don't see), or, if the mouse points to air, the hindmost bounding box.

-- Simon

[namida]

When the mouse is on a solid pixel, this pixel's tile should be selected. Reason: If we went by the tiles' bounding boxes, we would select a very large and thin tile instead, e.g., we select a Crystal-mesh that overlays filled smaller tiles, even though the mouse points through the holes of the mesh to the filled tiles behind.

I would personally 100% expect to select the mesh in this case, and not have to specifically locate a pixel where the mesh is solid.

I would wonder though if this holds true for someone who's not used to LemEdit and/or the two NL editors.

[Simon]

Interesting, thanks. I'll have to philosophize over this. Bounding-box-only was old behavior during very early D Lix editor development. I spent extra time to introduce selection by solid pixel, exactly because I didn't want the mesh to snatch all the careful pointing to the stuff behind it. I.e., personal strong feeling.

Accompanying reasoning was that when we select by solid pixels first, we will always be able to point to the interesting tile, because tiles will only be in the level when they produce at least some visible pixels.

Still, a feeling should probably come before such a reason. I agree that it would be helpful to see what others feel.

-- Simon

[namida]

I should note that, in the case of a piece that isn't exactly rectangular, I have no strong feelings about what should happen if I click in the empty space around it. I'd lean slightly towards "it should be selected, even if another piece has solid pixels below the mouse", but not nearly as strongly in this case. I feel far more strongly about it in the mesh case, or in general, in cases where we're dealing with a hollow space inside the piece rather than empty space around it (but within the bounding rectangle).

[Simon]

Right, if the pieces are rectangular, selection by visible pixels and selecting by bounding box always produce the same result.

If there is space around a rectangular tile, then Lix, already during tile loading from disk, cuts the selection box to the smallest rectangle that contains all solid pixels. The only effect of the cut-away space is that it still affects how the tile aligns with the 16x16 grid.

Slopes have lots of empty space, but usually you put nothing into that space, to allow the lemmings to walk up the slope. Thus, slopes aren't good tiles to investigate this either.

Decorative mesh laid over terrain still seems the best example. Here, the mesh must be in the foreground.

Or a long diagonal pole that you put in the foreground, it has a huge bounding box.

Other very thin, concave tiles come to mind, e.g., the palm tree from L2 beach. But you'd probably put the palm tree in the background, not the foreground, and then it won't snatch your hovering even when the program goes only by bounding box.



Fixed the hovering regressions. All mouse-hovering in the Undo branch should behave exactly as in 0.9.34.

-- Simon