Slice 1: port terrain brushes to Rust + command-system infra

Port the four terrain brush commands (shape / level / smooth / metal) plus
brush-settings to Rust and flip them to Rust-only via nativeCommandsOnly.

Command system:
- Heightmap brushes wrap their per-point writes in
  TerrainControl::set_height_map_func so the engine recalcs the touched area
  (face normals + LOS/pathing); raw add_height_map changes data but never
  recalcs. Metal needs no recalc.
- Undo/redo for Rust-only commands forward UndoCommand/RedoCommand to the native
  manager (Lua wasn't driving Rust's undo stack). Those control commands are now
  braced structs so they deserialize from the {className} payload.
- Native dispatch gated to the gadget (synced) state: every command reaches the
  gadget first, so the single shared native manager sees it once. Prevents
  cross-executed commands (e.g. SetMultipleCommandModeCommand) hitting it twice
  and desyncing the streaming/undo state.

Infrastructure:
- IO worker shell (boxed dyn IoJob / dyn IoOutcome seam, no job types yet) for
  off-engine-thread file/image work.
- In-engine integration test harness (SBC_TEST_SPEC-gated), driven by the native
  Update callin (replaces the old Lua widget:Update poll_io driver).
- Terrain brush tests assert the recalc signal (get_ground_normal), not just
  get_ground_height; terrain_drag_stroke covers streaming-mode grouping.

Docs: command-system + async-io design docs, porting conventions (stepdown,
comments), slice/review-queue status, and a todo.md for deferred work.

Known issue: set_height_map_func recalc doesn't fire on undo (stale normals) —
engine-side, tracked separately; terrain_shape_brush is red until it lands.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: gajop

docs/design/async-io.md

@@ -0,0 +1,87 @@
+---
+name: Async IO
+description: How SpringBoard runs file IO and image work off the engine thread
+---
+
+# Async IO
+
+Some commands do **file IO** and **image decode/encode** — heightmap import /
+export, texture export, map compile. That work must not run on the engine thread
+(it would stall the frame), and the file/image work must not touch the engine
+(it runs on another thread, where engine calls are invalid).
+
+One rule keeps these apart:
+
+> **The background thread never touches the engine. The engine thread never does file IO.**
+
+## The worker
+
+A single background worker thread, owned by an `IoWorker` created once at plugin
+init and living for the whole plugin lifetime. Two channels connect it to the
+engine thread: jobs go out, outcomes come back.
+
+The worker is **feature-agnostic**. It moves a `Box<dyn IoJob>` to the thread and
+a `Box<dyn IoOutcome>` back, knowing nothing about what either does. Each feature
+slice defines its own job and outcome types in its own module, so adding one
+never edits the worker.
+
+Two trait bounds enforce the rule at compile time:
+
+- `IoJob: Send` and owns its data — it has **no engine handle**, so the worker
+  physically cannot reach the engine.
+- `IoOutcome::apply(&mut SBC)` takes the engine handle, and only runs back on the
+  engine thread, where engine calls are valid.
+
+## The three phases
+
+Take heightmap export as the worked example: read every height, encode a PNG,
+write it to disk.
+
+```mermaid
+sequenceDiagram
+    participant Cmd as Command (engine thread)
+    participant Worker as IoWorker (bg thread)
+    participant Poll as poll_io (engine thread)
+
+    Note over Cmd: 1. submit
+    Cmd->>Cmd: read heights into an owned buffer<br/>(fast memory read, no IO)
+    Cmd->>Worker: submit(Box<dyn IoJob>)
+
+    Note over Worker: 2. run (no engine access)
+    Worker->>Worker: encode PNG + write file<br/>on owned buffers only
+    Worker-->>Poll: Box<dyn IoOutcome> (queued)
+
+    Note over Poll: 3. apply
+    Poll->>Poll: drain ready outcomes
+    Poll->>Poll: outcome.apply(&mut SBC)<br/>(engine calls valid here)
+```
+
+1. **Submit (engine thread, on command dispatch).** Read whatever engine data
+   the job needs into an owned buffer — a fast memory read, not IO. Build an
+   `IoJob` (owns only `Send` data, holds no engine handle) and `submit` it.
+2. **Run (worker thread).** `IoJob::run` does the file read/write and image
+   decode/encode on owned buffers only, returning a `Box<dyn IoOutcome>`. No
+   engine access — the missing handle makes that a compile-time guarantee. A
+   failed job still returns an outcome that applies as a logging no-op.
+3. **Apply (engine thread, on drain).** `IoOutcome::apply(&mut SBC)` applies the
+   effect (e.g. `set_height_map` for import) where engine calls are valid.
+
+Import is the mirror image: the job reads + decodes a file, the outcome writes
+the decoded heights into the engine.
+
+## The drain point
+
+`SBC::update` (the native `Update` callin — unsynced, once per render frame)
+drains any ready outcomes and applies them. Unsynced rather than `game_frame`
+because IO shouldn't be tied to the sim clock: `update` fires at a steady cadence
+regardless of pause or game speed.
+
+## Status
+
+The worker shell, the `IoJob` / `IoOutcome` seam, the `poll_io` handler, and the
+`widget:Update` driver all landed with slice 1 as common infrastructure, with no
+job types wired. The first concrete job/outcome types land with the heightmap
+slice (import / export); adding them does not touch the shell.
+
+See also: [Command system](command-system.md) — async IO is kicked off by command
+dispatch and applied on the same boundary the command system runs on.

docs/porting/01-slices.md

@@ -30,7 +30,7 @@ spread across the slices they belong to, not deferred as a catch-all.
 | # | Slice | Status |
 |--:|-------|--------|
 | 0 | [Command infrastructure](#0-command-infrastructure) — trait + dispatch + undo/redo + compound + bridge + widget-notify | done (stable) |
-| 1 | [Terrain](#1-terrain) — shape / level / smooth / metal brushes | done (wip; flipped to Rust-only) |
+| 1 | [Terrain](#1-terrain) — shape / level / smooth / metal brushes | review (in stable; flipped to Rust-only; heightmap recalc fixed via `set_height_map_func`) |
 | 2 | [Heightmap](#2-heightmap) — load (sync) + import / export (async IO) | todo |
 | 3 | [Map settings](#3-map-settings) — sun / atmosphere / water / map-rendering | todo (setters bound; undo needs gl getters) |
 | 4 | [Textures](#4-textures) — diffuse / shading / terrain texture / cache + grass + DNTS | blocked (texture-atlas GL) |
@@ -67,9 +67,9 @@ This slice delivered it.
 control commands). Transport (`{tag, data}` decode) lives at the `SBC` boundary
 in [sbc.rs](../../native/src/sbc/sbc.rs).
 
-Convention: each slice is a directory; `mod.rs` files only wire submodules, never
-hold code. Feature-slice managers (e.g. terrain's `TerrainManager`) construct
-through `sbc.rs`.
+Tree-layout conventions (slice = directory, thin `mod.rs`, etc.) are in
+[conventions.md](conventions.md#code-structure). Feature-slice managers (e.g.
+terrain's `TerrainManager`) construct through `sbc.rs`.
 
 **Lua files**:
 - [scen_edit/command/command.lua](../../scen_edit/command/command.lua) — base class
@@ -78,12 +78,18 @@ through `sbc.rs`.
 
 The bridge sends every command to Rust via `Spring.InvokeNativeModule(json.encode(msg:serialize()))`. Lua execution **also** runs (parallel) unless the class name appears in `nativeCommandsOnly`. Slice 0 leaves that allowlist empty — feature slices populate it as they land.
 
-**Deferred to slice 1 (first feature slice that flips a command to Rust-only):**
+**Still open — required for full 1:1 once Rust is the *only* executor of undo/redo:**
 
-These exist on the Lua side and are required for full 1:1 parity once Rust is the only executor. Until then Lua handles them; slice 0 leaves them out by design, not by omission.
+These exist on the Lua side and are needed once Rust owns the undo stack. They
+are **not** yet ported. Slice 1 did not need them: flipping a command via
+`nativeCommandsOnly` only suppresses Lua's `cmd:execute()`
+([command_manager.lua](../../scen_edit/command/command_manager.lua) line ~129) —
+the `undoListAdd` / `notify` path (line ~133) is **not** gated, so during the
+parallel period Lua still owns undo/redo bookkeeping and the widget notify. These
+land with the slice that first moves the undo stack itself to Rust.
 
 - `__cmd_id` allocation on every executed command (Rust counterpart to Lua's `idCount`).
-- Widget notify after `execute` / `undo` / `redo` / `clear_*` / `undo_list_add` (when it pops oldest) — Lua dispatches `WidgetCommandExecuted` / `WidgetCommandUndo` / `WidgetCommandRedo` / `WidgetCommandClearUndoStack` / `WidgetCommandClearRedoStack` / `WidgetCommandRemoveFirstUndo` to the widget. Rust needs the equivalent via `send_lua_uimsg`, matching `scen_edit/message/message_manager.lua`'s prefix-framed wire format.
+- Widget notify after `execute` / `undo` / `redo` / `clear_*` / `undo_list_add` (when it pops oldest) — Lua dispatches `WidgetCommandExecuted` / `WidgetCommandUndo` / `WidgetCommandRedo` / `WidgetCommandClearUndoStack` / `WidgetCommandClearRedoStack` / `WidgetCommandRemoveFirstUndo` to the widget. Rust needs the equivalent via `send_lua_uimsg` (a `lua_bridge` module — exists in wip, not yet in stable), matching `scen_edit/message/message_manager.lua`'s prefix-framed wire format.
 - `display()` method on commands (Lua returns `self.className`; `CompoundCommand` returns the first sub-command's display). Required for the `display` field of `WidgetCommandExecuted`.
 
 **Out of scope for slice 0** (handled by their feature slices):
@@ -96,20 +102,26 @@ These exist on the Lua side and are required for full 1:1 parity once Rust is th
 
 Brush-based heightmap and metal-map editing. Engine-side via `Spring.SetHeightMap` / `Spring.SetMetalAmount` / `Spring.AddHeightMap`, all bound natively.
 
-**Status:** done in wip — 4 brush commands + brush-settings, flipped to Rust-only via `nativeCommandsOnly`. Needs in-game verification (the slice-0 widget-notify path is exercised here).
+**Status:** in stable, awaiting review — four brush commands + brush-settings,
+flipped to Rust-only. Also lands shared infra: the IO-worker shell (no job types
+yet), the in-engine test harness, and the Lua `poll_io` driver.
+
+The three heightmap brushes wrap their writes in `TerrainControl::set_height_map_func`
+so the engine recalcs (without it, heights change in data but the terrain doesn't
+move) — see `SBC_PORT_MISSING_BINDINGS.md`. Metal needs no recalc.
 
 **Model:**
 - [scen_edit/model/terrain_manager.lua](../../scen_edit/model/terrain_manager.lua) — brush state, listeners, generated metadata
 - [scen_edit/model/heightmap.lua](../../scen_edit/model/heightmap.lua)
 - [scen_edit/model/rendering/texture_undo_stack.lua](../../scen_edit/model/rendering/texture_undo_stack.lua) (also used by textures)
 
-**Commands:**
-- [abstract_terrain_modify_command.lua](../../scen_edit/command/abstract_terrain_modify_command.lua) — shared base
-- [terrain_shape_modify_command.lua](../../scen_edit/command/terrain_shape_modify_command.lua)
-- [terrain_level_command.lua](../../scen_edit/command/terrain_level_command.lua)
-- [terrain_smooth_command.lua](../../scen_edit/command/terrain_smooth_command.lua)
-- [terrain_metal_command.lua](../../scen_edit/command/terrain_metal_command.lua)
-- [set_heightmap_brush_command.lua](../../scen_edit/command/set_heightmap_brush_command.lua)
+**Commands (Lua source → Rust):**
+- [abstract_terrain_modify_command.lua](../../scen_edit/command/abstract_terrain_modify_command.lua) — shared base; its brush-stamp logic became [brush_modify.rs](../../native/src/sbc/commands/heightmap/brush_modify.rs) (composition via closures, not inheritance) + [brush_filter_generator.rs](../../native/src/sbc/commands/heightmap/brush_filter_generator.rs)
+- [terrain_shape_modify_command.lua](../../scen_edit/command/terrain_shape_modify_command.lua) → [terrain_shape_modify_command.rs](../../native/src/sbc/commands/heightmap/terrain_shape_modify_command.rs)
+- [terrain_level_command.lua](../../scen_edit/command/terrain_level_command.lua) → [terrain_level_command.rs](../../native/src/sbc/commands/heightmap/terrain_level_command.rs)
+- [terrain_smooth_command.lua](../../scen_edit/command/terrain_smooth_command.lua) → [terrain_smooth_command.rs](../../native/src/sbc/commands/heightmap/terrain_smooth_command.rs)
+- [terrain_metal_command.lua](../../scen_edit/command/terrain_metal_command.lua) → [terrain_metal_command.rs](../../native/src/sbc/commands/heightmap/terrain_metal_command.rs)
+- [set_heightmap_brush_command.lua](../../scen_edit/command/set_heightmap_brush_command.lua) → [set_heightmap_brush_command.rs](../../native/src/sbc/commands/set_heightmap_brush_command.rs) (registers the greyscale brush shape in [terrain_manager.rs](../../native/src/sbc/commands/heightmap/terrain_manager.rs); **not** flipped to Rust-only — Lua still needs the shape for its own preview)
 
 ---
 
@@ -126,8 +138,10 @@ crate), replacing what the spring-launcher used to do over IPC.
   thread. Port directly.
 - **Import / export (async IO):** decode/encode an image file. File IO + image
   work runs on a background worker thread (must not touch the engine); the engine
-  thread applies/reads heights. Needs the IO-worker + `widget:Update` poll (see
-  "Async IO" below).
+  thread applies/reads heights. The IO-worker shell + the `widget:Update` poll
+  already exist (landed with slice 1 as common infra) — this slice just adds the
+  first concrete `IoJob`/`IoOutcome` types (and the `image` crate dep). See
+  [docs/design/async-io.md](../design/async-io.md).
 
 **Model:**
 - [scen_edit/model/heightmap.lua](../../scen_edit/model/heightmap.lua) (shared with slice 1)
@@ -323,44 +337,9 @@ pure project logic (no engine API).
 
 ---
 
-## Widget commands — not a slice
-
-Widget commands aren't deferred as a catch-all. Each is part of the feature slice
-it serves and is listed there:
-
-- `widget_terrain_change_texture_command` → slice 4 (textures)
-- `widget_follow_unit_command` → slice 5 (objects); genuinely widget-only, stays Lua
-- `widget_command_executed` / undo / redo / clear / remove-first → slice 0
-  (the command-system widget-notify path; already handled)
-
-The remaining genuinely widget-only ones (display text, unit say, draw texture,
-execute-unsynced-action) are unsynced UI helpers; they stay Lua until the view
-layer moves to Rust (Phase 2/3). Listed for completeness:
-- [widget_display_text.lua](../../scen_edit/command/widget_display_text.lua)
-- [widget_unit_say_command.lua](../../scen_edit/command/widget_unit_say_command.lua)
-- [widget_draw_texture_command.lua](../../scen_edit/command/widget_draw_texture_command.lua)
-- [widget_execute_unsynced_action_command.lua](../../scen_edit/command/widget_execute_unsynced_action_command.lua)
-
-## Async IO (heightmap import/export, compile, image export)
-
-Import/export/compile do file IO + image decode/encode. Rule: **the background
-thread never touches the engine; the engine thread never does file IO.**
-
-- Engine thread (on command dispatch): read engine data into an owned buffer
-  (export) — fast memory read, no IO. Hand the buffer to the worker.
-- Background worker thread: file read/write + image decode/encode on owned
-  buffers only. No engine access.
-- Engine thread (on drain): apply results (e.g. `set_height_map` for import).
-
-Drain point: there's no native per-frame callin (see the engine note, Category 0),
-so a throttled `widget:Update` pokes the plugin via
-`InvokeNativeModule(json.encode({tag="poll_io"}))` and the plugin drains completed
-jobs on that call. `widget:Update` (not `gadget:GameFrame`) because GameFrame
-doesn't fire while paused and SBC is effectively always paused.
-
-## Where things go in Rust
+The async-IO model (heightmap import/export, compile, image export) is a design
+concern, not a slice — see [docs/design/async-io.md](../design/async-io.md). The
+worker shell landed with slice 1; the first job types land with slice 2.
 
-- Per-command Rust file: [native/src/sbc/commands/](../../native/src/sbc/commands/)`<slice>/`. Each file owns its serde struct, the `inventory::submit!` registration, and the execute/unexecute logic.
-- Per-manager Rust file: a per-slice `model`/manager module.
-- The command-system internals + the single public `commands_api` surface live under [native/src/sbc/commands/command_system/](../../native/src/sbc/commands/command_system/).
-- Cross-slice managers (texture undo stack, heightmap) live where the first slice that needs them puts them; later slices reuse.
+Where ported code lives in the tree is a porting convention — see
+[conventions.md](conventions.md#code-structure).

docs/porting/README.md

@@ -26,7 +26,15 @@ Phase 1 ports the model and command layers together in slices (one feature end-t
 
 Phase 2 is in progress independently of the Rust port (some Chili→RmlUi work landed earlier).
 
-Pending review items: [review-queue.md](review-queue.md). Rules: [conventions.md](conventions.md).
+Pending review items: [review-queue.md](review-queue.md). Rules: [conventions.md](conventions.md). Deferred improvements: [todo.md](todo.md).
+
+## Design docs
+
+Living documents describing how subsystems work (outlast the slice docs, which
+are purged once a slice lands):
+
+- [Command system](../design/command-system.md) — how editor actions become commands and execute
+- [Async IO](../design/async-io.md) — running file IO / image work off the engine thread
 
 ## Where things live
 

docs/porting/conventions.md

@@ -102,6 +102,14 @@ The registry is built at startup from `inventory::iter`. Adding a command = one
 
 If `inventory` becomes problematic (e.g. platforms where life-before-`main` link sections misbehave), the fallback is a `build.rs` that scans `commands/*.rs` and generates the registry. Same property: command files stay self-contained.
 
+**Where ported code lives in the tree:**
+
+- Per-command file under [native/src/sbc/commands/](../../native/src/sbc/commands/)`<slice>/` — owns its serde struct, its `inventory::submit!` registration, and its execute/unexecute logic.
+- Per-manager file: a per-slice `model`/manager module.
+- The command-system internals + the single public `commands_api` surface live under [native/src/sbc/commands/command_system/](../../native/src/sbc/commands/command_system/).
+- Cross-slice managers (texture undo stack, heightmap) live where the first slice that needs them puts them; later slices reuse.
+- Each slice is a directory; `mod.rs` files only wire submodules, never hold code.
+
 | Layer | Self-contained unit | Registration |
 |-------|--------------------|--------------|
 | Commands | one file per command | `inventory` keyed by class name |
@@ -142,10 +150,40 @@ If any check fails, fix it. **Don't pad the queue.** Three clean items beat ten
 - **Imports grouped**: std → external crates → crate-local, separated by blank lines.
 - **Errors carry context.** Never bare `?` that drops meaning.
 - **Tests live next to code**: `#[cfg(test)] mod tests` at the bottom of the file.
-- **No WHAT-comments.** Only WHY-comments survive review.
 - **Public surface minimal.** `pub(crate)` over `pub` when in doubt.
 - **Short functions.** If you scroll to read it, split it.
 
+### Stepdown ordering
+
+Write each file **top-down in call order**: the public entry point first, then
+the things it calls, then their callees, with private leaf helpers last. A reader
+scrolling top-to-bottom meets each function *before* the helpers it depends on —
+the file reads like a newspaper (headline first, detail below).
+
+- In a command file: the `impl Command` block (`execute` / `unexecute` — what the
+  framework calls) goes first, then its `stamp`/private methods, then leaf helpers
+  (`brush`, `generate_*`), then free functions, then the `register_command!` line.
+- Keep one `impl` block per type — don't split a type's methods across several
+  `impl` blocks to satisfy ordering; order the methods *within* the block instead.
+- Free helper functions go below the code that calls them; a leaf used by several
+  callers goes after the last of them.
+
+### Comments
+
+Comment the **why**, not the **what**. The code already says what it does; a
+comment earns its place only by explaining something the code can't: a non-obvious
+reason, a constraint, a surprising interaction, a deliberate deviation.
+
+- **No WHAT-comments.** `// loop over the points` above a `for` loop is noise.
+- **Write WHY-comments** for: why this approach over the obvious one, why an engine
+  call is wrapped a certain way, why a value is what it is, what invariant must
+  hold. (Example: the `set_height_map_func` wrapper comment explaining the recalc.)
+- **Doc-comments (`///`, `//!`)** state a contract — what a caller must know to use
+  the item correctly — not a restatement of the body.
+- **Keep them true.** A stale comment is worse than none; update or delete it when
+  the code moves. Design narrative belongs in `docs/design/`, not in long module
+  headers that drift.
+
 ## Tests
 
 - **Unit tests** (every item with non-trivial logic): co-located, cover happy path + error paths + edge cases (empty, boundary, large). Skip trivial getters.