# Code review — NEO-16 (ZoomBandConfig + discrete zoom)

**Date:** 2026-04-08  
**Scope:** Branch `NEO-16-zoom-band-config-discrete-input` vs `origin/main` (NEO-16 zoom bands + follow-up `.tres` tuning commits). Issue **NEO-16**; no PR URL supplied.  
**Base:** `origin/main` @ `88653292104eddfc1db702f97b02a088fb80c1c7` (merge-base with HEAD at review time).  
**Follow-up:** Suggestions and nits below were implemented in-repo (README, plan typing note, positive-distance validation, `_sync_camera_state(effective_distance)`, telemetry TODO wording) and re-reviewed after `ff83081`.

## Verdict

**Approve**

## Summary

The branch delivers **data-driven discrete zoom** via `ZoomBandConfig`, **InputMap** actions, **`_unhandled_input`** on the follow rig, and **`CameraState`** updates for effective distance and band index, with **clamp** semantics and **`follow_distance`** fallback when config is missing, empty, or invalid. **GdUnit** coverage was added for the resource and **`effective_follow_distance`**. Module and dependency-register docs were updated. Overall risk is **low**: client-local camera only, no server coupling; logic is straightforward. Re-review after the follow-up commit found **no additional issues**.

## Documentation checked

| Document | Result |
|----------|--------|
| `docs/plans/NEO-16-implementation-plan.md` | **Matches** — technical approach updated to document **`Resource` export + `get_script()`** and positive-distance fallback; aligns with implementation. |
| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — implementation snapshot updated for NEO-16 (config path, input actions, distance-only bands, fallback, signal + E9.M1 TODO). |
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M2 note reflects zoom bands shipped. |
| `docs/decomposition/modules/client_server_authority.md` (camera client-local) | **Matches** — no server use of camera pose introduced. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A** for mandatory table edits; E1.M2 status remains “In progress” appropriately (NEO-17/28 open). |

## Blocking issues

None.

## Suggestions

1. ~~**`client/README.md` (testing section)** — The bullet that lists covered scripts is now slightly stale: it does not mention **`isometric_follow_camera.gd`**, **`camera_state.gd`**, or **`zoom_band_config.gd`**, though **`client/test/`** includes suites for them. Per [testing-expectations](../../.cursor/rules/testing-expectations.md), consider extending that sentence so contributors know camera/zoom logic is under GdUnit.~~ **Done.** Scope bullet extended; short **Camera zoom** note added for Input Map / layout remapping.

2. ~~**`ZoomBandConfig` validation** — Designer-editable **`band_distances`** could theoretically include **non-positive** values, which would produce odd framing. Optional: `_validate_property` or a short runtime guard in the rig when loading config (non-blocking for prototype).~~ **Done.** **`all_band_distances_positive()`** on the resource; **`_zoom_config_valid()`** and **`effective_follow_distance`** fall back when any distance is ≤ 0; tests added.

3. ~~**Plan vs implementation (typing)** — As above, add a brief note to **`NEO-16-implementation-plan.md`** that **`zoom_band_config` is exported as `Resource`** (or untyped) with script identity checked at runtime, so future readers are not confused by the plan’s “ZoomBandConfig” type mention.~~ **Done.** Technical approach step 2 rewritten accordingly.

## Nits

- ~~**`_current_follow_distance()`** is invoked multiple times per **`_process`** / **`_sync_camera_state`**; a single local **`var d`** per frame would avoid redundant calls (micro-optimization, clarity).~~ **Done.** **`_sync_camera_state(focus, effective_distance)`** takes the distance computed once per frame (and **`_ready`** passes **`dist0`**).

- ~~**Keyboard zoom:** **`=`** without **Shift** may not match all layouts’ “+” expectations; wheel and keypad bindings mitigate this. Document in README or input remap note if QA reports gaps.~~ **Done.** Documented under **Camera zoom** in **`client/README.md`**.

- ~~**Signal name vs telemetry TODO:** Comment mentions **`camera_zoom_changed`**; signal is **`zoom_band_changed`** — harmless but slightly inconsistent wording for whoever wires E9.M1 later.~~ **Done.** TODO now references mapping product telemetry to **`zoom_band_changed`**.

## Verification

- From `client/`: Godot **4.6** headless import (if needed) then GdUnit:  
  `godot --headless --import --path . --quit-after 8`  
  `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test`
- **Observed on re-review:** GdUnit passed locally: **28 test cases, 0 failures**.
- **Manual:** F5, cycle zoom (wheel, `=`, `-`, keypad), confirm framing and that **`CameraState`**-consumers (if any) see band index + distance; confirm **`zoom_band_config` cleared** falls back to **`follow_distance`**.
