Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/disallow immediate dev card #290

Closed
wants to merge 5 commits into from
Closed

Feature/disallow immediate dev card #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d98b96d
Disallow player to play dev card same turn
zarns Nov 3, 2024
63cbaa2
Allow player to play dev cards before rolling
zarns Nov 3, 2024
cb4b99c
Display Select action instead of Roll if playing dev cards before rol…
zarns Nov 3, 2024
3b2f398
hide buttons while playing dev cards
zarns Nov 5, 2024
b8e9b9b
Allow playing dev cards before rolling. Remove duplicate logic
zarns Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 16 additions & 19 deletions catanatron_core/catanatron/models/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,26 @@ def generate_playable_actions(state) -> List[Action]:
elif action_prompt == ActionPrompt.MOVE_ROBBER:
return robber_possibilities(state, color)
elif action_prompt == ActionPrompt.PLAY_TURN:
actions = []
# Allow playing dev cards before and after rolling
if player_can_play_dev(state, color, "YEAR_OF_PLENTY"):
actions.extend(year_of_plenty_possibilities(color, state.resource_freqdeck))
if player_can_play_dev(state, color, "MONOPOLY"):
actions.extend(monopoly_possibilities(color))
if player_can_play_dev(state, color, "KNIGHT"):
actions.append(Action(color, ActionType.PLAY_KNIGHT_CARD, None))
if (
player_can_play_dev(state, color, "ROAD_BUILDING")
and len(road_building_possibilities(state, color, False)) > 0
):
actions.append(Action(color, ActionType.PLAY_ROAD_BUILDING, None))

if state.is_road_building:
actions = road_building_possibilities(state, color, False)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scenario they are road building, I think I'd like the game state to stay focused on that what do you think? Feels a bit off to: Play Road Building, Place a Road, Play Knight, ... It might lead to subtle bugs of states we are not thinking about.

Could you keep this branch of state.is_road_building to only return those options? Thanks!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe then the final structure is something like:

if state.is_road_building:
   return road_building_possibilities...
else:
  # Allow playing dev cards before and after rolling
  ...
  if not player_has_rolled(state, color):
    actions.append(ROLL)
   else:
      build houses, roads, cities...
  return actions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Didn't realize this testing thru the UI because is_road_building prevents the UI player from doing anything else

elif not player_has_rolled(state, color):
actions = [Action(color, ActionType.ROLL, None)]
if player_can_play_dev(state, color, "KNIGHT"):
actions.append(Action(color, ActionType.PLAY_KNIGHT_CARD, None))
actions.append(Action(color, ActionType.ROLL, None))
else:
actions = [Action(color, ActionType.END_TURN, None)]
actions.append(Action(color, ActionType.END_TURN, None))
actions.extend(road_building_possibilities(state, color))
actions.extend(settlement_possibilities(state, color))
actions.extend(city_possibilities(state, color))
Expand All @@ -70,21 +82,6 @@ def generate_playable_actions(state) -> List[Action]:
if can_buy_dev_card:
actions.append(Action(color, ActionType.BUY_DEVELOPMENT_CARD, None))

# Play Dev Cards
if player_can_play_dev(state, color, "YEAR_OF_PLENTY"):
actions.extend(
year_of_plenty_possibilities(color, state.resource_freqdeck)
)
if player_can_play_dev(state, color, "MONOPOLY"):
actions.extend(monopoly_possibilities(color))
if player_can_play_dev(state, color, "KNIGHT"):
actions.append(Action(color, ActionType.PLAY_KNIGHT_CARD, None))
if (
player_can_play_dev(state, color, "ROAD_BUILDING")
and len(road_building_possibilities(state, color, False)) > 0
):
actions.append(Action(color, ActionType.PLAY_ROAD_BUILDING, None))

# Trade
actions.extend(maritime_trade_possibilities(state, color))
return actions
Expand Down
4 changes: 4 additions & 0 deletions catanatron_core/catanatron/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@
# de-normalized features (for performance since we think they are good features)
"ACTUAL_VICTORY_POINTS": 0,
"LONGEST_ROAD_LENGTH": 0,
"KNIGHT_PURCHASED_THIS_TURN": 0,
"MONOPOLY_PURCHASED_THIS_TURN": 0,
"YEAR_OF_PLENTY_PURCHASED_THIS_TURN": 0,
"ROAD_BUILDING_PURCHASED_THIS_TURN": 0,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share more about the motivation of these? I believe this is what the _HAS_PLAYED_DEVELOPMENT_CARD_IN_TURN state flag is going after (to know if player can play again). Could we use the existing _HAS_PLAYED_DEVELOPMENT_CARD_IN_TURN instead?

Copy link
Contributor Author

@zarns zarns Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a player has a knight card at the start of their turn, then purchases 2 knight cards, they should still be able to play the one knight card.
If a player has no knight card at the start of their turn, then purchases 2 knight cards, they should not be able to play them in the same turn.
This is to disallow them from purchasing/using on the same turn. The other way to handle it would be to track which dev cards they started with, but this seemed more intuitive to me at the time.

Alternatively, we could have something like KNIGHT_OWNED_AT_START_OF_TURN, and check in player_can_play_dev() for:
{ KNIGHT_OWNED_AT_START_OF_TURN && !HAS_PLAYED_DEVELOPMENT_CARD_IN_TURN }

Looking back, maybe that approach is more straightforward

}
for resource in RESOURCES:
PLAYER_INITIAL_STATE[f"{resource}_IN_HAND"] = 0
Expand Down
18 changes: 17 additions & 1 deletion catanatron_core/catanatron/state_functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,17 @@ def player_resource_freqdeck_contains(state, color, freqdeck):

def player_can_play_dev(state, color, dev_card):
key = player_key(state, color)

cards_in_hand = state.player_state[f"{key}_{dev_card}_IN_HAND"]
cards_purchased_this_turn = state.player_state[
f"{key}_{dev_card}_PURCHASED_THIS_TURN"
]
playable_cards = cards_in_hand - cards_purchased_this_turn

return (
not state.player_state[f"{key}_HAS_PLAYED_DEVELOPMENT_CARD_IN_TURN"]
and state.player_state[f"{key}_{dev_card}_IN_HAND"] >= 1
# Must have at least 1 card that wasn't purchased this turn
and playable_cards > 0
)


Expand Down Expand Up @@ -259,6 +267,9 @@ def buy_dev_card(state, color, dev_card):
state.player_state[f"{key}_{dev_card}_IN_HAND"] += 1
if dev_card == VICTORY_POINT:
state.player_state[f"{key}_ACTUAL_VICTORY_POINTS"] += 1
else:
# Mark as purchased this turn
state.player_state[f"{key}_{dev_card}_PURCHASED_THIS_TURN"] += 1

state.player_state[f"{key}_SHEEP_IN_HAND"] -= 1
state.player_state[f"{key}_WHEAT_IN_HAND"] -= 1
Expand Down Expand Up @@ -334,3 +345,8 @@ def player_clean_turn(state, color):
key = player_key(state, color)
state.player_state[f"{key}_HAS_PLAYED_DEVELOPMENT_CARD_IN_TURN"] = False
state.player_state[f"{key}_HAS_ROLLED"] = False
# Reset all purchased-this-turn counters
state.player_state[f"{key}_KNIGHT_PURCHASED_THIS_TURN"] = 0
state.player_state[f"{key}_MONOPOLY_PURCHASED_THIS_TURN"] = 0
state.player_state[f"{key}_YEAR_OF_PLENTY_PURCHASED_THIS_TURN"] = 0
state.player_state[f"{key}_ROAD_BUILDING_PURCHASED_THIS_TURN"] = 0
5 changes: 1 addition & 4 deletions tests/test_game.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,7 @@ def test_play_road_building(fake_roll_dice):
):
game.play_tick()

# roll not a 7
fake_roll_dice.return_value = (1, 2)
game.play_tick() # roll

# Play Road Building before rolling
game.execute(Action(p0.color, ActionType.PLAY_ROAD_BUILDING, None))
assert game.state.is_road_building
assert game.state.free_roads_available == 2
Expand Down
18 changes: 9 additions & 9 deletions ui/src/pages/ActionsToolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ function PlayButtons() {
[enqueueSnackbar, closeSnackbar]
);

const { gameState, isPlayingMonopoly, isPlayingYearOfPlenty } = state;
const { gameState, isPlayingMonopoly, isPlayingYearOfPlenty, isRoadBuilding } = state;
const key = playerKey(gameState, gameState.current_color);
const isRoll =
gameState.current_prompt === "PLAY_TURN" &&
Expand Down Expand Up @@ -198,51 +198,51 @@ function PlayButtons() {
return (
<>
<OptionsButton
disabled={playableDevCardTypes.size === 0 || isPlayingYearOfPlenty}
disabled={playableDevCardTypes.size === 0 || isPlayingMonopoly || isPlayingYearOfPlenty || isRoadBuilding}
menuListId="use-menu-list"
icon={<SimCardIcon />}
items={useItems}
>
Use
</OptionsButton>
<OptionsButton
disabled={buildActionTypes.size === 0 || isPlayingYearOfPlenty}
disabled={buildActionTypes.size === 0 || isPlayingMonopoly || isPlayingYearOfPlenty || isRoadBuilding}
menuListId="build-menu-list"
icon={<BuildIcon />}
items={buildItems}
>
Buy
</OptionsButton>
<OptionsButton
disabled={tradeItems.length === 0 || isPlayingYearOfPlenty}
disabled={tradeItems.length === 0 || isPlayingMonopoly || isPlayingYearOfPlenty || isRoadBuilding}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I'd be awesome if we can give the isPlayingMonopoly || isPlayingYearOfPlenty || isRoadBuilding expression a name. Can you think of one? I can't 😅 . If not, that's ok, we don't have to force it, as is its good.

menuListId="trade-menu-list"
icon={<AccountBalanceIcon />}
items={tradeItems}
>
Trade
</OptionsButton>
<Button
disabled={gameState.is_initial_build_phase}
disabled={gameState.is_initial_build_phase || isRoadBuilding}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to see ResourceSelector "Select" button for Monopoly/YOP, hide other buttons while playing dev cards

variant="contained"
color="primary"
startIcon={<NavigateNextIcon />}
onClick={
isRoll
? rollAction
: isDiscard
isDiscard
? proceedAction
: isMoveRobber
? setIsMovingRobber
: isPlayingYearOfPlenty || isPlayingMonopoly
? handleOpenResourceSelector
: isRoll
? rollAction
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved isRoll to later in the ternary statements because if a dev card is played before rolling, we want to see the ResourceSelector button for monopoly/yearOfPlenty

: endTurnAction
}
>
{
isRoll ? "ROLL" :
isDiscard ? "DISCARD" :
isMoveRobber ? "ROB" :
isPlayingYearOfPlenty || isPlayingMonopoly ? "SELECT" :
isRoll ? "ROLL" :
"END"
}
</Button>
Expand Down
Loading