From 487b3aa5b4ce90c4f72c99a506be028ca9439716 Mon Sep 17 00:00:00 2001 From: MelekTaus Date: Wed, 23 Sep 2026 08:56:17 +0200 Subject: [PATCH] Painting over identical tiles no longer adds an empty undo step (#212) A rectangle drawn over cells that already hold exactly that tile pushed a command that changed nothing. Undo then seemed to do nothing and the project was marked as modified. PaintTilesCommand.changes() was meant to prevent that, but nothing called it since #128. - New paintTiles() in tilemapCommands.ts returns the command for a finished stroke or fill, or null when no cell changes. It is the first caller of PaintTilesCommand.changes(). - All three paint paths in the Scene View go through it: brush and eraser, rectangle (both in endStroke) and flood fill (fillFrom). - Flood fill was not affected, because floodFill already returns [] for the same tile. The guard is there anyway, as the issue asked. - A rectangle that changes even one cell stays a normal undo step. Tests: - ./check.sh: 145 files, 2190 tests, CHECK EXIT 0 (before 2187). - 3 new unit tests in tests/core/tilemap.test.ts: same rectangle twice gives no command, a wider rectangle still does, a fill that finds nothing gives no command. - 1 new e2e check in 40d: the same rectangle again, then one undo must take 9 cells to 0. A redo restores them for later blocks. - GM_STEPS=39,40 on the final build: ALL PASSED, 81 checks. - Counter-probe with changes(...) || true: exactly the two unit tests fail, and in the browser exactly the new check fails with {"afterUndo":9}. Original restored via cmp (IDENTICAL), rebuilt. - Editor suite check total: CI measured 1312 at 0e85856, new total 1313 (calculated) in README and wiki. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- src/core/commands/tilemapCommands.ts | 15 +++++++++++ src/editor/viewport/SceneViewport.ts | 31 +++++++++-------------- tests/core/tilemap.test.ts | 37 ++++++++++++++++++++++++++++ tools/e2e/40-tilemap.mjs | 15 +++++++++++ 5 files changed, 80 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 6ab7f662..84948c58 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ npm run build # writes dist/ and regenerates index.html; the folder is wh The browser suite and the three example games need a machine with a GPU: ```bash -node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1312 checks (GM_STEPS=20 for one step) +node tools/seed-e2e.mjs && node tools/e2e.mjs # the editor, 1313 checks (GM_STEPS=20 for one step) # every tool signs in first – see the Accounts page node tools/seed-jumprun.mjs && node tools/smoke-jumprun.mjs node tools/seed-tetris.mjs && node tools/smoke-tetris.mjs diff --git a/src/core/commands/tilemapCommands.ts b/src/core/commands/tilemapCommands.ts index 7b118b78..5aba7877 100644 --- a/src/core/commands/tilemapCommands.ts +++ b/src/core/commands/tilemapCommands.ts @@ -82,6 +82,21 @@ export class PaintTilesCommand implements Command { } } +/** + * The command for a finished stroke or fill, or null when it would change no cell: such a step + * would undo nothing and still count as an edit of the project (#212). + */ +export function paintTiles( + layer: TilemapLayer, + assetId: string, + edits: readonly CellEdit[], + label?: string, +): PaintTilesCommand | null { + return PaintTilesCommand.changes(layer, edits) + ? new PaintTilesCommand(assetId, layer.id, edits, label) + : null; +} + /** * Where a placed tilemap's origin goes when `edge` gains `count` cells, starting from `from`, so the * painted cells stay where they are in the scene (#136). The shift is worked out in the tilemap's diff --git a/src/editor/viewport/SceneViewport.ts b/src/editor/viewport/SceneViewport.ts index 019770d5..019f644e 100644 --- a/src/editor/viewport/SceneViewport.ts +++ b/src/editor/viewport/SceneViewport.ts @@ -10,7 +10,7 @@ import { PlaceAssetCommand, SetOverrideCommand, } from '../../core/commands/sceneObjectCommands'; -import { type CellEdit, PaintTilesCommand } from '../../core/commands/tilemapCommands'; +import { type CellEdit, paintTiles } from '../../core/commands/tilemapCommands'; import type { TilemapAsset, TilemapLayer } from '../../core/model/Asset'; import { hasSceneIcon } from '../../core/model/AssetTypes'; import { findObject, subtreeRoots } from '../../core/model/hierarchy'; @@ -503,15 +503,13 @@ export class SceneViewport extends FreeViewport { return; } const cells = floodFill(target.item.props, layer, at, tileId); - if (cells.length > 0) - this.state.execute( - new PaintTilesCommand( - target.asset.id, - target.layerId, - cells.map((cell) => ({ ...cell, tileId })), - 'Fill tiles', - ), - ); + const command = paintTiles( + layer, + target.asset.id, + cells.map((cell) => ({ ...cell, tileId })), + 'Fill tiles', + ); + if (command) this.state.execute(command); } private startStroke( @@ -588,15 +586,10 @@ export class SceneViewport extends FreeViewport { tileId: stroke.tileId, })) : stroke.edits; - if (edits.length > 0) - this.state.execute( - new PaintTilesCommand( - stroke.assetId, - stroke.layerId, - edits, - stroke.tool === 'erase' ? 'Erase tiles' : 'Paint tiles', - ), - ); + const command = + layer && + paintTiles(layer, stroke.assetId, edits, stroke.tool === 'erase' ? 'Erase tiles' : 'Paint tiles'); + if (command) this.state.execute(command); else this.state.preview(); this.redraw(); } diff --git a/tests/core/tilemap.test.ts b/tests/core/tilemap.test.ts index e83ddc39..41c5faf5 100644 --- a/tests/core/tilemap.test.ts +++ b/tests/core/tilemap.test.ts @@ -10,6 +10,7 @@ import { DuplicateTilemapLayerCommand, MoveTilemapLayerCommand, PaintTilesCommand, + paintTiles, RemoveTilemapLayerCommand, RenameTilemapLayerCommand, SetTilemapLayerHiddenCommand, @@ -26,6 +27,7 @@ import { cellOrigin, cellsAlong, cellsInRect, + floodFill, inGrid, layerTiles, neighboursOf, @@ -151,6 +153,41 @@ describe('painting a tilemap', () => { expect(PaintTilesCommand.changes(layer, [{ x: 5, y: 5, tileId: f.tileId }])).toBe(true); }); + it('makes no undo step for a rectangle over cells that already hold its tile', () => { + const f = makeTilemap(); + const box = cellsInRect(f.assetOf().props, { x: 0, y: 0 }, { x: 2, y: 2 }); + f.paint(box); + const again = box.map((at) => ({ ...at, tileId: f.tileId })); + expect(paintTiles(f.assetOf().layers[0]!, f.tilemapId, again)).toBeNull(); + // The one undo left takes back the first rectangle, not an empty second one. + f.stack.undo(); + expect(cellsOf(f.project, f.tilemapId)).toEqual({}); + }); + + it('still makes one when a single cell of the rectangle changes', () => { + const f = makeTilemap(); + f.paint(cellsInRect(f.assetOf().props, { x: 0, y: 0 }, { x: 1, y: 1 })); + const wider = cellsInRect(f.assetOf().props, { x: 0, y: 0 }, { x: 2, y: 1 }); + const command = paintTiles( + f.assetOf().layers[0]!, + f.tilemapId, + wider.map((at) => ({ ...at, tileId: f.tileId })), + ); + expect(command).not.toBeNull(); + f.stack.execute(command!); + expect(Object.keys(cellsOf(f.project, f.tilemapId))).toHaveLength(6); + f.stack.undo(); + expect(Object.keys(cellsOf(f.project, f.tilemapId)).sort()).toEqual(['0,0', '0,1', '1,0', '1,1']); + }); + + it('makes no undo step for a fill that finds nothing to fill', () => { + const f = makeTilemap(); + f.paint([{ x: 0, y: 0 }]); + const layer = f.assetOf().layers[0]!; + expect(floodFill(f.assetOf().props, layer, { x: 0, y: 0 }, f.tileId)).toEqual([]); + expect(paintTiles(layer, f.tilemapId, [])).toBeNull(); + }); + it('covers the cells between two pointer events', () => { expect(cellsAlong({ x: 0, y: 0 }, { x: 3, y: 0 })).toEqual([ { x: 1, y: 0 }, diff --git a/tools/e2e/40-tilemap.mjs b/tools/e2e/40-tilemap.mjs index e2747db0..fc418606 100644 --- a/tools/e2e/40-tilemap.mjs +++ b/tools/e2e/40-tilemap.mjs @@ -117,6 +117,7 @@ export async function tilemap(t) { shot, settle, undo, + redo, select, } = t; const { saved, api, navigate, projectId } = t; @@ -287,6 +288,20 @@ export async function tilemap(t) { Object.keys(cells).length === 9 && cells['1,1'] === 'at-grass' && cells['2,2'] === 'at-grass', JSON.stringify(Object.keys(cells).sort()), ); + // The same box again changes no cell, so it leaves no step behind (#212): the one undo takes + // back the first rectangle, and redo brings it back for the blocks below. + await drag(corner.x, corner.y, far.x, far.y); + await settle(); + await undo(); + await settle(); + const afterUndo = Object.keys(await painted()).length; + await redo(); + await settle(); + check( + 'a rectangle over cells that already hold its tile is no undo step', + afterUndo === 0 && Object.keys(await painted()).length === 9, + JSON.stringify({ afterUndo, redone: Object.keys(await painted()).length }), + ); // 40e. The pixels. While the animation runs the block under the pointer changes between the two // steps, so it is cut down to one for the reading and put back afterwards (as step 39 does).