From 5e16fdccc822396be3cd3b6b1a6b74b3eb3d6f67 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 8 Apr 2026 23:50:48 -0400 Subject: [PATCH] NEON-27: restore occluders on all _process early-return paths Fixes review blocking issue: when the follow target is null (freed, renamed, or path unresolved) or the Camera3D child is missing, _restore_all_occluders() is now called before returning so geometry cannot be left stuck semi-transparent. --- client/scripts/isometric_follow_camera.gd | 2 + docs/reviews/2026-04-08-NEON-27.md | 45 +++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 docs/reviews/2026-04-08-NEON-27.md diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index 01ccefd..74fd654 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -80,10 +80,12 @@ func _ready() -> void: func _process(delta: float) -> void: if camera == null or _state == null: + _restore_all_occluders() return var target: Node3D = _resolve_target() if target == null: # No eye/state update until the path resolves — avoids chasing a freed or miswired node. + _restore_all_occluders() _warn_missing_follow_target_once() return _warned_missing_follow_target = false diff --git a/docs/reviews/2026-04-08-NEON-27.md b/docs/reviews/2026-04-08-NEON-27.md new file mode 100644 index 0000000..29024c4 --- /dev/null +++ b/docs/reviews/2026-04-08-NEON-27.md @@ -0,0 +1,45 @@ +# Code review — NEON-27 (OcclusionPolicy) + +**Date:** 2026-04-08 +**Scope:** Branch `NEON-27-occlusion-policy` vs `origin/main` (NEON-27 occlusion policy resource, camera integration, scene wiring, tests, and docs). Issue **NEON-27**; no PR URL supplied. +**Base:** `origin/main` @ `1d891eb8bb42ccc5e8a74b4fcdb349b9165f3454` (merge-base with HEAD at review time). + +## Verdict + +**Request changes** + +## Summary + +The branch adds a data-driven `OcclusionPolicy` resource, wires it into `IsometricFollowCamera`, tags the prototype obstacle as an `"occluder"`, and documents the prototype readability gate in the module doc. The main behavior matches the intended client-local camera scope and the implementation is otherwise straightforward. Risk is still moderate because the fade state is stateful: one cleanup edge case can leave geometry stuck transparent after the follow target disappears, and the review also found two documentation follow-ups so the saved plan/decomposition docs stay aligned with the chosen implementation. + +## Documentation checked + +| Document | Result | +|----------|--------| +| `docs/plans/NEON-27-implementation-plan.md` | **Partially matches** — overall approach, exported resource, scene wiring, tests, and module-doc update all align; however the “Material override strategy” paragraph still says `null` materials are skipped, while the implementation and Decisions table create a transparent `StandardMaterial3D` for that case. | +| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — implementation snapshot and readability gate note align with the code and scene changes. | +| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E1.M2 status remains appropriately **In progress**, but the note still says occlusion policy “remain[s]”; after this merge it should mention occlusion shipped and only integration hardening remains. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — the tracking table still omits `E1.M2` despite NEON-25/26/27 landing meaningful implementation; add or update that row after merge. | +| `docs/decomposition/modules/client_server_authority.md` | **Matches** — the change stays client-local and does not introduce any server use of camera pose. | + +## Blocking issues + +1. ~~**`client/scripts/isometric_follow_camera.gd` — occluders are never restored when the follow target disappears mid-occlusion.** `_process()` returns immediately when `follow_target_path` no longer resolves, but that path does not call `_restore_all_occluders()`. If the player target is freed, renamed, or otherwise temporarily unavailable while an obstacle is faded, the saved surface overrides stay applied until the camera node exits the tree. Because this leaves world geometry stuck semi-transparent in a recoverable runtime failure mode, it should be fixed before merge by restoring all active occluders on the early-return path (and similarly on any other “cannot update camera this frame” exit that can strand state).~~ Done. + +## Suggestions + +1. **`docs/plans/NEON-27-implementation-plan.md` — reconcile the null-material story.** The plan currently says null materials are skipped in the “Material override strategy” section, but the implementation and Decisions table intentionally create a transparent `StandardMaterial3D` for null-material surfaces. Update the technical-approach prose so the plan is a single source of truth. + +2. **`docs/decomposition/modules/module_dependency_register.md` and `docs/decomposition/modules/documentation_and_implementation_alignment.md` — refresh implementation tracking after merge.** `E1.M2` has meaningful shipped work now (follow, zoom, occlusion), so the register note and tracking table should reflect that status explicitly instead of implying occlusion is still pending or leaving the implementation table empty for this module. + +## Nits + +None. + +## Verification + +- From `client/`: Godot 4.6 headless import, then GdUnit: + `godot --headless --import --path . --quit-after 8` + `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test` +- Manual: run the prototype scene, walk the player behind the `Obstacle`, confirm it fades and restores normally, then test the failure path by making `follow_target_path` unresolved or removing the target while the obstacle is faded; geometry should restore immediately instead of staying transparent. +- Review environment note: I could not run the automated Godot command here because no `godot` binary was available on `PATH`.