NEON-27: finalise review (Approved) + Godot uid files
Review verdict updated to Approved; all blocking issues and suggestions resolved and documented. Add auto-generated Godot .uid sidecar files for occlusion_policy.gd and occlusion_policy_test.gd.pull/35/head
parent
71232c36ef
commit
f1b4e50729
|
|
@ -0,0 +1 @@
|
|||
uid://dtq0253dpm3kg
|
||||
|
|
@ -0,0 +1 @@
|
|||
uid://1ypnjycsj3y5
|
||||
|
|
@ -3,34 +3,35 @@
|
|||
**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).
|
||||
**Follow-up:** Previously requested changes below are done and were re-reviewed against the updated branch state.
|
||||
|
||||
## Verdict
|
||||
|
||||
**Request changes**
|
||||
**Approve**
|
||||
|
||||
## 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.
|
||||
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. Re-review confirms the prior cleanup bug is fixed: occluders are now restored on the `_process()` early-return paths as well as on node exit, so transient target-resolution failures no longer strand transparent geometry. The plan and decomposition tracking docs have also been brought back into alignment with the implemented null-material behavior and shipped E1.M2 scope. Residual risk is low and limited to the manual runtime behavior that still needs Godot-side verification.
|
||||
|
||||
## 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/plans/NEON-27-implementation-plan.md` | **Matches** — technical approach now reflects the implemented three-way material handling, including the explicit null-material fallback. |
|
||||
| `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/module_dependency_register.md` | **Matches** — E1.M2 still correctly reads **In progress**, and the note now reflects follow + zoom + occlusion shipped with NEON-28 remaining. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — implementation tracking now includes `E1.M2` with current shipped scope and pointers. |
|
||||
| `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.
|
||||
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.** `_process()` now restores all occluders before both early returns (`camera/_state` unavailable and unresolved `follow_target_path`), so the fade state is cleared when camera updates cannot proceed.
|
||||
|
||||
## 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.~~ Done.
|
||||
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.~~ **Done.** The “Material override strategy” section now documents the three explicit cases: `StandardMaterial3D`, null material, and non-`StandardMaterial3D`.
|
||||
|
||||
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.~~ Done.
|
||||
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.~~ **Done.** The register note now says NEON-25 through NEON-27 are in repo, and the implementation alignment table now includes an `E1.M2` row with current scope and pointers.
|
||||
|
||||
## Nits
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue