jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(client): sprint 16 — zone_id O(1) + sprite integration #58

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 2 Commits - Files Changed -
+638 -74
jpmschweitzer commented 2026-02-23 21:02:30 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 16 client work: two tickets.

  • #543 — Optimized zone_id extraction in game_state.gd from O(N) tile scan to O(1) dictionary lookup. Builds _tile_by_coord from member visible_tiles (covers both test-mode and live-server paths). 9 tests.
  • #540 — Migrated entity renderer from ColorRect placeholders to Sprite2D with D-019 angle sprites (-72.5deg from horizontal). self_modulate for D-033 relationship tinting, modulate.a reserved for D-015 peripheral dimming. 8-to-4 octant direction mapping, feet-anchored y-sort. 11 tests.

Key Decisions

  • D-019 (camera angle), D-033 (entity color = relationship), D-043 (visual style), D-044 (visual hierarchy), D-066 (dual-scale grid)

Test Plan

  • 9 tests in test_snapshot_zone_id.gd (S16-Z01–Z09) — zone_id lookup correctness including test-mode regression guard
  • 11 tests in test_sprite_integration.gd (S16-S01–S11) — footprint, z-sort, sprite assets, fog independence
  • test_color_shift.gd updated for Sprite2D/self_modulate migration
## Summary Sprint 16 client work: two tickets. - **#543** — Optimized zone_id extraction in `game_state.gd` from O(N) tile scan to O(1) dictionary lookup. Builds `_tile_by_coord` from member `visible_tiles` (covers both test-mode and live-server paths). 9 tests. - **#540** — Migrated entity renderer from ColorRect placeholders to Sprite2D with D-019 angle sprites (-72.5deg from horizontal). `self_modulate` for D-033 relationship tinting, `modulate.a` reserved for D-015 peripheral dimming. 8-to-4 octant direction mapping, feet-anchored y-sort. 11 tests. ## Key Decisions - D-019 (camera angle), D-033 (entity color = relationship), D-043 (visual style), D-044 (visual hierarchy), D-066 (dual-scale grid) ## Test Plan - [x] 9 tests in `test_snapshot_zone_id.gd` (S16-Z01–Z09) — zone_id lookup correctness including test-mode regression guard - [x] 11 tests in `test_sprite_integration.gd` (S16-S01–S11) — footprint, z-sort, sprite assets, fog independence - [x] `test_color_shift.gd` updated for Sprite2D/self_modulate migration
jpmschweitzer added 3 commits 2026-02-23 21:02:30 +01:00
refactor(client): optimize zone_id extraction from O(N) to O(1) lookup (#543) ebc973b558 
Build _tile_by_coord dictionary from member visible_tiles (covers both
test-mode "tiles" key and live-server "visible_tiles" key), then replace
the linear scan with a single dict lookup. Net-zero complexity: adds one
dict-set per tile in an existing iteration, removes the separate scan loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): integrate D-019 angle sprites into entity renderer (#540) a0260176b4 
Migrate entity rendering from ColorRect placeholders to Sprite2D with
rendered PNGs at -72.5° from horizontal. Key changes:
- Sprite2D.centered=false, scale=0.5 for 64px source → 32px runtime
- self_modulate for D-033 relationship tinting (modulate.a reserved
  for D-015 peripheral dimming)
- 8-octant to 4-cardinal direction mapping for sprite selection
- Feet-anchored ENTITY_OFFSET_Y for correct y-sort with tilted sprites
- Facing indicator repositioned to sprite local center (32,32)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog f67f13ac95 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-23 21:05:03 +01:00
Author
Owner
Copy Link
Copy Source

PR #58 Review: client → main

Hoshe (Code Quality): REQUEST_CHANGES

# File Severity Issue
1 entity_renderer.gd:156 warning _load_sprite_texture can return null — entity becomes invisible with no crash signal. Both call sites (112, 156) should guard against null.
2 entity_renderer.gd:206-210 warning NPC _entity_direction + _octant_to_direction match table have zero test coverage.
3 entity_renderer.gd:215-221 suggestion _octant_to_direction fallback silently returns "south" — add push_warning on default branch.
4 game_state.gd:246-252 suggestion Stale visible_tiles scenario not tested (pre-existing quirk, higher exposure now).
5 test_color_shift.gd:89-94 suggestion Forward-compat guard silently passes when method absent.
6 test_snapshot_zone_id.gd:176-191 suggestion No test for negative tile coordinates.
7 test_sprite_integration.gd:8 suggestion Missing GDScript type annotation.

Tyre (Architecture): APPROVE

D-033/D-015 separation correct, D-066 footprint verified, z-sort correct, D-010/D-012 no violations.

# File Severity Issue
1 entity_renderer.gd:227 suggestion Cache Texture2D by direction string instead of calling load() on every direction change.
2 game_state.gd:250-252 suggestion Tag _tile_by_coord with # perf comment for future profiling.
3 entity_renderer.gd:206-210 suggestion Add TODO comment for future NPC facing ticket.
4 test_sprite_integration.gd:153-159 suggestion Asset existence tests hard-fail — consider conditional skip pattern.

Verdict: CHANGES REQUESTED

## PR #58 Review: `client` → `main` ### Hoshe (Code Quality): REQUEST_CHANGES | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `entity_renderer.gd:156` | **warning** | `_load_sprite_texture` can return `null` — entity becomes invisible with no crash signal. Both call sites (112, 156) should guard against null. | | 2 | `entity_renderer.gd:206-210` | **warning** | NPC `_entity_direction` + `_octant_to_direction` match table have zero test coverage. | | 3 | `entity_renderer.gd:215-221` | suggestion | `_octant_to_direction` fallback silently returns "south" — add `push_warning` on default branch. | | 4 | `game_state.gd:246-252` | suggestion | Stale `visible_tiles` scenario not tested (pre-existing quirk, higher exposure now). | | 5 | `test_color_shift.gd:89-94` | suggestion | Forward-compat guard silently passes when method absent. | | 6 | `test_snapshot_zone_id.gd:176-191` | suggestion | No test for negative tile coordinates. | | 7 | `test_sprite_integration.gd:8` | suggestion | Missing `GDScript` type annotation. | ### Tyre (Architecture): APPROVE D-033/D-015 separation correct, D-066 footprint verified, z-sort correct, D-010/D-012 no violations. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `entity_renderer.gd:227` | suggestion | Cache `Texture2D` by direction string instead of calling `load()` on every direction change. | | 2 | `game_state.gd:250-252` | suggestion | Tag `_tile_by_coord` with `# perf` comment for future profiling. | | 3 | `entity_renderer.gd:206-210` | suggestion | Add TODO comment for future NPC facing ticket. | | 4 | `test_sprite_integration.gd:153-159` | suggestion | Asset existence tests hard-fail — consider conditional skip pattern. | ### Verdict: CHANGES REQUESTED
jpmschweitzer added 2 commits 2026-02-23 21:06:40 +01:00
fix(client): guard null texture and warn on unknown octant (#540 review) 56b8381c40 
Add push_error on null texture at create time, keep previous texture
on null at update time (entity stays visible mid-game). Add push_warning
on unrecognised octant in _octant_to_direction fallback.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
test(client): add direction mapping tests for octant and entity facing (#540 review) 0ebd417a1f 
14 tests covering _octant_to_direction (all 8 octants + 2 fallbacks)
and _entity_direction (NPC default south, player facing 3 cases).
Closes review warning on zero test coverage for direction system.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-23 21:07:41 +01:00
Author
Owner
Copy Link
Copy Source

PR #58 Re-review: client → main

Hoshe (Code Quality): APPROVE

Both warnings resolved. Null-texture guard correctly split (push_error on create, keep previous on update). Direction mapping tests cover all 8 octants + fallbacks + NPC/player paths.

Tyre (Architecture): APPROVE

Fix commits clean. Null-guard asymmetry correct by design. No new architectural concerns.

Verdict: APPROVED

## PR #58 Re-review: `client` → `main` ### Hoshe (Code Quality): APPROVE Both warnings resolved. Null-texture guard correctly split (push_error on create, keep previous on update). Direction mapping tests cover all 8 octants + fallbacks + NPC/player paths. ### Tyre (Architecture): APPROVE Fix commits clean. Null-guard asymmetry correct by design. No new architectural concerns. ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-24 02:15:20 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#58