Add gravestone entity on player death with storable inventory#354
Conversation
Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/cbcee017-2333-496f-ad1b-cddf1334f80e Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new “gravestone” drop mechanic so player-death inventory recovery is done in one interaction instead of scattered pickups, with room persistence support.
Changes:
- Introduces
StorableInventorymixin +Gravestoneentity (with placeholder sprite). - Updates
WorldScreen.respawnPlayer()to drop a gravestone containing the player’s inventory and adds right-click interaction to retrieve items. - Extends
RoomJsonReaderWriter(and tests) to serialize/deserialize gravestones with their stored inventory.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/entity/storableEntity.py |
Adds a reusable stored-inventory mixin for entities. |
src/entity/gravestone.py |
Implements the Gravestone entity as a solid DrawableEntity with storage. |
src/screen/worldScreen.py |
Drops gravestone on death and adds gravestone retrieval via place-action interaction. |
src/world/roomJsonReaderWriter.py |
Persists gravestone stored inventory via new helper methods. |
tests/entity/test_gravestone.py |
Unit tests for gravestone initialization and storage behavior. |
tests/screen/test_worldScreen_gravestone.py |
Tests death-drop and interaction behavior in WorldScreen. |
tests/world/test_roomJsonReaderWriter.py |
Adds serialization/deserialization + round-trip tests for gravestones. |
assets/images/gravestone.png |
Placeholder sprite asset for the gravestone. |
CHANGELOG.md |
Documents the feature and adds learning-log notes. |
| for item in list(slot.getContents()): | ||
| if not self.player.getInventory().placeIntoFirstAvailableInventorySlot( | ||
| item | ||
| ): | ||
| self.status.set("Inventory full") |
There was a problem hiding this comment.
_interactWithGravestone() places item objects into the player's inventory without removing them from the gravestone slots, and if placement fails partway through it returns without rolling back already-moved items. This violates the "no partial transfer" behavior and can duplicate items (some end up in player inventory while still remaining in the gravestone). Consider making the transfer atomic: pre-check capacity for all items, or track moved items and rollback on failure, and only remove/clear the gravestone inventory after a fully successful transfer.
| constructor = self.entityConstructors.get(entityClass) | ||
| if constructor is None: | ||
| _logger.warning("unknown stored item class", entityClass=entityClass) | ||
| return None |
There was a problem hiding this comment.
_createStoredItem() logs and returns None for unknown stored item classes, which will silently drop items from a saved gravestone on load. For persistence, it’s safer to fail loudly (e.g., raise a ValueError like generateEntityFromJson does) or otherwise surface the corruption, rather than losing player items without notice.
| return None | |
| raise ValueError(f"unknown stored item class: {entityClass}") |
| from src.entity.gravestone import Gravestone | ||
| from src.entity.apple import Apple | ||
| from src.entity.oakWood import OakWood |
There was a problem hiding this comment.
This test imports entities via the src.entity... package path, while the game code imports via entity... (because src is on pytest's pythonpath). Mixing these can create two distinct class objects for the same entity and break isinstance checks. Prefer importing from entity.gravestone import Gravestone / from entity.apple import Apple / etc. consistently in tests.
| from src.entity.gravestone import Gravestone | |
| from src.entity.apple import Apple | |
| from src.entity.oakWood import OakWood | |
| from entity.gravestone import Gravestone | |
| from entity.apple import Apple | |
| from entity.oakWood import OakWood |
| os.environ["SDL_AUDIODRIVER"] = "dummy" | ||
| import pygame | ||
| import pytest | ||
| from unittest.mock import MagicMock, patch |
There was a problem hiding this comment.
Unused import: patch is imported but never used in this test module. Please remove it to keep the test code clean (and to avoid autoflake/linters flagging it).
| from unittest.mock import MagicMock, patch | |
| from unittest.mock import MagicMock |
| from entity.gravestone import Gravestone | ||
| from entity.oakWood import OakWood | ||
| from inventory.inventory import Inventory | ||
| from lib.pyenvlib.grid import Grid |
There was a problem hiding this comment.
Unused import: Grid is imported but never used in this test module. Please remove it to keep imports minimal.
| from lib.pyenvlib.grid import Grid |
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/bbdb32d6-b3ff-4d16-9c02-192d9355ab8f Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Addressed all four review threads in ff07cd6:
|
There was a problem hiding this comment.
Modify name of file to match class name 'StorableInventory' and update any references in code/docs
There was a problem hiding this comment.
Done in ae80ca9 — renamed storableEntity.py → storableInventory.py and updated the import in gravestone.py and the reference in CHANGELOG.md.
…lass name Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/b4c56a29-4b2c-432c-862e-80489f38a43c Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
| def _inventoryCanFitAll(self, inventory, items): | ||
| """Return True if all items can be placed into inventory without overflow.""" | ||
| maxStack = 20 | ||
| # Available space in existing non-empty slots keyed by item name | ||
| slotSpace = {} | ||
| freeSlots = 0 | ||
| for slot in inventory.getInventorySlots(): | ||
| if slot.isEmpty(): | ||
| freeSlots += 1 | ||
| else: | ||
| name = slot.getContents()[0].getName() | ||
| available = maxStack - slot.getNumItems() | ||
| if available > 0: | ||
| slotSpace[name] = slotSpace.get(name, 0) + available | ||
| for item in items: | ||
| name = item.getName() | ||
| if slotSpace.get(name, 0) > 0: | ||
| slotSpace[name] -= 1 | ||
| elif freeSlots > 0: | ||
| freeSlots -= 1 | ||
| # Opening a new slot gives (maxStack - 1) additional spaces | ||
| slotSpace[name] = slotSpace.get(name, 0) + (maxStack - 1) | ||
| else: | ||
| return False | ||
| return True |
There was a problem hiding this comment.
_inventoryCanFitAll() hardcodes maxStack = 20. Inventory stacking limits already live on InventorySlot.getMaxStackSize() (and are used elsewhere like InventoryScreen.craftRecipe). Using the slot’s getMaxStackSize() (or a shared helper) here avoids this logic silently diverging if stack size ever changes.
| for itemJson in slotJson.get("slotContents", []): | ||
| item = self._createStoredItem(itemJson) | ||
| if item is not None: | ||
| inventory.placeIntoFirstAvailableInventorySlot(item) |
There was a problem hiding this comment.
_restoreStoredInventory() ignores the boolean return from placeIntoFirstAvailableInventorySlot(). If the JSON ever contains more items than the stored inventory can hold (corrupt save, future inventory size changes, etc.), items will be silently dropped during load. Consider checking the return value and raising/logging an error so data loss is visible.
| inventory.placeIntoFirstAvailableInventorySlot(item) | |
| itemPlaced = inventory.placeIntoFirstAvailableInventorySlot(item) | |
| if not itemPlaced: | |
| _logger.error( | |
| "failed to restore stored inventory item %s (%s) from saved slot %s: " | |
| "no inventory space available", | |
| itemJson.get("entityClass"), | |
| itemJson.get("entityId"), | |
| slotJson.get("slotIndex"), | |
| ) |
| stone1 = Gravestone() | ||
| stone2 = Gravestone() | ||
|
|
||
| stone1.getStoredInventory().placeIntoFirstAvailableInventorySlot(Apple()) | ||
|
|
||
| assert stone1.getStoredInventory().getNumItems() == 1 | ||
| assert stone2.getStoredInventory().getNumItems() == 0 |
There was a problem hiding this comment.
Variable names stone1/stone2 in this gravestone test are misleading (they’re Gravestone instances). Renaming them to gravestone1/gravestone2 would make the test intent clearer and avoid confusion when reading failures.
| stone1 = Gravestone() | |
| stone2 = Gravestone() | |
| stone1.getStoredInventory().placeIntoFirstAvailableInventorySlot(Apple()) | |
| assert stone1.getStoredInventory().getNumItems() == 1 | |
| assert stone2.getStoredInventory().getNumItems() == 0 | |
| gravestone1 = Gravestone() | |
| gravestone2 = Gravestone() | |
| gravestone1.getStoredInventory().placeIntoFirstAvailableInventorySlot(Apple()) | |
| assert gravestone1.getStoredInventory().getNumItems() == 1 | |
| assert gravestone2.getStoredInventory().getNumItems() == 0 |
| from inventory.inventory import Inventory | ||
|
|
||
|
|
||
| # @author Copilot | ||
| # @since April 21st, 2026 | ||
| class StorableInventory: | ||
| """Mixin that provides an internal Inventory for entities that can store items. | ||
|
|
||
| Intended to be used by Gravestone and, in the future, Chest entities. | ||
| """ |
There was a problem hiding this comment.
PR description mentions a new StorableInventory living in src/entity/storableEntity.py, but the implementation added here is src/entity/storableInventory.py. Aligning the PR description (or the filename) will prevent confusion for reviewers and future contributors searching for the class.
|
@copilot You are a coding agent tasked with resolving all open comments and review threads Instructions1. Discover all comments
2. For each comment, do the following:
3. Types of issues to handle:
4. Do NOT:
After making changes
Context
|
Agent-Logs-Url: https://github.com/Preponderous-Software/roam/sessions/6af43ff9-aee8-4883-a3c0-991f3c47572a Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
Addressed all open review threads in c09b2fe:
|
On player death, items were scattered individually across the room, requiring tedious one-by-one retrieval after respawn. This PR replaces that behavior with a
Gravestoneentity that holds all dropped items and returns them in bulk on right-click.New entities
StorableInventory(src/entity/storableInventory.py) — mixin that owns anInventoryand exposesgetStoredInventory(); decoupled for reuse by the futureChestentityGravestone(src/entity/gravestone.py) — solid, non-pickupable, non-craftable; extendsDrawableEntity+StorableInventoryassets/images/gravestone.pngDeath behavior (
worldScreen.py)respawnPlayer()now transfers the full player inventory into aGravestone's stored inventory and places it at the player's last location instead of scattering items. No gravestone is spawned if the inventory was empty.Interaction
Right-clicking a
Gravestonein the world transfers all stored items to the player's inventory and removes the gravestone. If any item doesn't fit, the gravestone is left in place with"Inventory full"status — no partial transfer. The capacity pre-check (_inventoryCanFitAll) usesInventorySlot.getMaxStackSize()so it stays in sync with the inventory's actual stacking limits.Persistence
RoomJsonReaderWriterserializes/deserializesGravestoneentities with their full stored inventory contents:{ "entityClass": "Gravestone", "storedInventory": { "inventorySlots": [ { "slotIndex": 0, "slotContents": [{ "entityClass": "Apple", ... }] } ] } }New helpers:
_generateJsonForStoredInventory,_restoreStoredInventory,_createStoredItem. On load,_restoreStoredInventorylogs an error if an item cannot be placed (e.g. corrupt save), and_createStoredItemraisesValueErrorfor unknown entity classes rather than silently dropping items.Tests
tests/entity/test_gravestone.py— initialization, storage isolation, multi-item storagetests/screen/test_worldScreen_gravestone.py— spawn on death, empty-inventory guard, item retrieval, full-inventory guardtests/world/test_roomJsonReaderWriter.py— 4 new cases covering JSON generation, deserialization, and round-trip fidelity