feat(all): add read model for the inventoryManager extended feed - #1524
feat(all): add read model for the inventoryManager extended feed#1524MahtraDR wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Problem: Lich has no way to answer whole-inventory questions ("find item X
anywhere", "audit everything I carry", per-container weight). GameObj only
holds containers the passive stream has already streamed (hands, worn, a pack
you just opened) and carries no weight, capacity, or closed/locked state.
Fix: add Lich::Common::Inventory, a standalone read model that parses a single
Saga "extended feed" <inventoryManager> response into an immutable, id-keyed
tree with per-item weight, container load, and flags. It is a passive,
read-only tap on the game parser thread and stays completely inert unless the
extended feed actually appears, so non-Saga sessions are unaffected.
Key safety properties (verified against the real 418-item DR capture):
- observe never mutates the stream, never sends upstream, and never raises
(the parser thread's only rescue closes the socket and would drop the
session); the new snapshot is built into a local and swapped in atomically
last under a mutex.
- Ox does not raise on truncated input, so damage is detected via a collecting
error() callback plus a structural-close check; any parse error, missing
close, continuation/paginated fragment, or zero-item passive response over a
non-empty snapshot fails closed and keeps the prior snapshot.
- Locked containers emit zero children by game design and are modeled as opaque
(contents unknown), never empty; used_lbs/space_left are nil for them.
- type/sellable derive from each item's own noun/name via a transient GameObj
(GameObj[id] is nil for delta items in unopened containers).
- refresh runs on the caller thread with a per-id completion latch, a bounded
timeout, and capped-exponential-backoff re-probing when the feed is absent.
Wiring: required after gameobj in the load list; one guarded observe call on
the parser thread beside process_downstream_hooks.
Tests: DAMP specs against the real capture fixture plus hand-built edge cases
(truncation, continuations, cyclic loc, empty envelopes, feed-absent backoff,
crash safety). 49 new examples; full suite 6330 examples, 0 failures; rubocop
clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8663942 to
0e45d54
Compare
| # @return [GameObj] | ||
| # @api private | ||
| def bridge_gameobj | ||
| @bridge_gameobj ||= GameObj.new(@id, @noun, @name) |
There was a problem hiding this comment.
This is going to populate a lot of new GameObjs the way it's written currently and not reuse existing GameObjs. Suggest swapping to:
@bridge_gameobj ||= GameObj.index_or_create(@id, @noun, @name, @before, @after)https://github.com/elanthia-online/lich-5/blob/main/lib/common/gameobj.rb#L475
Also suggest have a way to have the LONG parsed to have the before/after (dropping the a/an/some) so the above can get the full name support on the GameObj
I'd also suggest using smart logic to know if something is in a container/ground/worn/etc to use the various GameObj.new_loot, GameObj.new_room_desc, or GameObj.new_inv. So that it has some nested functionality.
Basically, GameObj.new by itself should be avoided at all costs.
There was a problem hiding this comment.
Done in be0abd7:
#1 / #4 (avoid bare GameObj.new): the classification bridge now builds via GameObj.index_or_create (memoized per item), so it reuses an existing pooled instance for the same id|noun|name and joins the shared index + TTL instead of allocating an unpooled object each time. It's still deliberately not pushed into a registry, so GameObj[] won't start resolving delta items in unopened containers.
#2 (parse the LONG for before/after): it now feeds the full descriptive name — the item's long when present — with the leading a/an/some/the peeled into before_name, so GameObj#full_name reconstructs the whole phrase and classification matches on name/noun/full_name the way it does for a live GameObj. Added a spec that classifies via a full_name-only pattern to lock it in (it failed with the old short-name feed).
#3 (location-aware new_inv / new_loot / new_room_desc): I'd like to grab a few minutes with you to make sure I build exactly what you have in mind here. This one has Inventory populate the GameObj registries, which is a bigger step than the read-model separation the current design landed on, and I want to get the details right — e.g. @@contents for unopened containers (and how it interacts with commit_container when one is later opened), worn items vs @@inv/commit_inv, and how to route ground items (loc='room') between new_loot and new_room_desc when the wire doesn't distinguish them. Will ping you to talk it through.
There was a problem hiding this comment.
#3 is implemented (68ceaea). Live DR probing first turned up that the feed carries more relations than the design had assumed — so the mapping is richer than we'd discussed:
righthand,player/lefthand,player— held items (confirmed both hands)atfeet,player— items at the player's own feetroom— the shared room floor: two characters observing the same room get the same ground item by the same global exist id
Location → GameObj mirror, on each committed snapshot (parser thread), all via index_or_create/new_*, never bare new:
loc |
GameObj home |
|---|---|
in,{id} / on,{id} |
new_inv(…, {id}) → GameObj.contents[{id}] — incl. unopened containers |
worn,player |
new_inv(…, nil) → GameObj.inv (deduped by exist id) |
righthand/lefthand,player |
new_right_hand / new_left_hand |
atfeet,player / room |
new_loot → GameObj.loot |
The hand/loot constructors take only a name, so I backfill before_name/after_name afterward (nil-only, never clobbering the classic values) to keep full_name classification working. Classic stream stays live-authoritative — commit_inv/commit_container replace our writes for opened/worn slots, so those are transient; the durable win is @@contents for unopened containers. GameObj[id] now resolves delta items via @@contents, so the earlier "nil for delta items" concern is gone.
Also fixed two things the hand data exposed: worn? now keys on relation == 'worn' (was parent == 'player', which mis-bucketed held items as worn), and total_weight counts worn and held, excluding ground.
Full suite 6345 examples / 0 failures, rubocop clean, CI green. If any of the mapping choices (worn→inv dedupe, room+atfeet→loot, hand-slot writes) aren't what you had in mind, happy to adjust.
There was a problem hiding this comment.
Correction to the mapping above (f56a123): atfeet,player is on-character (at the player's own feet), so it does not go to @@loot — that registry is off-character room-floor loot (the classic room objs stream, cleared on nav). Only room (the shared room floor) maps to new_loot now; atfeet is pooled for identity/classification and surfaced via Inventory.at_feet only.
Address review (mrhoribu): the type/sellable bridge built its GameObj with a bare GameObj.new, which bypasses the shared identity index -- allocating an unpooled object per item instead of reusing an existing one. - Build the bridge via GameObj.index_or_create so it reuses an existing instance for the same id|noun|name and participates in the shared index + TTL. It still is NOT pushed into any registry, so Inventory remains a non-writer of GameObj's query surfaces (GameObj[] still won't resolve a delta item). - Feed the full descriptive name (the item's `long` when present) with the leading article split into before_name, so GameObj#full_name reconstructs the whole phrase and classification matches via name/noun/full_name as it does for a live GameObj. Previously only the lossy short name was fed, missing every full_name-keyed classifier. Spec: add a full_name-classification case (passes only with the long-derived name); isolate GameObj @@index/@@type_cache between examples now that the bridge pools identity. Full suite 6331 examples, 0 failures; rubocop clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| before, name = bridge_name_parts | ||
| GameObj.index_or_create(@id, @noun, name, before) |
There was a problem hiding this comment.
this is still missing the after_name bit. Here's some sample data mappings. I think the a/an/some/the may very well go into the before_name but I'm also ok with that being nil instead for those. Since I know you also store some of the get cmd stuff in there on the DR side
<i id='165666521' loc='righthand,player' name="a gnarled,rowan,crook" long="a $_gnarled rowan crook$_ wound with knotted yarn" weight='2'/>
id: 165666521
noun: crook
name: gnarled rowan crook
before: nil
after: wound with knotted yarn
<i id='165666628' loc='worn,player' name="a tooled,leather coin,bag" long="a tooled leather drawstring coin bag" weight='1'/>
id: 165666628
noun: bag
name: tooled leather drawstring coin bag
before: nil
after: nil
<i id='145925572' loc='worn,player' name="a,linen,badge" long="a tin-bound linen badge" weight='1'/>
id: 145925572
noun: badge
name: tin-bound linen badge
before: nil
after: nil
<i id='165666522' loc='worn,player' name="a voluminous,mist tartan,cloak" long="a $_voluminous mist tartan cloak$_ lined in silver-tipped aquerne" weight='5' in_max='2500'/>
id: 165666522
noun: cloak
name: voluminous mist tartan cloak
before: nil
after: lined in silver-tipped aquerne
There was a problem hiding this comment.
Fixed in 4c277e1 — the $_ markers and after_name are now handled:
- Split the raw
longon the$_..._$markers: the text between them isname, the text after them becomesafter_name. - Dropped the leading article with
before_nameleftnil(went with nil per your note, since DR already usesbefore_namefor the get-command context). - Also strips the
$_markers fromItem#longso script authors get a clean description.
Verified against all four of your samples:
| id | noun | name | before | after |
|---|---|---|---|---|
| 165666521 | crook | gnarled rowan crook | nil | wound with knotted yarn |
| 165666628 | bag | tooled leather drawstring coin bag | nil | nil |
| 145925572 | badge | tin-bound linen badge | nil | nil |
| 165666522 | cloak | voluminous mist tartan cloak | nil | lined in silver-tipped aquerne |
Each is now a spec case. One thing your data surfaced that's worth a separate note: loc='righthand,player' — the feed does emit righthand/lefthand relations, which the earlier design notes had assumed it didn't. That's orthogonal to this thread (it's about top-level bucketing, not the bridge), so I'll fold it into the #3 conversation rather than change it here.
Address review (mrhoribu, with sample DR wire data): the bridge was dropping after_name and leaving Simu's $_ highlight markers in the name. - descriptive_parts now splits the raw `long` on the $_..._$ markers: the text between them is the name, the text after them becomes after_name. The leading article is dropped and before_name stays nil (per the author's guidance, so it does not collide with the DR get-command context stored in before_name). - Item#long now strips the $_ markers so script authors get a clean description. Verified against the author's four sample mappings (crook/coin bag/badge/cloak). Full suite 6335 examples, 0 failures; rubocop clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Live DR probes proved the inventoryManager feed emits more relations than the
earlier design assumed: righthand/lefthand (held), atfeet (your feet), and room
(shared room-ground loot, listing every observer's droppings by global exist id)
-- not just worn/in/on. This reworks the model accordingly and, per the GameObj
author's request, mirrors each snapshot into GameObj.
Read model:
- worn? now keys on relation == 'worn', not parent == 'player' (which also
covered hands/feet -- a held greatsword was wrongly bucketed as worn).
- New relation-based buckets/predicates: right_hand, left_hand, at_feet, plus
in_right_hand?/in_left_hand?/held?/at_feet?; in_room? is relation-based and
room is documented as shared (ids not necessarily the player's).
- total_weight now counts worn AND held items; ground (room/at_feet) excluded.
GameObj integration (mirror on each committed snapshot, on the parser thread):
in,{id}/on,{id} -> GameObj.contents[{id}] (the real gap: unopened trees)
worn,player -> GameObj.inv (deduped by exist id)
righthand/lefthand,player -> GameObj.right_hand/left_hand
atfeet,player / room -> GameObj.loot
before_name/after_name are backfilled on the hand/loot constructors so
full_name classification still works. Classic stream stays live-authoritative;
Inventory writes are transient for fast-changing slots. GameObj[id] now resolves
delta items (m4 premise resolved), which the specs assert.
Specs: real captured items (skate/rapier in hands, jade bowl at feet, lollipop
on shared ground) drive the bucket + mirror tests. Full suite 6345, 0 failures;
rubocop clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@@loot is off-character room-floor loot (populated by the classic `room objs` stream, cleared on nav). atfeet,player is on-character (at the player's own feet), so mirroring it into @@loot was wrong. Now only `room` (the shared room floor) maps to new_loot; `atfeet` is pooled for identity/classification via index_or_create and surfaced solely through Inventory#at_feet. Full suite green; rubocop clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@mrhoribu I've made some big changes to this for how it deals with pagination/continuation if you wouldn't mind looking again? |
The inventory read-model mirrored snapshots into GameObj's registries but only ever added, never retracted, so moved/removed items became permanent stale ghosts. Reconcile through the elanthia-online#1370/elanthia-online#1497 begin_*/commit_* staging system (atomic wholesale replace) instead of surgical removal: - Worn -> @@inv via begin_inv/commit_inv; container contents -> @@contents[id] via begin_container/commit_container (per non-opaque container). Anything not re-registered is gone after the swap. - Skip opaque (locked) containers so their last-known contents are preserved rather than wiped to []; delete_container a container that vanished from the tree entirely. - Add GameObj.inv_refresh_open? / container_refresh_open?(id) predicates so Inventory skips a target whose classic staged refresh is already open, never truncating an in-flight classic fill (self-heals next response). Stop mirroring hands and room loot: the classic <right>/<left> and room-objs streams already own those slots live and authoritatively, so the mirror only added redundant, occasionally-stale writes. Non-mirrored items are still pooled via index_or_create for type/sellable classification. Move continuation folding + GameObj mirroring back onto the parser thread (observe -> route_response -> fold_part -> finalize_assembly); refresh now only sends the continuation requests the parser thread asks for and waits. This keeps all registry writes single-threaded, which is what makes the staging swaps and the lock-free reads from elanthia-online#1370 sound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the PR elanthia-online#1524 review findings (rounds 2-3): - GameObj#type/#sellable now degrade to nil instead of raising when gameobj-data.xml is missing/corrupt (load_data nils the data hashes). Guards both the matching_data_keys(nil) crash and the nil.empty? reload trigger; fails loudly once then quietly, since these are public API on every Inventory::Item. Fixes the ~40-50%-of-seeds spec flakiness at its root rather than by test order, and adds a direct regression test. - inventory_spec now resets @@type_data/@@sellable_data between examples so the whole suite is order-independent (only @@type_cache was cleared). - Inventory.refresh now mints the request id under @Mutex, matching the drain_queue call site, so the two id call sites no longer race. - Inventory.reset! now mutates shared state under @Mutex like every other mutator, so an off-parser-thread reconnect call can't tear state out from under a draining route_response. - Document @refresh_mutex as a deliberate serialization of .refresh (the exchange is stateful on the wire); the id fix makes lifting it id-collision-safe should concurrent refreshes ever be needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
Lich can't answer whole-inventory questions — "find item X anywhere", "audit everything I carry", per-container weight budgeting.
GameObjonly holds containers the passive<container>/clearContainerstream has already streamed (hands, worn, a pack you just opened), and carries no weight, capacity, encumbrance, or closed/locked state. Unopened containers are simply absent fromGameObj.@@contents.The Saga "extended feed"
inventoryManagerstream (confirmed live on both GS and DR) returns the player's entire nested item tree in one response — every container's contents at once, with per-itemweight, container load (in_encum), andclosed/lockedflags. Nothing consumes it today.Fix
Add
Lich::Common::Inventory— a standalone, read-only snapshot model that parses one<inventoryManager>response into an immutable, id-keyed tree. It is a passive tap on the game parser thread and is completely inert unless the extended feed appears, so non-Saga sessions are unaffected. It bridges toGameObjby exist id (borrowingtype/sellable) rather than duplicating or rewriting it.Safety properties, verified against a real 418-item DR capture:
observenever mutates the stream, never sends upstream, and never raises — the parser thread's only rescue does@socket.closeand would drop the player's session. The new snapshot is built into a local and swapped in atomically last under a mutex.Ox.sax_parsedoesn't raise on truncated input (it auto-closes), so damage is detected via a collectingerror()callback and a structural-close check. Any parse error, missing close, continuation/paginated fragment, or a zero-item passive response over a non-empty snapshot is discarded and the prior snapshot kept.used_lbs/space_leftarenilfor it. A merely closed container still enumerates its contents.type/sellablederive from each item's own noun/name via a transientGameObj(GameObj[id]isnilfor exactly the delta items — those in unopened containers).refresh. Runs on the caller thread with a per-id completion latch, a bounded timeout, and capped-exponential-backoff re-probing when the feed is absent — no permanent latch, no repeated stalls. Returns an immutable snapshot object (ornil), never blocks unbounded.Wiring: required after
gameobjin the load list; one guardedInventory.observe(server_string)call on the parser thread besideprocess_downstream_hooks.Every public class/method carries YARD (per
docs/YARD-STYLE-GUIDE.md) with runnable@examples and documentednil/failure returns.Tests
DAMP specs in
spec/lib/common/inventory_spec.rbrun against the real capture fixture (spec/fixtures/inventory/dr_full_inventory.xml) plus hand-built edge cases: truncation, continuation fragments, paginated responses, cyclicloc, self-closing/empty envelopes, empty-passive guard, feed-absent backoff,nil-capacityspace_left, delta-item classification, and crash safety. Unshipped paths (continuation assembly, multi-line) are not faked.rubocopclean on all touched filesUser- and developer-facing wiki guides (
Inventory-Module-Guide,Inventory-Module-Developer-Guide) are drafted and will be published to thelich-5.wikiafter review.🤖 Generated with Claude Code