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

feat(client): protocol v2 decoder + D-033 entity visuals (#130) #11

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-12 00:56:02 +01:00
Owner
Copy Link
Copy Source

Summary

  • Protocol v2 decoder — extracts game_time, player_facing, visible_tiles, and per-entity visibility sectors from ObserverSnapshot v2 (backward compatible with v1)
  • GameState v2 storage — stores game time, player facing, visibility sectors; derives visible positions from visible_tiles when present
  • D-033 entity color palette — relationship-based colors (teal/green/amber/red), Phase 1 defaults by entity kind
  • Peripheral vision dimming (D-015) — entities in peripheral vision rendered at 50% alpha
  • Player facing direction indicator — Polygon2D triangle on player entity showing 8-directional facing
  • Tick-based world renderer invalidation — replaces count-based dirty tracking (Round 2 review fix)
  • Test snapshot updated to v2 format; 14 new tests (82 total passing)

Closes #130.

Test plan

  • All 82 gdUnit4 tests pass (make ci-client)
  • Protocol v2 fixture decoding verified (version, game_time, player_facing, visible_tiles, entity visibility)
  • Existing v1-style test paths still work (backward compatibility)
  • D-033 colors: player white-blue, NPC teal, objects grey
  • Peripheral entity dimmed to 50% alpha, forward entity at full alpha
  • Facing indicator present on player, absent on NPCs
  • Tick-based invalidation re-renders on tick change, skips duplicate ticks

🤖 Generated with Claude Code

## Summary - **Protocol v2 decoder** — extracts `game_time`, `player_facing`, `visible_tiles`, and per-entity `visibility` sectors from ObserverSnapshot v2 (backward compatible with v1) - **GameState v2 storage** — stores game time, player facing, visibility sectors; derives visible positions from visible_tiles when present - **D-033 entity color palette** — relationship-based colors (teal/green/amber/red), Phase 1 defaults by entity kind - **Peripheral vision dimming** (D-015) — entities in peripheral vision rendered at 50% alpha - **Player facing direction indicator** — Polygon2D triangle on player entity showing 8-directional facing - **Tick-based world renderer invalidation** — replaces count-based dirty tracking (Round 2 review fix) - Test snapshot updated to v2 format; 14 new tests (82 total passing) Closes #130. ## Test plan - [x] All 82 gdUnit4 tests pass (`make ci-client`) - [x] Protocol v2 fixture decoding verified (version, game_time, player_facing, visible_tiles, entity visibility) - [x] Existing v1-style test paths still work (backward compatibility) - [x] D-033 colors: player white-blue, NPC teal, objects grey - [x] Peripheral entity dimmed to 50% alpha, forward entity at full alpha - [x] Facing indicator present on player, absent on NPCs - [x] Tick-based invalidation re-renders on tick change, skips duplicate ticks 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 3 commits 2026-02-12 00:56:02 +01:00
feat(client): upgrade protocol decoder and GameState for v2 snapshot 57da75c509 
Server shipped ObserverSnapshot v2 with game_time, player_facing,
visible_tiles (with visibility sectors), and per-entity visibility.
Protocol decoder was silently ignoring these fields. Now extracts
all v2 data with null defaults for backward compatibility.

GameState gains game_time, player_facing, visibility_sectors vars.
Derives visible_positions from visible_tiles when present (for real
server mode). Test snapshot updated with v2 fields.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): D-033 entity colors, peripheral dimming, facing indicator (#130) f3793bf16e 
Replace hardcoded entity colors with D-033 relationship palette:
Player=#e0e8ff, Npc=unknown teal #4a9ebb, Object=grey #8b8ba0.
Phase 1 defaults by entity kind; Phase 2 (#361) will derive color
from RelationshipState via knowledge graph.

Entities in peripheral vision dimmed to 50% alpha (D-015).
Player entity gets a Polygon2D triangle indicator showing facing
direction, rotated from GameState.player_facing.

82 tests total, 0 failures.

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

Dual-Agent Review: client -> main (PR #11)

Hoshe (Code Quality): REQUEST_CHANGES

Clean implementation with strong test coverage (149 new test lines). One critical type safety bug in edge case.

# File Severity Issue
1 game_state.gd:66-73 critical visible_tiles loop accesses vtile.x/vtile.y without validating vtile is a Dictionary — will crash on malformed data (e.g. null element)
2 entity_renderer.gd:96 warning _facing_to_rotation(GameState.player_facing) — if player_facing is somehow null, the _: fallback handles it silently. Low risk since GameState defaults to "North"
3 test_rendering.gd warning No test for malformed visible_tiles entries (negative path for issue #1)
4 entity_renderer.gd:87 suggestion modulate.a written every frame even if unchanged — minor perf optimization to check before write
5 test_rendering.gd suggestion Add test for facing indicator rotation accuracy, not just presence

Tyre (Architecture): APPROVE

Architecturally correct — follows thin client principle perfectly. No coupling or boundary violations.

# File Severity Issue
1 entity_renderer.gd:48-51 suggestion Add clarifying comment that null visibility (v1 backward compat) defaults to full alpha
2 test_protocol.gd suggestion Add negative test for v1 backward compatibility (no v2 fields -> graceful null defaults)
3 constants.gd:7-12 suggestion Clarify comment that Phase 1 maps by entity kind, Phase 2 (#361) by RelationshipState

Architectural positives: D-010/D-020 thin client principle respected (pure renderer, no game logic). D-033 color palette matches spec exactly. D-015 peripheral dimming correct. D-031 game_time fields stored for future HUD. No inappropriate server knowledge in client.

Verdict: CHANGES REQUESTED

Blocking issue:

  1. Type safety in game_state.gd visible_tiles loop — add Dictionary validation before accessing .x/.y fields

The rest are suggestions. Solid PR overall.

## Dual-Agent Review: client -> main (PR #11) ### Hoshe (Code Quality): REQUEST_CHANGES Clean implementation with strong test coverage (149 new test lines). One critical type safety bug in edge case. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `game_state.gd:66-73` | critical | `visible_tiles` loop accesses `vtile.x`/`vtile.y` without validating vtile is a Dictionary — will crash on malformed data (e.g. `null` element) | | 2 | `entity_renderer.gd:96` | warning | `_facing_to_rotation(GameState.player_facing)` — if `player_facing` is somehow null, the `_:` fallback handles it silently. Low risk since GameState defaults to `"North"` | | 3 | `test_rendering.gd` | warning | No test for malformed `visible_tiles` entries (negative path for issue #1) | | 4 | `entity_renderer.gd:87` | suggestion | `modulate.a` written every frame even if unchanged — minor perf optimization to check before write | | 5 | `test_rendering.gd` | suggestion | Add test for facing indicator rotation accuracy, not just presence | ### Tyre (Architecture): APPROVE Architecturally correct — follows thin client principle perfectly. No coupling or boundary violations. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `entity_renderer.gd:48-51` | suggestion | Add clarifying comment that null visibility (v1 backward compat) defaults to full alpha | | 2 | `test_protocol.gd` | suggestion | Add negative test for v1 backward compatibility (no v2 fields -> graceful null defaults) | | 3 | `constants.gd:7-12` | suggestion | Clarify comment that Phase 1 maps by entity kind, Phase 2 (#361) by RelationshipState | **Architectural positives:** D-010/D-020 thin client principle respected (pure renderer, no game logic). D-033 color palette matches spec exactly. D-015 peripheral dimming correct. D-031 game_time fields stored for future HUD. No inappropriate server knowledge in client. ### Verdict: CHANGES REQUESTED **Blocking issue:** 1. Type safety in `game_state.gd` visible_tiles loop — add Dictionary validation before accessing `.x`/`.y` fields The rest are suggestions. Solid PR overall.
jpmschweitzer added 2 commits 2026-02-12 01:07:31 +01:00
fix(client): address PR #11 review — type safety, perf, tests 64897a40b3 
Add Dictionary validation in game_state visible_tiles loop to prevent
crash on malformed data (Hoshe critical). Skip redundant modulate.a
writes (Hoshe suggestion). Add 3 tests: malformed visible_tiles,
facing rotation accuracy, v1 backward compatibility. Clarify D-033
Phase 1/Phase 2 comments.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 995baf393d 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-12 01:10:13 +01:00
Author
Owner
Copy Link
Copy Source

TYRE Re-Review: APPROVE

All three suggestions from my previous review have been addressed in fix commit 64897a4.

Issue Resolution Status

SUGGESTION 1: Add clarifying comment that null visibility defaults to full alpha

  • Status: RESOLVED
  • Location: client/scripts/rendering/entity_renderer.gd:82
  • Evidence: Comment added: # null visibility (v1 backward compat) defaults to full alpha
  • Assessment: Clear and accurate. Explains both the technical behavior (null → full alpha) and the reason (v1 backward compatibility).

SUGGESTION 2: Add v1 backward compatibility test

  • Status: RESOLVED
  • Location: client/tests/test_protocol.gd:327-345
  • Evidence: New test test_decode_v1_snapshot_graceful_defaults() validates:
    • v1 snapshot (tick + entities only) decodes without error
    • v2 fields (version, game_time, player_facing, visible_tiles) gracefully default to null/empty
    • Entity visibility field is null (not missing, not crashing)
  • Assessment: Comprehensive negative-path test. Exactly what was needed.

SUGGESTION 3: Clarify D-033 Phase 1/Phase 2 comments

  • Status: RESOLVED
  • Location: client/scripts/constants.gd:10-11
  • Evidence: Updated comment:
    • Phase 1: "default colors mapped by entity kind (Player/Npc/Object/Terrain)"
    • Phase 2: "colors derived from RelationshipState via the knowledge graph"
  • Assessment: Crystal clear. Phase 1 = static mapping, Phase 2 = dynamic knowledge-driven. Links to ticket #361.

Bonus Fixes (from Hoshe's review)

Fix commit also addressed Hoshe's critical type safety issue and two suggestions:

  1. Type safety in visible_tiles loop (CRITICAL): Added Dictionary validation (if not vtile is Dictionary or not vtile.has("x") or not vtile.has("y"): continue) — prevents crash on malformed data. ✅
  2. Skip redundant modulate.a writes (SUGGESTION): Changed to if not is_equal_approx(entity_node.modulate.a, target_alpha): entity_node.modulate.a = target_alpha — minor perf improvement. ✅
  3. Test for malformed visible_tiles (SUGGESTION): New test test_game_state_skips_malformed_visible_tiles() validates null, string, and missing-field cases. ✅
  4. Test for facing rotation accuracy (SUGGESTION): New test test_entity_renderer_facing_indicator_rotation_accuracy() validates all 8 cardinal/diagonal rotation values. ✅

Architectural Compliance (D-010 / D-020)

The implementation continues to perfectly respect the thin client principle:

  • Pure renderer: Client receives ObserverSnapshot, draws what it's told. Zero game logic in GDScript.
  • Server authority: All visibility, relationship state, and entity properties flow from Rust simulation.
  • Backward compatibility: v1 snapshots (pre-visibility) gracefully degrade to full alpha. No client-side assumptions about missing data.
  • Future-ready: Phase 2 color derivation (#361) will be server-side (knowledge graph → RelationshipState → color in snapshot). Client just renders the color it receives.

No boundary violations. No inappropriate server knowledge.

New Issues Introduced by Fix Commit

NONE.

The fix commit is clean, focused, and addresses all feedback without introducing regressions or new concerns.

Verdict: APPROVE

This PR is ready to merge. All architectural, code quality, and test coverage issues resolved.

## TYRE Re-Review: APPROVE All three suggestions from my previous review have been addressed in fix commit 64897a4. ### Issue Resolution Status **SUGGESTION 1: Add clarifying comment that null visibility defaults to full alpha** - **Status:** RESOLVED - **Location:** `client/scripts/rendering/entity_renderer.gd:82` - **Evidence:** Comment added: `# null visibility (v1 backward compat) defaults to full alpha` - **Assessment:** Clear and accurate. Explains both the technical behavior (null → full alpha) and the reason (v1 backward compatibility). **SUGGESTION 2: Add v1 backward compatibility test** - **Status:** RESOLVED - **Location:** `client/tests/test_protocol.gd:327-345` - **Evidence:** New test `test_decode_v1_snapshot_graceful_defaults()` validates: - v1 snapshot (tick + entities only) decodes without error - v2 fields (version, game_time, player_facing, visible_tiles) gracefully default to null/empty - Entity visibility field is null (not missing, not crashing) - **Assessment:** Comprehensive negative-path test. Exactly what was needed. **SUGGESTION 3: Clarify D-033 Phase 1/Phase 2 comments** - **Status:** RESOLVED - **Location:** `client/scripts/constants.gd:10-11` - **Evidence:** Updated comment: - Phase 1: "default colors mapped by entity kind (Player/Npc/Object/Terrain)" - Phase 2: "colors derived from RelationshipState via the knowledge graph" - **Assessment:** Crystal clear. Phase 1 = static mapping, Phase 2 = dynamic knowledge-driven. Links to ticket #361. ### Bonus Fixes (from Hoshe's review) Fix commit also addressed Hoshe's critical type safety issue and two suggestions: 1. **Type safety in visible_tiles loop** (CRITICAL): Added Dictionary validation (`if not vtile is Dictionary or not vtile.has("x") or not vtile.has("y"): continue`) — prevents crash on malformed data. ✅ 2. **Skip redundant modulate.a writes** (SUGGESTION): Changed to `if not is_equal_approx(entity_node.modulate.a, target_alpha): entity_node.modulate.a = target_alpha` — minor perf improvement. ✅ 3. **Test for malformed visible_tiles** (SUGGESTION): New test `test_game_state_skips_malformed_visible_tiles()` validates null, string, and missing-field cases. ✅ 4. **Test for facing rotation accuracy** (SUGGESTION): New test `test_entity_renderer_facing_indicator_rotation_accuracy()` validates all 8 cardinal/diagonal rotation values. ✅ ### Architectural Compliance (D-010 / D-020) The implementation continues to perfectly respect the **thin client principle**: - **Pure renderer:** Client receives `ObserverSnapshot`, draws what it's told. Zero game logic in GDScript. - **Server authority:** All visibility, relationship state, and entity properties flow from Rust simulation. - **Backward compatibility:** v1 snapshots (pre-visibility) gracefully degrade to full alpha. No client-side assumptions about missing data. - **Future-ready:** Phase 2 color derivation (#361) will be server-side (knowledge graph → RelationshipState → color in snapshot). Client just renders the color it receives. **No boundary violations. No inappropriate server knowledge.** ### New Issues Introduced by Fix Commit **NONE.** The fix commit is clean, focused, and addresses all feedback without introducing regressions or new concerns. --- **Verdict: APPROVE** This PR is ready to merge. All architectural, code quality, and test coverage issues resolved.
jpmschweitzer commented 2026-02-12 01:15:26 +01:00
Author
Owner
Copy Link
Copy Source

Re-Review: client -> main (PR #11) — APPROVED

Fix commit 64897a4 addresses all 6 issues from the initial review.

Hoshe (Code Quality): APPROVE

  • Dictionary validation in visible_tiles loop (critical fix)
  • Skip redundant modulate.a writes (perf)
  • Malformed visible_tiles test added
  • Facing rotation accuracy test (all 8 directions)
  • v1 backward compatibility test
  • D-033 Phase 1/Phase 2 comments clarified

85/85 tests passing. No new issues.

Tyre (Architecture): APPROVE

All suggestions addressed. Thin client principle (D-010/D-020) fully respected. No boundary violations.

Verdict: APPROVED — ready to merge.

## Re-Review: client -> main (PR #11) — APPROVED Fix commit 64897a4 addresses all 6 issues from the initial review. ### Hoshe (Code Quality): APPROVE - Dictionary validation in visible_tiles loop (critical fix) - Skip redundant modulate.a writes (perf) - Malformed visible_tiles test added - Facing rotation accuracy test (all 8 directions) - v1 backward compatibility test - D-033 Phase 1/Phase 2 comments clarified 85/85 tests passing. No new issues. ### Tyre (Architecture): APPROVE All suggestions addressed. Thin client principle (D-010/D-020) fully respected. No boundary violations. ### Verdict: APPROVED — ready to merge.
jpmschweitzer closed this pull request 2026-02-12 01:16:09 +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#11