Feature/disallow immediate dev card #290
Conversation
👷 Deploy request for catanatron-staging pending review.Visit the deploys page to approve it
|
| ? proceedAction | ||
| : isMoveRobber | ||
| ? setIsMovingRobber | ||
| : isPlayingYearOfPlenty || isPlayingMonopoly | ||
| ? handleOpenResourceSelector | ||
| : isRoll | ||
| ? rollAction |
There was a problem hiding this comment.
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
| menuListId="trade-menu-list" | ||
| icon={<AccountBalanceIcon />} | ||
| items={tradeItems} | ||
| > | ||
| Trade | ||
| </OptionsButton> | ||
| <Button | ||
| disabled={gameState.is_initial_build_phase} | ||
| disabled={gameState.is_initial_build_phase || isRoadBuilding} |
There was a problem hiding this comment.
Still need to see ResourceSelector "Select" button for Monopoly/YOP, hide other buttons while playing dev cards
zarns
force-pushed
the
feature/disallow_immediate_dev_card
branch
from
e48f827 to
b8e9b9b
Compare
Pull Request Test Coverage Report for Build 11690556474Details
💛 - Coveralls |
There was a problem hiding this comment.
Similar to the other PR, do you mind running many games before and after this change to keep in mind performance? Doing something like catanatron-play -n 1000 is probably good enough for some rough numbers. 👍
Thanks again for this. Great contributions left and right. 💪
| 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) |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 actionsThere was a problem hiding this comment.
Great catch! Didn't realize this testing thru the UI because is_road_building prevents the UI player from doing anything else
| "KNIGHT_PURCHASED_THIS_TURN": 0, | ||
| "MONOPOLY_PURCHASED_THIS_TURN": 0, | ||
| "YEAR_OF_PLENTY_PURCHASED_THIS_TURN": 0, | ||
| "ROAD_BUILDING_PURCHASED_THIS_TURN": 0, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| menuListId="build-menu-list" | ||
| icon={<BuildIcon />} | ||
| items={buildItems} | ||
| > | ||
| Buy | ||
| </OptionsButton> | ||
| <OptionsButton | ||
| disabled={tradeItems.length === 0 || isPlayingYearOfPlenty} | ||
| disabled={tradeItems.length === 0 || isPlayingMonopoly || isPlayingYearOfPlenty || isRoadBuilding} |
There was a problem hiding this comment.
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.
|
Superseded by #292 |
closes #149
Players should be able to,
Players should not be able to,