Entity system#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an experimental, data-driven entity system intended to centralize tile/entity configuration (sprite frames, XP/levels, placement “satisfaction” rules) and to replace many per-entity subclasses with factory-based creation.
Changes:
- Added
entity_definitions.pywith anEntityDefregistry, satisfaction callbacks, and anEntityFactoryfor creatingBoardTileinstances. - Refactored creatures/items/obstacles/spells modules from
BoardTilesubclasses into factory functions that delegate toentity_factory. - Updated
DungeonFloor.add_tile()to support callable factories and updatedBoardTileto load data fromEntityDefand use data-driven satisfaction.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dragonsweepyr/monsters/tiles.py | Adds load_from_def() and data-driven satisfaction support on BoardTile. |
| src/dragonsweepyr/monsters/entity_definitions.py | Introduces entity registry + factory + sprite coord loading and satisfaction callbacks. |
| src/dragonsweepyr/monsters/creatures.py | Replaces creature subclasses with factory functions using entity_factory. |
| src/dragonsweepyr/monsters/items.py | Replaces item subclasses with factory functions using entity_factory. |
| src/dragonsweepyr/monsters/obstacles.py | Replaces obstacle subclasses with factory functions using entity_factory. |
| src/dragonsweepyr/monsters/spells.py | Replaces spell subclasses with factory functions using entity_factory. |
| src/dragonsweepyr/monsters/init.py | Exposes entity_factory, registry, and types at the package level. |
| src/dragonsweepyr/monsters/entity_examples.py | Adds an example module demonstrating entity creation/usage patterns. |
| src/dragonsweepyr/dungeon.py | Extends add_tile() to support callable entity factories during dungeon generation. |
| IMPLEMENTATION_COMPLETE.md | Documents the refactor and intended benefits. |
| ENTITY_SYSTEM_MIGRATION.md | Provides migration guidance and usage patterns. |
| ARCHITECTURE_IMPROVEMENTS.md | Expanded architectural rationale and examples for the new system. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.minotaurChestLocation = [-1, -1] | ||
| self._satisfaction_func = None | ||
|
|
There was a problem hiding this comment.
self._satisfaction_func is introduced without a type annotation, even though EntityDef.satisfaction_func is typed. Adding an explicit type (e.g., Callable[[BoardTile], int] | None) will improve readability and static checking, especially since satisfaction() now depends on it.
| elif callable(tile_class): | ||
| # It's a factory function, call it with appropriate kwargs | ||
| # Extract constructor-specific kwargs that the factory function expects | ||
| factory_kwargs = {} | ||
| constructor_params = ['monster_level', 'xp', 'contains'] | ||
| for param in constructor_params: | ||
| if param in kwargs: | ||
| factory_kwargs[param] = kwargs.pop(param) | ||
| tile = tile_class(**factory_kwargs) |
There was a problem hiding this comment.
In the callable(factory) branch, this loop mutates the shared kwargs dict via pop(). When count > 1, only the first created tile will receive monster_level/contains (etc.); subsequent iterations will silently fall back to factory defaults and won’t get those properties set later because they were removed. Use a per-iteration copy of kwargs (or avoid popping entirely) so each tile gets the same inputs.
| # It's a factory function, call it with appropriate kwargs | ||
| # Extract constructor-specific kwargs that the factory function expects | ||
| factory_kwargs = {} | ||
| constructor_params = ['monster_level', 'xp', 'contains'] | ||
| for param in constructor_params: | ||
| if param in kwargs: | ||
| factory_kwargs[param] = kwargs.pop(param) | ||
| tile = tile_class(**factory_kwargs) |
There was a problem hiding this comment.
This factory-call path hardcodes constructor_params = ['monster_level', 'xp', 'contains'] and forwards those into the factory. If a caller passes e.g. xp=... to add_tile() for a factory that doesn’t accept xp, this will raise TypeError (previously xp could be applied as an attribute after construction). Consider filtering kwargs based on the callable’s signature (e.g. inspect.signature) and leaving unsupported keys to the attribute-setting loop.
| from dragonsweepyr.monsters import TileID, entity_factory | ||
|
|
||
| # Create an entity by ID | ||
| bat = entity_factory.create(TileID.Bat) | ||
| print(f"Created {bat.name} at sprite frame {bat.strip_frame}") | ||
|
|
||
| # Create an entity by name | ||
| dragon = entity_factory.create_by_name("Dragon") | ||
| print(f"Dragon has {dragon.xp} xp") | ||
|
|
There was a problem hiding this comment.
This module executes a large amount of example code at import time (entity creation + print() calls, list comprehensions, etc.). Because it lives under src/dragonsweepyr/monsters/, importing it (directly or indirectly) will produce side effects and slow down startup. Move these examples into documentation/tests, or guard all executable example code under if __name__ == '__main__':.
| sprite_frame=24, | ||
| is_monster=False, |
There was a problem hiding this comment.
TileID.Treasure is registered with xp=0 (default) and sprite_frame=24 (the xp=5 frame), but the legacy Treasure() factory defaults to xp=1 and uses frame 30. As a result, entity_factory.create(TileID.Treasure) produces a different default treasure than the rest of the code expects. Align the registry defaults with the legacy behavior (e.g., default xp=1 + frame 30), and keep alternate frames in metadata for other xp values.
| sprite_frame=24, | |
| is_monster=False, | |
| sprite_frame=30, | |
| is_monster=False, | |
| xp=1, |
| ValueError: If name is not found in registry. | ||
| """ | ||
| for definition in self.registry.values(): | ||
| if definition.name == name: |
There was a problem hiding this comment.
create_by_name() matches entity names case-sensitively (definition.name == name). Given that BoardTile.load_from_def() lowercases tile.name, and callers may naturally pass lowercase names, consider making this lookup case-insensitive (e.g., compare .lower() on both sides) or document the exact casing required.
| if definition.name == name: | |
| if definition.name.lower() == name.lower(): |
| def create(self, tile_id: int, **kwargs) -> BoardTile: | ||
| """ | ||
| Create a board tile from an entity definition. | ||
|
|
||
| Args: | ||
| tile_id: The TileID to create. | ||
| **kwargs: Additional arguments to pass to the BoardTile constructor. | ||
|
|
||
| Returns: | ||
| A BoardTile instance initialized from the entity definition. | ||
|
|
||
| Raises: | ||
| ValueError: If tile_id is not in the registry. | ||
| """ | ||
| from dragonsweepyr.monsters.tiles import BoardTile | ||
|
|
||
| definition = self.get_definition(tile_id) | ||
| if not definition: | ||
| raise ValueError(f"Unknown entity ID: {tile_id}") | ||
|
|
||
| tile = BoardTile() | ||
| tile.load_from_def(definition) | ||
|
|
||
| # Apply any additional kwargs | ||
| for key, value in kwargs.items(): | ||
| if hasattr(tile, key): | ||
| setattr(tile, key, value) | ||
|
|
There was a problem hiding this comment.
EntityFactory.create() docstring says **kwargs are passed to the BoardTile constructor, but the implementation always constructs BoardTile() with no kwargs and then sets attributes. Update the docstring to reflect the actual behavior (attribute overrides), or change the implementation to forward supported kwargs into BoardTile.__init__ if that’s the intent.
Experimental system to streamline tile (entity) creation and storage