|
| 1 | +# 2026-07-13 — Fix 2.5D Selectable Hit Testing and Ground Projection Indicator |
| 2 | + |
| 3 | +- Date: 2026-07-13 |
| 4 | +- GitHub Issue: https://github.com/Apptive-Game-Team/WordOnlineClient/issues/350 |
| 5 | +- Status: Implemented; manual Play Mode verification pending |
| 6 | + |
| 7 | +## Goal |
| 8 | + |
| 9 | +- Make pointer hover/click follow the visible `SpriteRenderer` reliably under the 2.5D camera, including rotated, scaled, animated, and y != 0 objects. |
| 10 | +- Make selection collider ownership deterministic instead of accidentally reusing an unrelated child collider. |
| 11 | +- Visualize the selected world position and its ground projection `(x, 0, z)` with a vertical line so the selected x/z coordinate is unambiguous. |
| 12 | +- Preserve the existing `CardInputSender` protocol while sending an object's actual world position and using `(x, 0, z)` only for ground visualization. |
| 13 | + |
| 14 | +## Non-goals |
| 15 | + |
| 16 | +- Do not change server DTOs, STOMP destinations, range calculations, or magic validation behavior. |
| 17 | +- Do not redesign the 2.5D camera or mass-rewrite prefab hierarchies. |
| 18 | +- Do not replace every gameplay collider with a new physics abstraction. |
| 19 | +- Do not change unrelated `feature/345` audio work or carry its dirty workspace changes into this branch. |
| 20 | + |
| 21 | +## Context / Constraints |
| 22 | + |
| 23 | +- Owning repository: `WordOnlineClient` (`client/`); no cross-repository contract change is expected. |
| 24 | +- Work branch: `fix/350`, based on local `main` at `70ce401`, in a dedicated Git worktree. |
| 25 | +- Current `Selectable.Awake()` finds a `SpriteRenderer`, creates or reuses a `BoxCollider` on the renderer GameObject, sizes it from `sprite.bounds`, and adds a `PhysicsRaycaster` to the camera. |
| 26 | +- Attaching the collider to the renderer GameObject should share rotation, so different tilt is a hypothesis, not a confirmed root cause. Actual ray hits, hierarchy, layer masks, animation, scale, and existing collider ownership must be observed before choosing a fix. |
| 27 | +- Existing dirty changes in the primary `feature/345` worktree overlap `Selectable.cs` and `FieldSelector.cs`; they are user-owned and must remain untouched. |
| 28 | +- Procedural indicators are rendered through `SkillIndicatorShapeRenderer`; reuse its material/sorting behavior where practical rather than restoring deleted indicator prefabs. |
| 29 | +- Main, tutorial, and debug/secondary selection flows must be checked. Only change secondary flows that reproduce or consume the same contract. |
| 30 | +- Unity versions documented in repo files disagree (`2022.3.34f1` in `AGENTS.md`, `2022.3.22f1` in `CLAUDE.md`); validate against `ProjectSettings/ProjectVersion.txt` before Editor testing. |
| 31 | + |
| 32 | +## Approach (Checklist) |
| 33 | + |
| 34 | +- [x] **Step 0: Recon** (Inspect existing code, locate files) |
| 35 | + - [ ] Reproduce in `GameScene` with at least one ground object and one y != 0 object; record which sprite regions miss hover/click. (Play Mode pending) |
| 36 | + - [x] Inspect `PhysicsRaycaster` setup, prefab object layers, renderer hierarchy, and current input boundary. |
| 37 | + - [x] Compare `Selectable`, `SpriteRenderer`, existing `BoxCollider2D`, and motion transforms in representative prefab YAML. |
| 38 | + - [x] Identify the unstable contract: a separate runtime `BoxCollider` used `sprite.bounds` once and did not track renderer-local bounds changes. |
| 39 | + - [x] Inspect `FieldSelector`, `TutorialFieldSelector`, `SkillIndicatorShapeRenderer`, and `LineSkillIndicator` to preserve existing procedural visual conventions. |
| 40 | + - [x] Confirm field clicks already produce ground coordinates while object clicks must preserve the selected object's world y. |
| 41 | +- [x] **Step 1: Implementation** (Code changes, file paths) |
| 42 | + - [x] Refactor `Assets/Scripts/GameScene/ServedObjectComponent/Selectable.cs` to own a dedicated `SelectableHitbox` child under the selected renderer. |
| 43 | + - [x] Synchronize collider center/size from `SpriteRenderer.localBounds` only when bounds change. |
| 44 | + - [x] Keep `PhysicsRaycaster` setup deterministic and warn when no renderer exists. |
| 45 | + - [x] Add `Assets/Scripts/GameScene/SelectionGroundIndicator.cs` to draw the selected position to `(x, 0, z)`. |
| 46 | + - [x] Connect hover lifecycle and hide the line on exit, click, disable, destroy, or owner replacement. |
| 47 | + - [ ] If the field aim cursor can represent y != 0, update `Assets/Scripts/GameScene/FieldSelector.cs` to keep world cursor and ground projection as separate values. Mirror only contract-equivalent behavior in `Assets/Scripts/TutorialScene/Battle/TutorialFieldSelector.cs`. |
| 48 | + - [x] Preserve actual object `(x, y, z)` input and keep ground projection local to the indicator. |
| 49 | + - [x] Restrict UI blocking to `GraphicRaycaster` hits so the `PhysicsRaycaster` ground hit does not disable field selection. |
| 50 | + - [x] Give `Selectable.OnPointerClick` priority over `FieldSelector` ground input when the pointer raycast hits a selectable object. |
| 51 | +- [ ] **Step 2: Tests** (Unit tests, manual verification steps) |
| 52 | + - [ ] Add Edit Mode tests under `Assets/Tests/` for collider target resolution and renderer-local bounds mapping if those rules can be isolated without scene dependencies. |
| 53 | + - [ ] Add tests for projection endpoints: `(x, y, z)` maps to `(x, 0, z)`, y == 0 hides or collapses the vertical line, and cleanup is idempotent. |
| 54 | + - [ ] In Unity, verify top/center/bottom clicks on rotated ground and airborne sprites, including during idle animation and after sprite replacement. |
| 55 | + - [ ] Verify overlapping sprite/collider cases select the expected object and UI pointer blocking remains unchanged. |
| 56 | + - [ ] Verify indicator endpoints retain identical x/z, correct world-space y, visible sorting, and cleanup across hover/click/cancel/disable/destroy. |
| 57 | + - [ ] Smoke-test tutorial/debug selection paths if recon finds shared behavior. |
| 58 | +- [ ] **Step 3: Rollout / Rollback** (Feature flags, migration steps) |
| 59 | + - [ ] Ship as client-only behavior with no migration or server release ordering. |
| 60 | + - [ ] Capture before/after Scene/Game view evidence for the PR and document tested prefab types. |
| 61 | + - [ ] Keep collider fix and indicator addition as separable commits when implementation boundaries remain clean. |
| 62 | + - [ ] Roll back through commit revert; no persistent data or protocol rollback required. |
| 63 | + |
| 64 | +## Validation |
| 65 | + |
| 66 | +- **Commands to run:** |
| 67 | + - `cat ProjectSettings/ProjectVersion.txt` |
| 68 | + - Unity Editor compile using the version declared by `ProjectSettings/ProjectVersion.txt` |
| 69 | + - Unity Test Runner Edit Mode/Play Mode tests added for issue #350 |
| 70 | + - `git diff --check` |
| 71 | + - `git status --short` |
| 72 | +- **Expected output:** |
| 73 | + - Unity scripts compile without errors. |
| 74 | + - Added tests pass; no existing test regression. |
| 75 | + - Visible sprite regions reliably produce hover/click events in all listed manual cases. |
| 76 | + - y != 0 selection shows one line with endpoints `(x, y, z)` and `(x, 0, z)`; line disappears on every exit path. |
| 77 | + - Input payload remains compatible with existing `CardInputSender`/server expectations. |
| 78 | + - `git diff --check` prints no errors and diff contains only issue #350 files. |
| 79 | + |
| 80 | +## Risks & Rollback |
| 81 | + |
| 82 | +- **Risks:** |
| 83 | + - Enlarging or thickening colliders can make transparent sprite regions or overlapping objects capture unintended clicks. |
| 84 | + - Reusing gameplay colliders may change physics behavior; selection collider must remain trigger-only and ownership-specific. |
| 85 | + - Rebuilding bounds too often can add avoidable per-frame allocations/CPU cost across many objects. |
| 86 | + - LineRenderer/procedural indicator sorting may place the vertical line behind sprites or range meshes in the 2.5D camera. |
| 87 | + - Flattening `y` at the wrong boundary can change server targeting semantics; recon must confirm current contract. |
| 88 | + - Tutorial/debug flows may have different cursor lifecycles and should not be mechanically coupled without evidence. |
| 89 | +- **Rollback steps:** |
| 90 | + - Revert indicator commit independently if visual lifecycle/sorting regresses while keeping validated hit-testing fix. |
| 91 | + - Revert collider commit if click ordering or prefab physics regress. |
| 92 | + - No database, server, asset migration, or feature-flag cleanup is required. |
| 93 | + |
| 94 | +## Open Questions |
| 95 | + |
| 96 | +- Resolved: show the vertical line while hovering the active target, before input is committed. |
| 97 | +- Resolved: project to world `y = 0`, matching the requested acceptance baseline. |
| 98 | +- Resolved: object selection sends actual world y; only field selection and the visual ground endpoint use ground coordinates. |
| 99 | +- Which prefabs reproduce the miss most consistently? Record them during Editor recon and use them as the PR manual test matrix. |
0 commit comments