51 lines
4.3 KiB
Markdown
51 lines
4.3 KiB
Markdown
# Code review — NEON-26 (ZoomBandConfig + discrete zoom)
|
||
|
||
**Date:** 2026-04-08
|
||
**Scope:** Branch `NEON-26-zoom-band-config-discrete-input` vs `origin/main` (NEON-26 zoom bands + follow-up `.tres` tuning commits). Issue **NEON-26**; no PR URL supplied.
|
||
**Base:** `origin/main` @ `88653292104eddfc1db702f97b02a088fb80c1c7` (merge-base with HEAD at review time).
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## 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 or empty. **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. Follow-up tuning of **`isometric_zoom_bands.tres`** (band count and distances) is data-only and acceptable on the same story.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| `docs/plans/NEON-26-implementation-plan.md` | **Matches** intent and acceptance criteria. Minor **partial match**: the plan’s technical approach mentions `@export var zoom_band_config: ZoomBandConfig`; the implementation correctly uses **`Resource` + `get_script()`** comparison to avoid `class_name`, consistent with `godot-client-script-organization` and NEON-14 headless notes. Consider a one-line clarification in the plan that the export is untyped/`Resource` by design. |
|
||
| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — implementation snapshot updated for NEON-26 (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 (NEON-27/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.
|
||
|
||
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).
|
||
|
||
3. **Plan vs implementation (typing)** — As above, add a brief note to **`NEON-26-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.
|
||
|
||
## 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).
|
||
|
||
- **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.
|
||
|
||
- **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.
|
||
|
||
## 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`
|
||
- **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`**.
|