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

feat(simulation): knowledge graph snapshot integration (#366) #12

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

Summary

  • VisibleEntity now carries relationship (D-033 entity color) and observation (Visible vs Remembered) fields
  • compute_observer_snapshot queries the player's KnowledgeGraph to overlay relationship state on visible entities and include remembered (not-in-LOS) entities as fog ghosts at their last known position
  • New fields use #[serde(default)] for backward-compatible deserialization of old fixtures
  • Regenerated all msgpack fixtures with new wire format

Tickets

  • Closes #366 — Observer snapshot knowledge integration

Test plan

  • visible_npc_has_relationship_from_knowledge — visible NPC carries Hostile relationship from knowledge graph
  • remembered_entity_appears_as_ghost — not-in-LOS NPC appears as Remembered entity at last known position with correct confidence/age
  • direct_confidence_not_shown_as_remembered — Direct-confidence entities excluded from ghost list (transient inconsistency guard)
  • All 115 existing tests pass (fixtures deserialize with new fields via serde default)
  • Regenerated msgpack fixtures

🤖 Generated with Claude Code

## Summary - **VisibleEntity** now carries `relationship` (D-033 entity color) and `observation` (Visible vs Remembered) fields - **compute_observer_snapshot** queries the player's KnowledgeGraph to overlay relationship state on visible entities and include remembered (not-in-LOS) entities as fog ghosts at their last known position - New fields use `#[serde(default)]` for backward-compatible deserialization of old fixtures - Regenerated all msgpack fixtures with new wire format ## Tickets - Closes #366 — Observer snapshot knowledge integration ## Test plan - [x] `visible_npc_has_relationship_from_knowledge` — visible NPC carries Hostile relationship from knowledge graph - [x] `remembered_entity_appears_as_ghost` — not-in-LOS NPC appears as Remembered entity at last known position with correct confidence/age - [x] `direct_confidence_not_shown_as_remembered` — Direct-confidence entities excluded from ghost list (transient inconsistency guard) - [x] All 115 existing tests pass (fixtures deserialize with new fields via serde default) - [x] Regenerated msgpack fixtures 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 2 commits 2026-02-12 01:33:58 +01:00
feat(simulation): integrate knowledge graph into observer snapshot (#366) 9c0190c7c2 
VisibleEntity now carries relationship state (D-033 entity color) and
observation type (Visible vs Remembered). compute_observer_snapshot
queries the player's KnowledgeGraph to overlay relationship data on
visible entities and include remembered (not-in-LOS) entities as fog
ghosts at their last known position. Regenerated msgpack fixtures.

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

Dual-Agent Review: server -> main (PR #12)

Branch: server — feat(simulation): knowledge graph snapshot integration (#366)

Hoshe (Code Quality): REQUEST_CHANGES

Remembered entities from the knowledge graph are now overlaid onto the observer snapshot. Good use of #[serde(default)] for backward compat.

# File Severity Issue
1 server/src/perception/observer.rs ~Step 6 warning Remembered entity position could collide with a visible entity's tile. No dedup check on (x, y, z) — client may render two sprites on the same tile
2 server/src/perception/observer.rs ~Step 6 warning No z-level filter for remembered entities — visible entities are filtered to player_z but remembered entities use last_known_position which could be on a different z-level
3 server/src/perception/observer.rs suggestion Missing test coverage for remembered entity integration, edge cases (entity in both visible + remembered, stale knowledge, no position)

Tyre (Architecture): REQUEST_CHANGES

Overall the integration is clean and follows D-041 patterns.

# File Severity Issue
1 server/src/perception/observer.rs ~Step 6 warning Same z-level concern as Hoshe — remembered entities should respect z-level filtering
2 server/src/bridge/types.rs suggestion ObserverSnapshot version is still 2 — consider bumping to 3 since wire format changed (though #[serde(default)] provides compat)

Note: Tyre also flagged EntityVisibility missing Default and a "phase ordering bug" for Direct confidence skip — both are false positives. The code already has impl Default for EntityVisibility and the Direct confidence skip is intentional with explanatory comments.

Verdict: CHANGES REQUESTED

The z-level filter for remembered entities is the most substantive issue — it could cause entities from other floors to appear in the snapshot. Position collision is a real edge case worth addressing too.

## Dual-Agent Review: server -> main (PR #12) **Branch:** `server` — `feat(simulation): knowledge graph snapshot integration (#366)` ### Hoshe (Code Quality): REQUEST_CHANGES Remembered entities from the knowledge graph are now overlaid onto the observer snapshot. Good use of `#[serde(default)]` for backward compat. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `server/src/perception/observer.rs` ~Step 6 | warning | Remembered entity position could collide with a visible entity's tile. No dedup check on `(x, y, z)` — client may render two sprites on the same tile | | 2 | `server/src/perception/observer.rs` ~Step 6 | warning | No z-level filter for remembered entities — visible entities are filtered to `player_z` but remembered entities use `last_known_position` which could be on a different z-level | | 3 | `server/src/perception/observer.rs` | suggestion | Missing test coverage for remembered entity integration, edge cases (entity in both visible + remembered, stale knowledge, no position) | ### Tyre (Architecture): REQUEST_CHANGES Overall the integration is clean and follows D-041 patterns. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `server/src/perception/observer.rs` ~Step 6 | warning | Same z-level concern as Hoshe — remembered entities should respect z-level filtering | | 2 | `server/src/bridge/types.rs` | suggestion | ObserverSnapshot `version` is still 2 — consider bumping to 3 since wire format changed (though `#[serde(default)]` provides compat) | Note: Tyre also flagged `EntityVisibility` missing `Default` and a "phase ordering bug" for Direct confidence skip — both are **false positives**. The code already has `impl Default for EntityVisibility` and the Direct confidence skip is intentional with explanatory comments. ### Verdict: CHANGES REQUESTED The z-level filter for remembered entities is the most substantive issue — it could cause entities from other floors to appear in the snapshot. Position collision is a real edge case worth addressing too.
jpmschweitzer added 1 commit 2026-02-12 01:47:40 +01:00
fix(simulation): address PR #12 review — z-level filter, visible tile dedup, version bump 22de6c714f 
- Filter remembered entities by z-level (Hoshe + Tyre warning)
- Skip remembered ghosts on currently visible tiles (Hoshe warning)
- Bump ObserverSnapshot version to 3 (Tyre suggestion)
- Add edge case tests: visible tile collision, different z-level,
  knowledge without position (Hoshe suggestion)
- Regenerate msgpack fixtures for v3

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-12 01:47:44 +01:00
Author
Owner
Copy Link
Copy Source

All review items addressed in 22de6c7. Z-level filter added, ghosts on visible tiles suppressed, version bumped to v3, 3 new edge case tests. 118/118 pass.

All review items addressed in 22de6c7. Z-level filter added, ghosts on visible tiles suppressed, version bumped to v3, 3 new edge case tests. 118/118 pass.
jpmschweitzer added 1 commit 2026-02-12 01:50:21 +01:00
Merge remote-tracking branch 'origin/main' into server bf8e8f1baa
jpmschweitzer commented 2026-02-12 01:56:02 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: server -> main (PR #12, re-review)

Branch: server — feat(simulation): knowledge graph snapshot integration (#366)

Hoshe (Code Quality): APPROVE

All 4 previously flagged issues fixed. 13 observer tests cover all edge cases including z-level filtering, visible tile dedup, Direct confidence skip, and no-position entities. 116 tests pass. Implementation is correct with defensive programming throughout.

Tyre (Architecture): APPROVE

D-020/D-033/D-041 compliant. Knowledge graph integration respects client-server boundary — ObserverSnapshot remains the sole wire-crossing structure. #[serde(default)] on new fields provides backward compat. Version bumped to 3. Performance within D-041 budget. Architecture extensible for Sprint 3 (gossip, contradiction detection, Stale state).

Verdict: APPROVED

## Dual-Agent Review: server -> main (PR #12, re-review) **Branch:** `server` — `feat(simulation): knowledge graph snapshot integration (#366)` ### Hoshe (Code Quality): APPROVE All 4 previously flagged issues fixed. 13 observer tests cover all edge cases including z-level filtering, visible tile dedup, Direct confidence skip, and no-position entities. 116 tests pass. Implementation is correct with defensive programming throughout. ### Tyre (Architecture): APPROVE D-020/D-033/D-041 compliant. Knowledge graph integration respects client-server boundary — `ObserverSnapshot` remains the sole wire-crossing structure. `#[serde(default)]` on new fields provides backward compat. Version bumped to 3. Performance within D-041 budget. Architecture extensible for Sprint 3 (gossip, contradiction detection, Stale state). ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-12 01:56:28 +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#12