Implement actions and global refactoring

[gcoter]

It was simpler for me to treat several issues at the same time to have the global picture in mind, sorry for the big PR 😁

Implement these issues:

• Add get_power and get_toughness #8
• Create a class PhaseWherePlayersGetPriority #13
• Create a class for player actions #15
• List possible actions #16
• Implement combat #17
• Implement a RandomPlayer #21

[MaximeBastion]

A first few comments from what I looked into

[MaximeBastion]

We need to keep in mind that, during the damage step, all damage is dealt simultaneously, we don't check if a creature or player is dead between the damage dealt by creature 1 and creature 2 (unless one of the two has first strike or double strike)

[gcoter]

Ok understood, is it better like this?
2d284f4

(I know there is a lot of code duplication here, I will rewrite the code more elegantly once we agree on the implementation)

[MaximeBastion]

Nope, we resolve a single element
Then, both players need to pass priority consecutively to resolve the next element

[gcoter]

Ok thanks. Actually, I have in mind that for each subclass of Action, we could list the possible "counter action", i.e. the other Action subclasses which correspond to possible reactions of the other players. So I assume that for instance, in this particular example, when an element on the stack is resolved the players can either play an instant, or activate an ability or pass priority (I might be wrong but I hope you get the general idea of listing possible answers). In that case, I could add a method in the while loop here to check for actions from the players before resolving the next element

[MaximeBastion]

I don't know that it makes sense to talk about counter actions because the possibilities are not that different when a spell is on the stack VS not (except that sorcery speed things cannot be done even during the main phase, and that spells and abilities that targets stuff on the stack, like counterspells, can be played since there are legal targets)

[gcoter]

Ok I see. I guess what is missing currently is that instead of just asking the current player to do actions we should have a for loop on the players (where the current player is the first one) and each player can choose an action. With this process, it would allow other players to do actions depending on the action of the current player while keeping the implementation generic, without this concept of "counter action"

[MaximeBastion]

I'd say no: a land can be tapped by another effect without producing mana
For instance, with Ice https://scryfall.com/card/mh2/290/fire-ice someone (btw hello split cards that we probably won't support :p)
It's the effect of the activated ability t: add W from a plains for instance that produces mana, not the fact that it gets tapped

[MaximeBastion]

Exactly like with Llanowar Elves https://scryfall.com/card/gnt/46/llanowar-elves

[gcoter]

Yes exactly, it is the activated ability that produces mana, however we currently don't support abilities at all, this is why I marked it as a FIXME 😅 Yesterday I was hesitating, like maybe for now we could create mana as the result of tapping lands, otherwise we need to start thinking about the logic for abilities. I don't know which way is best

[gcoter]

split cards

WTF? One card actually being two cards? 😁

[MaximeBastion]

Oh sure we could have land tapping = mana produced for now

[gcoter]

I did a naive implementation of this in this commit: 38e54d7

[MaximeBastion]

If I understood the code well, there is an issue here: here this list is a list of this creature attacks this player like

• creature a attacks player B
• creature a attacks player C
• creature b attacks player B
• creature b attacks player C
  And then the random IA will choose one of these.
  But isn't what we want to choose at random a set of "this creature attacks this player" rather than a single one?
  Like, option 1 could be "a attacks B and b attacks C"
  Also, we need to support options where a creature does not attack, like the set "a attacks B"

[gcoter]

Yes you are right, the players needs to be able to declare several attackers. The way I chose to do it (because I found it easier to implement although I do understand that it is not clear without explanations) is to use a while loop in the run method:

def run(self, game_state: GameState) -> GameState:
        while True:
            possible_actions = CombatDeclareAttackersPhase.list_possible_actions(game_state)
            current_player = game_state.current_player
            action = current_player.choose_action(
                game_state=game_state,
                possible_actions=possible_actions,
            )
            if action is None:
                break
            game_state = action.execute(game_state)
        return game_state

Here is the process:

• Current player gets to choose one DeclareAttackerAction action among the possible actions
• If the chosen action is None, we consider the player stops declaring attackers (btw I realize that I did not include this in the implementation of the Player, so you are right it would be nice to have an action to mean "I don't want to declare more attackers")
• Otherwise, the chosen action is applied. You can see how I implement the declaration of the attackers in magic_the_gathering/actions/declare_attacker.py.
• Now we enter the loop again, we list the possible actions (which is different since the previous action affected the game state so that one card is already an attacker and thus cannot be chosen again)
• The current player gets to choose whether it declares one more attacker
• Etc...

For me it was simpler to implement it this way. The DeclareAttackerAction is more "atomic", we support the declaration of several attackers and it means that on the player side everything looks like choosing one action at a time (which is simpler than deciding a set of actions at once).

However, if you find issues with this implementation, we can discuss of course 🙂

[gcoter]

I think I found how to handle the "I don't want to declare attackers anymore":

@staticmethod
    def list_possible_actions(game_state: GameState) -> List[Action]:
        possible_actions = [None]
        possible_actions.extend(DeclareAttackerAction.list_possible_actions(game_state=game_state))
        return possible_actions

If there is one None element in the list, then the player can choose it and then it will be interpreted in the run method as I explained before 🙂

[gcoter]

I added None to the possible actions in all the relevant phases. Of course we can change this if you think there is a better way 😉

[MaximeBastion]

Ah ok
One problem I can think of with this approach is that it seems that there is a need to check if a "sub-battle" is legal, with a sub-battle being one attacker being blocked by one or more blockers.
Ex: menace makes a creature only be blockable by 2 creatures or more.
This makes it so some combination of blocks may be individually legal, but not legal when looked at together.
And menace is common.
Like an attacker with menace attacks, I have a single creature, I list the possible first blocks I could take, I block it with one creature, but then I can't block it with more, and my blocking combination is illegal.

[gcoter]

Ah, I see! I was not aware of that. I will create an issue for this, thank you 🙂

[gcoter]

#27

[gcoter]

Could you describe what you did in notebooks/explore-combat-logic.py? If I understand well, you try to compute all the possible sets of attackers and blockers.

I am wondering whether my current code is not already doing something similar (using the logic of while loop I described here #24 (comment)). Actually, this is also to avoid having to compute all the possible sets of attackers or blockers that I decided to use atomic actions and a while loop to iteratively declare them and check progressively what are the possible actions left. What do you think?

[gcoter]

Here is some background about my thinking process:

• An Action subclass is a rather "atomic" modification of the GameState. Each Action subclass is able to list all its possible instances for a given GameState. Each time an action is executed, an history is updated and we check whether the game is over

def execute(self, game_state: GameState) -> GameState:
    game_state = self._execute(game_state)
    game_state.action_history.append(self)
    game_state.check_if_game_is_over()
    return game_state

• A Phase is part of a Turn, it modifies the GameState using Action instances. However, there are two kinds of Phase:
  • Phase, the base class, is very basic and will typically correspond to "automatic" phases where the game does things without the players intervention
  • PhaseWherePlayersGetPriority corresponds to phases in which the players get to choose which action to run. Since the players need to be able to choose from a list of possible Action instances, we implement for each PhaseWherePlayersGetPriority subclass a static method which lists the possible actions. Typically, this is quite equivalent to calling list_possible_actions of one Action class, however we can have exceptions like MainPhase during which two kinds of actions are currently supported (PlayLandAction and CastCardAction). Currently, PhaseWherePlayersGetPriority is the parent class of the main phase and most of combat phases. In general during these phases, the players can choose a sequence of actions, this is why the run method will typically include a while loop which is stopped once the chosen action is None.
• Thanks to the efforts made on the Action classes being atomic and the Phase classes using while loops to ask iteratively for actions, the Player logic is actually quite simple: it just implements a method choose_action which will return one Action given a GameState (which is assumed to contain all the necessary information) and a list of possible Action instances.
  • In particular, the RandomPlayer chooses one action randomly from the possible actions, independently from the provided GameState

In this implementation, most of the complexity is in the Action classes. Using the list_possible_actions we define the legal actions. Then the Phase subclasses are just here to orchestrate the execution of Action instances by following the turn order and by asking the players what they want to do.

Of course here, we don't implement abilities and also we miss the possible "reactions" of players. However, I think they are feasible:

• Regarding the abilities, they could be another kind of Action caused by the action TriggerAbilityAction for instance
• Regarding the possible reactions, we could add a method in Action to list them. Basically, similarly to listing the legal instances of an Action, we could list the possible Action instances which "respond" to one Action of a given subclass. For instance, in reaction to CastCardAction, I assume that CastInstantAction (not implemented yet) could be a possible reaction (maybe I'm wrong but you get the idea)

[gcoter]

Regarding the mana, there are several things which are missing:

1. As we said above, the mana is not produced through an ability for now
2. In Magic, Lands can produce mana of an undefined color (which can chosen by the player) and the mana cost of a card can mix the costs of specific colors and unspecified colors (for instance: {1}{W}). The current code cannot handle unspecified colors properly
3. The mana pool is reset during the UntapPhase, it is probably not the right place to do this?

[gcoter]

Another thing which is missing in the current implementation. The stack is only used during the MainPhase. I assume that several other elements of the game in the other phases should also transit through the stack (it is highly probable I forgot a lot of things like this...).

Similarly, the stack is resolved only during the MainPhase and it is resolved all at once (without allowing players to "react" after each resolution). I suppose that normally the stack should be resolved more regularly? It is not really clear in my mind 😅

[MaximeBastion]

Regarding the mana, there are several things which are missing:

1. As we said above, the mana is not produced through an ability for now
2. In Magic, Lands can produce mana of an undefined color (which can chosen by the player) and the mana cost of a card can mix the costs of specific colors and unspecified colors (for instance: {1}{W}). The current code cannot handle unspecified colors properly
3. The mana pool is reset during the UntapPhase, it is probably not the right place to do this?

3 -> The mana pool is emptied when going from any phase to the next one

[MaximeBastion]

Another thing which is missing in the current implementation. The stack is only used during the MainPhase. I assume that several other elements of the game in the other phases should also transit through the stack (it is highly probable I forgot a lot of things like this...).

Similarly, the stack is resolved only during the MainPhase and it is resolved all at once (without allowing players to "react" after each resolution). I suppose that normally the stack should be resolved more regularly? It is not really clear in my mind 😅

The stack can get used and resolved basically anytime players can get priority, and resolve one element at a time

[MaximeBastion]

Could you describe what you did in notebooks/explore-combat-logic.py? If I understand well, you try to compute all the possible sets of attackers and blockers.

I am wondering whether my current code is not already doing something similar (using the logic of while loop I described here #24 (comment)). Actually, this is also to avoid having to compute all the possible sets of attackers or blockers that I decided to use atomic actions and a while loop to iteratively declare them and check progressively what are the possible actions left. What do you think?

Yes, that's roughly what my code does, coming from the perspective that individual blocks must be legal, but "sub-battles" too, like with menace