tstand90 / tcod_tutorial_v2 Goto Github PK

By default variables returned by tcod.event are not typed:

for event in tcod.event.wait():
    ...  # event is type Any
    if event.type == "KEYDOWN":
        ...  # event is type Any, there is no static type checking for it in this block.

Something which can be done is to use isinstance to differentiate them. I used to avoid this function a lot since it had potential to mess up Python's duck-typing, but things have improved since then and now I just avoid it because I'm old:

for event in tcod.event.wait():
    ...  # event is type Any
    if isinstance(event, tcod.event.KeyDown):
        ...  # event is type KeyDown

Another option is tcod.event.EventDispatch, this class passes events to overridable methods based on their type, the class can also return a hinted type from those functions:

class EventHandler(tcod.event.EventDispatch[Action]):
    def ev_keydown(event: tcod.event.KeyDown) -> Optional[Action]:
        ... # event is type KeyDown


handler = EventHandler()
for event in tcod.event.wait():
    ...  # event is type Any
    action = handler.dispatch(event)
    ...  # action is type Optional[Action]
    if not action:
        continue
    ... # action is type Action

EventDispatch is nice since it can be subclassed into an abstract state class which can then be subclassed into a set of states. The last tutorial already used states but didn't use the double dispatch pattern. You can look at my implementation of states to see a likely over engineered example.

I went through this tutorial because a friend was asking for my opinions on it. There's a few things I'd have done differently, but for the most part nothing stood out as actually wrong. However part 12 has a couple things worthy of comment.

You use simple lists of 2-tuples for maximum item and monster counts, but dictionaries for spawn chances. Yet you only ever iterate through the dictionary like it was a list - there's no lookups by key and the data is static so the unique-key constraint is not utilized either. My instincts say that you should pick the simplest container which provides all the operations you need at an acceptable complexity, so the spawn chances should also be a list. In fact, using a list would allow using the bisect module from Python's standard library to quickly locate the most appropriate entry. With such small numbers of items this is of course rather inconsequential, but it could be an interesting side lesson about data structures and algorithms.

You're also passing the floor number to place_entities and calling get_max_value_for_floor from there. This looks up the max values again for every room, even though the results are the same every time. I would have preserved the parameters to place_entities and done the lookups in generate_dungeon instead, before the rooms loop.

Currently entities are stored in Engine.entities, but it might be better move this container to the GameMap class instead.

Until then the procgen.generate_dungeon function doesn't have enough scope to spawn new entities. This change would also make it easier to keep persistent maps, and you won't have to clear it manually when moving to a new level (part 11.)

I'd make a PR but I'm not sure where and when to make it right now. I have changes to part 2 here: part-2...HexDecimal:entities-in-map, but this would be easy to do in part 5.

I plan on making several changes to how the code is formatted and how errors are checked. Enforcing a constant formatting makes committing changes with Git easier. Tools that enforce error checking will prevent regressions in the tutorial code.

Other changes are to fix complaints people have had with the tutorial. Such as issues caused by the complexity of TYPE_CHECKING and how the tutorial does not go into how to make binary releases of the project.

• Update .gitignore and .gitattributes files to use common settings.
• Use Black and isort for consistent formatting. Line length will be set to 120.
• Try adding other Python linters: MyPy, Flake8, pylint.
• Setup PyInstaller early on in the tutorial so that deployable builds are available immediately.
• Remove reliance on TYPE_CHECKING. First-party from-imports will only be for objects required at import-time. isort will have from_first=true enabled. This works the best at removing issues from cyclic importing wherever they can't be resolved completely. Type checking is still needed, but its use will be more limited.
• Reorder most modules into sub-packages, so that there are far fewer top-level modules which should reduce name shadowing.
• Add GitHub workflows which will enforce the above linters and verify deployments.
• Add VS Code workspace settings for the above. This will be the easiest method to modify the tutorial by contributors, since PyCharm doesn't support workspace settings very well.
• Resolve all warnings such as deprecated tcod functions.
  • Frame titles need to be replaced with a custom banner.
• Add more docstrings to everything. Ideally every module and function would have a docstring.
• Use better color choices.
• Organize runtime assets into a data directory.
• Use the pathlib module for file paths.
• Apply the refactoring of part 6 and part 8 to the earlier steps of the tutorial.
• Refactor equipment_types.py.
• Switch font to a 16x16 font.
• Use builtin logging module for any logged info, avoid plain print calls.
• Add error code 0 to all raises of SystemExit. TStand90/roguelike-tutorials-website#26
• Fix hard-coded health bar size. TStand90/roguelike-tutorials-website#26
• Fix blast radius shape.

In the tutorial text, you say:

We’ve changed each entity to make use of the Actor class, and used the HostileEnemy AI class for the Orc and the Troll types, while using the BaseAI for our player.

However, the diff and the code currently in the repo has:

player = Actor(
    char="@", color=WHITE, name="Player", ai_cls=HostileEnemy, fighter=Fighter(hp=30, defence=2, power=5)
)

Just confirming, that the ai_cls for the player actor should indeed be set to BaseAI.

But, I was wonder if the ai_cls component should in fact be Optional, as my first though was I'm the AI, and so the AI component should be set to None for the player actor. Though this would also require changing the implementation of the is_alive property on the Actor class.

I notice most of the time we go to engine for game_map, even though it's accessible via entity. Would you mind taking a moment to explain the difference in thought behind when you might access it via one or the other?

Thank you

The modern API does FOV with tcod.map.compute_fov. The syntax will be familiar, you just need to add in the transparency array.

The visible/explored arrays should be added as separate arrays, not added in the tile dtype.

A memory array could be added with a graphics dtype and filled with a blank shroud graphic. This array would hold the dark graphics and be used for non-visible areas of the map. You could also put item and enemy graphics here so that they show as ghosts when out of view. The memory array can hold out of date data so that changes to the tiles array are not reflected until the player sees the changes themselves.

np.where will be used a lot here. Think of it like a ternary operator. out = (cond ? a : b) is similar to out = np.where(cond, a, b) but np.where works on entire arrays.

In the function, names is capitalized after joined together. If there are two entities there only first letter is capitalized.
It is better to capitalize then join them.

From: (Player remains of troll)

names = ", ".join(
        entity.name for entity in game_map.entities if entity.x == x and entity.y == y
    )

    return names.capitalize()

To: (Player Remains of troll)

names = ", ".join(entity.name.capitalize() for entity in game_map.entities
                      if entity.x == x and entity.y == y)

    return names

Python 3.10 will break with other parts of the code such as.

for entity in self.game_map.entities - {self.player}:
TypeError: unsupported operand type(s) for -: 'list' and 'set'

It seems to be a simple fix - just declare it as {player}. I didn't make a pull request because I don't know in which part this "mistake" began, or if there's something else I'm missing.

I had to use shift+key (for example shift+a) to use and drop inventory items.
Issue was in repl.it.
I added tcod.event.K_MODE to ignored modifier keys to solve.

class AskUserEventHandler(EventHandler):
    """Handles user input for actions which require special input."""

    def ev_keydown(self, event: tcod.event.KeyDown) -> Optional[ActionOrHandler]:
        """By default any key exits this input handler."""
        if event.sym in {  # Ignore modifier keys.
            tcod.event.K_LSHIFT,
            tcod.event.K_RSHIFT,
            tcod.event.K_LCTRL,
            tcod.event.K_RCTRL,
            tcod.event.K_LALT,
            tcod.event.K_RALT,
        }:
            return None
        return self.on_exit()

In the v1 tutorial entities end up being defined where they're spawned, such as orcs and trolls being defined in the middle of the dungeon generator code.

I'd like it if these definitions were moved to a specific place and could be referenced by the generator when they're needed, but I also don't want to lose the ability to make new entities on demand. I think this can be done with a careful prototype pattern.

# Defined in a Python module, but could also be loaded from tabulated data:
orc = Entity(name="Orc", char="o", color=(63, 127, 63))

orc.spawn(gamemap, x, y)  # Place a copy of orc at this location.

Care will need to be taken with mutable objects. deepcopy could be used, but it's inefficient since a lot of data will be shared between entities. I'm not sure if x and y will need to be given to the Entity constructor anymore.

Another thing is that Entity will expand to include a variety of object types, but the previous tutorial uses Optional typed attributes for all of them and the type-checking will need to be told which attribute isn't None every time.

class Entity:
    fighter: Optional[Fighter]


# An example that verifies Entity.fighter and calls another function.
def attack(attacker: Entity, defender: Entity) -> None:
    if attacker.fighter and defender.fighter:
        do_damage(attacker, defender)


# An example which makes assumptions about the entities attributes.
def do_damage(attacker: Entity, defender: Entity) -> None:
    # Type errors: attacker.fighter and defender.fighter could be None, because those types are Optional.
    defender.fighter.hp -= attacker.fighter.atk
    if defender.fighter.hp <= 0:
        kill(defender)  # Fighter type can't be passed directly because of this use of Entity.


def do_damage(attacker: Entity, defender: Entity) -> None:
    # Fixed, but dubious.
    assert attacker.fighter
    assert defender.fighter
    ...

This issue will repeat for every function which must assume that those attributes are not None and that will come more often once functions and methods are added to work with fighters and items specifically.

I think that best way to handle them would be with a class hierarchy:

class Actor(Entity):
    """Entities which perform actions."""

    def __init__(char: str, color: Tuple[int, int, int], ...):
        super().__init__(char=char, color=color)
        # All attributes are non-Optional.
        self.fighter = ...
        self.inventory = ...


def attack(attacker: Entity, defender: Entity) -> None:
    if isinstance(attacker, Actor) and isinstance(defender, Actor):
        do_damage(attacker, defender)


def do_damage(attacker: Actor, defender: Actor) -> None:
    ...

This is similar to the current use of Entity, allows the above prototypes, and it gives a clear type for entities which include attributes such as fighter.

I'm not sure about items yet. That will need to be its own post when it comes up.

Enemy turns were handed with a state in v1, but I think states should only be used to handle different kinds of player input and that enemy turn should be a method to call after the player finishes an action.

Actions might end up removing an entity from the entities set while it's being iterated over. So a copy of the set needs to be used for iteration. for entity in list(self.game_map.entities): would work but for entity in self.game_map.entities - {self.player}: makes a copy and prevents the need to compare every entity to player.

def enemy_turn(self) -> None:
    for entity in self.game_map.entities - {self.player}:
        if not isinstance(entity, Actor):
            continue  # This entity can't perform actions.
        if entity.fighter.hp <= 0:
            continue  # Entity was killed during this loop.
        ...  # Take action.

isinstance is a fairly slow check here. The alternatives would be to keep a separate set of Actors or to give Entity an on_turn method overridden by Actor, but I don't think the performance would be bad enough to require those.

The Diff tab for LightningDamageConsumable does not match the Original tab for LightningDamageConsumable. Following the LightningDamageConsumable Diff tab's code will result in three errors/bugs:

First, the code will not recognize distance() as an attribute of consumer.distance(). This is solved by changing "consumer: Actor" to "action: actions.ItemAction" in the function's incoming variables.
Second, the code will raise a notimplementederror() when activating the item. This can be solved by changing the function's name from "consume" to "activate".
Finally, the item will not leave the inventory. This can be solved by adding "self.consume" after "target.fighter.take_damage(self.damage)".

All of these are implemented in the Original tab for LightningDamageConsumable. Perhaps the Diff tab is referencing an older version of the code?

In part 5, what happens when the player picks a direction becomes context sensitive, and I think that the actions should reflect this.

The current movement could be changed into generic "bump" action. This action selects another action to perform based on the context (move onto tile, attack entity, open door.) Since multiple actions will have dx and dy it might be a good idea to build a hierarchy of actions, for example:

class ActionWithDirection(Action):
    """An action with relative direction parameters."""

    def __init__(self, dx: int, dy: int):
        super().__init__()
        self.dx = dx
        self.dy = dy

class BumpAction(ActionWithDirection):
    """Perform a context sensitive action based on a direction."""

    def perform(self, engine: Engine, entity: Entity) -> None:
        if ...:  # Enemy fighter is here.
            return MeleeAction(self.dx, self.dy).perform(engine, entity)

        # Default case, try to move onto the tile.
        return MovementAction(self.dx, self.dy).perform(engine, entity)

Say you have more than one of any item. If you were to drop them and try to pick them back up, only one of that item actually exists now on the game map.

Edit: I do believe I have traced the cause: items on the gamemap are stored in sets, which does not allow duplicates.

As a side note, I added the following to fighter.py under def die:

        for i in self.parent.inventory.items:
            self.parent.inventory.drop(i)

This also has weird behavior where it won't drop certain items (never seems to drop the second item in the list, and only dropped 3 of 5 potions, for example). I know this isn't a part of the tutorial but there seems to be some underlying issues that this bit of code is revealing.

Hello!

I'm getting the following error when trying to run the project after finishing part 6:

Here's the lambda part in game_map:

I've double-checked it all from the start and couldn't figure out what is the problem, even comparing the files from the repository version (Which in contrast is running fine).

I'm running Python 3.9.0 and not using a virtual environment.

Hey all. Thank you so much for the enlightening tutorial.

This snippet got me thinking:

class Fighter(BaseComponent):
    entity: Actor

As the Fighter class uses properties of Actor that do not exist in Entity, would it be more proper to name this property actor instead?

I know this type of feedback can come across as criticism, but I genuinely do not know as I am a novice when it comes to python (outside of data processing).