Rationalize notions of Seats, Active players

Realized while working on #752 #753 #754 .

The core game logic layer thinks that the number of players is set once at game start and never changes, and everyone is present all of the time.

But the server logic knows that actually some player "seats" are empty at any given time.

Some game seats you want to be able to play the game, leaving the "seat" open for someone to join in the future (but not necessarily at any time). Some seats need to be filled before the game can start.

In the same way #754 covers SetUp, we should rationalize when new players are allowed to join.

The custom game logic might at any time want to reason about number of seats (what the core game logic calls "numplayers" right now), "active players" (people who are actively playing right now), and "inactive players" (people sitting in a seat but not yet active; for example, they joined mid-round and won't become active until the next round starts).

PlayerIndex.Next() needs to be able to know which player seats are active.

You'd have a GameDelegate.PhaseAllowsPlayerJoin(phase int) bool or something.

Whether a player is "empty, active, or inactive" could be done as a behavior.PlayerState or something. Then there could be moves that take an inactive player and flip them to active. These could be fixup moves, that run in an "Activate Inactive Players" phase, to automatically run. Maybe the GameDelegate doensn't even need to know when those phases are; it's literally just "is it legal to run that fixup move right now?"

Games that don't have PlayerState or PlayerEmptyBehavior signal to the server: people can join whenver, and all seats should at some point be active.

There's a notion of Seats and Players. What the core engine has typically called Players is actually more akin to a notion of Seats--a place where a Player could go. A player is an identified thing, tied to an account, sitting in a seat.

The core engine things Seats has a 1:1 to players. They're always occupied by a player, and that player is always active. That's reasonable as a base case, but not in general.

A seat might be empty because no player has yet joined the game to sit in that seat yet, but we hope they do come. Or it might be empty because we don't want new players anymore because the game is in progress.

And a player sitting in a seat might not be active because they might have sat down in a seat in the middle of a round, and have to wait until the next round starts to be "dealt in."

The core game doesn't know anything about seats and players, but servers do. Custom game logic and the server logic need to signal to each other a few things:

Which seats are empty?
When does a seat become non-empty?
When is it legal to seat a player?
Which players are seated but not yet actively playing?

The way it does this is by a mix of player state behaviors, move types that game logic can include to seat players and activate them, and logic on the base.GameDelegate that helps count the players.

By default, the custom game logic has no idea if a seat is filled or not. The server seats players as soon as they come to the table, if there's still an empty seat. If the game logic gets to the point where a specific seat needs to make a move, and no one's sitting in it, then the core game logic just sits forever. Players can see in the UI that there's no one there to make a move, but the actual game logic can't do that. These are the most naive game types (on this dimension, at least). Simple games, and games with one round, are often fine with this.

If games want to know which seats are empty, they can embed behaviors.SeatEmptyBehavior. It adds an SeatFilled bool field, and a SetSeatFilled() method. The moves.SeatPlayer move is how the server will tell the game a player has been seated. It's not a FixUp move (it shouldn't be applied in general). It's a move that it's only legal for an admin to propose. The server will propose that move if there is a player waiting to be seated. When it is applied, it calls the SetSeatFilled() on the seat that was now occupied. This is how the game can tell that it was applied. By default you should have that move be legal in any phase. If you want waiting players to be seated only at specific times, you should make that SeatPlayer move be legal only when you want it to be legal, for example in the SetUp phase for a round.

When a user loads a game and is not yet seated, they see a message in the UI saying they're waiting to be seated.

Sometimes the game logic wants to communicate that empty seats should stay empty. That's why the SeatEmptyBehavior also has a SeatClosed bool, and SetSetClosed(). When someone is seated withSeatPlayermove, it sets both of those to true. But game logic can also call it when they want to signal that those empty seats aren't open to be filled anymore. There's another convenience FixUp move calledCloseEmptySeats`, which when run will set any empty seat to closed. You'd typically run this, for example, at he end of the InitialSetup phase, if you wanted to not allow any other players.

With this empty/closed logic, though, the game logic doesn't know if there's someone waiting to be seated, which might conceivably be useful for the game logic to know. That's why there's a notion of a player.Active: behaviors.PlayerInactive, which has a Inactive bool field, IsActive() bool, SetPlayerActive(), SetPlayerInactive(). If you don't include that behavior, then the server logic assumes that all players are active all the time.

An Inactive player is one who is seated, but is not included in the rules of this round. (Typically; technically it's up to the game logic to decide what it means for them). If you have this behavior included, then the SeatPlayer move will seat the player, but also set them to be Inactive. There's another move, ActivateInactivePlayer that is a FixUp move that will run and activate all inactive players. If you want the server to seat players at any time, and want them to immediately be active, then you would add two fixup moves to always be legal at any time: SeatPlayer and ActivateInactivePlayer, so the player would be seated, and then immediately the ActivateInactivePlayer would see that there's a person to activate and do that.

By default, when game logic gets to a given seat's turn and there's no one sitting in it, the game will just wait for it to be occupied. Even when you have player.InactiveBehavior, by default it's not set to true, so logic will stall to wait for the player to come. This is the safest default. But in some cases you want to proceed and not wait for the seat to be filled. There's a FixUp move called MarkEmptySeatsInactive that when run will take any seats that are currently empty and mark as inactive. Inactive players won't be selected as NextPlayer; everything will skip over them as though they don't exist. So games where you don't want to wait for every seat to be filled can have that FixUp move apply at the end of the RoundSetUp phase (for example) so the round doesn't stall waiting.

base.GameDelegate has a NumActivePlayers(), and NumFilledSeats(), which know about the two seat/player behaviors for its count. Typically your game logic will use NumActivePlayers() for any core game logic.

PlayerIndex.Next() and Prev() consult GameDelegate.SeatMayBeActive(index int) bool. base.GameDelegate looks for the Inactivated player behaviors and does the right thing, which is how PlayerIndex.Next() continues working.

Other things to figure out:

Can seats be sat in by different players at different times? Or is "taking a seat" a one time action for a seat?

Make sure that the pattern of "game with lots of seats, few players, is easy to play in admin mode for debug purposes" continues to work.

[x] Core engine NumPlayers --> NumSeats throughout (update all documentation)
[x] Core GameDelegate grows PlayerMayBeActivated() bool, which PlayerIndex.Next() and Prev() use. default.GameDelegate simply returns true for now
[x] Create behaviors.InactivePlayer. When Player is inactive, base.GameDelegate should return false for that player
[x] Create moves.ActivateInactivePlayer
[x] Create behaviors.SeatFilledBehavior (with both SeatFilled and SeatClosed) fields
[x] Create moves.SeatPlayer
[x] moves.SeatPlayer has IsSeatPlayerMove() bool so people can subclass it
[x] Create moves.CloseEmptySeats
[x] Create moves.InactivateEmptySeats
[ ] Create moves.ActivateEmptySeats
[x] Create a PlayerIsInactive method in behaviors that takes an immutablesubstate and does the conversion for you, and use that more places so you don't have to cast it
[x] Does logic to activate other inactive seats use player.Next() (which would skip inactive players?) i don't think so...
[x] Verify that other moves that do DefaultsForState to pick next player will skip inactive players
[x] Put documentation for Seat and Inactive in the package doc for behaviors. And everything else (moves, interfaces, etc) that references them should summarize what it does succinctly, but then point to that documentation for more about the underlying concepts.
[x] base.GameDelegate grows NumActiveSeatedPlayers, NumActivePlayers, NumSeatedPlayers (and documentation note on GameDelegate.NumPlayers to say not to use that number for game logic but instead to use NumActiveSeatedPlayers)
[ ] A player move to CloseAllSeats or InactivateAllSeats. You can imagine it being allowed for player 0 (or in the future whoever the engine tells us is the game creator) to say "screw it let's get started"
[x] Game grows a CheckFixUpMoves, which is exposed via ManagerInternals, and if it's called, if a move isn't being applied, then it checks if any fix up moves apply and if one does, it proposes it to propseMove right now. Need to verify that there isn't a race with the main loop--maybe have it be processed by mainLoop in a multiplexed proposeMove channel?
[x] StorageManager grows a FetchGameSpecialData(game, string) interface{}. ServerStorageManager overrids it and provides a github.com/jkomoros/boardgame/server/api.PlayerToSeat type, which SeatPlayer looks for and must be true for it to be legal.
[x] When a server boots up it checks which gamemanagers have a SeatPlayer move type in them
[x] apiGameSetup should get the seatsClosed and pass to calcEmptySeats
[x] SeatPlayerSignaler grows SeatIndex(), which is used in DefaultsForState if it returns a valid seat index (so that DefaultsForState and the seat that the server intends to seat the player in are aligned)
[x] As far as the server is concerned, once a player goes into the pendingSeating queue, they ARE seated in that seat, it might just not be effected yet. (DefaultsForState actually picks the seat right now, which could technically be out of sync). Commited() doesn't change anything in the storage layer; it just says "stop returning this playerToSeat for FetchInjectedContent"
[x] When a player is ready to be seated, we set api.PlayerToSeat to be a SeatPlayerSignaler with a committed callback that will remove it from the game queue, and then also tell manager to CheckFixUpMoves.
[ ] Consider optimizing it so that once a seat is closed we never check again, since currently a seat can only be closed, never un-closed. (But maybe that will change in the future, so not worth optimizing)
[x] Make the thing in PlayerToSeat be an object following an interface of Player(), and Committed(), and have SeatPlayer install the callback
[x] server needs some way to get a callback that a player was seated successfully. Maybe just as soon as it notes that the game has one fewer slots? ... But you want SeatPlayer flag to be lowered as soon as the player is seated, otherwise it could just run again and again. (Well it wouldn't right now because it's FixUp, not FixUpMulti, but it should be able to). And you can't just twiddle something in SeatPlayer.Apply() because the move could fail to apply after that.
[x] Wait, if you have the SeatPlayer move only legal at the end of a phase, between two runs of fixup moves, you need a way for the server to get in edgewise between other fixup moves, otherwise all of the fixups will run straight through. That implies the core game logic has a InjectedMove queue where the order is: at every stage if there's any moves in InjectedMove queue, see if one is legal. If not, continue to fixup moves. Allow a way to cancel an injection. Should there only be one, or a whole queue?
[x] moves.InactivateEmptySeats should only be legal if a configurable number of players (by default, MinNumPlayers) are seated (or should there be a move.WaitForEnoughPlayers ?)
[x] triggerFixUp() bails if the game is not modifiable. But it shoudl do what ProposeMove does and dispatch to the real version of the game instead of bailing quietly.
[x] Server's internal playerToSeat is not a valid instantiation of the itnerface
[x] The SeatPlayer fails to be saved because CurrentPlayerIndex is not valid because there are no players. (Make it so that the index can't be invalid if there are no players it could be)
[x] Remove the panic in mainLoop for fixUpTriggered. Have the same kind of DelayedError for the internals object
[x] PlayerIndex documentation should warn to never do arithmetic
[x] PlayerIndex shoud get a ValidOrNext() that will return an index that leave it the same or advance to the next one if it's not valid. And then use it canonically in accesses.
[x] use EnsureValid() canonically in moves
[x] Next and Prev shouldn't panic if nothing's valid, but just leave in the same place.
[x] (Another approach is to have when the state is inflated or created, do ValidOrNext on each player index. This has a couple of problems: 1) PlayerMayBeActive could depend on PlayerIndex properties, and if there are multiple PlayerIndex properties there could be weird path dependency. 2) PlayerIndexes that aren't even being inspected might be incremented,and then saved, when in reality you only want them to beincremented if they're being read)
[x] PlayerIndex should get WithinBounds() bool that just checks if the number if ObserverPlayerIndex..len(numPlayers)-1. That is what should be checked for valid on state save. Valid() check calls withinBounds and also checks if PlayerIsActive.
[x] Have a struct inflater tag that denotes that the field should be auto-validated on save. (No, you still don't want that, because it might be temporary and over by the time you need to read it back. Maybe it can be a move that goes through and sets all fields to ValidOrNext? ... And shouldn't games that have behaviors.InactivePlayer and behaviors.Seat auto-set all seats to inactive as the very first move?
[x] the golden storage manager should scan the game type it's created for and if there are no seat player moves in the golden record, then report back that the game won't use SeatPlayer, so the actual game logic won't wait for it
[x] Inactive client renderers and player rosters should render as disabled
[x] GameDelegate should get a ReversePlayerOrder(state ImmutableState) int that reverses the order Next/Prev go in
[x] SeatPlayer when finished doesn't actually send new states to client
[x] It now says its setting the player to seated... but the state never actually changes to have the player be marked as seated.
[x] Make it so that golden's storage manager is also able to seatPlayer. Its should inject a player when the game is stalled and when the next move in the golden is a type that appears to be IsSeatPlayer.
[x] Blackjack should use moves.InactivateEmptySeats (will require new goldens, or surgery on the goldens)
[x] Blackjack should use moves.WaitForEnoughPlayers
[x] A way for special players to say "It's OK to start". Maybe have it just be that WaitForEnoughPlayers checks, if Admin then it checks at least TargetCount, but if it's a player it lets them propose it before then? Or I guess you always need at least TargetCount to be legal to go, so maybe WaitForEnoughPlayers has a AllowPlayer() bool and if true then it only is legal if a player does it. (And update the logic of wilSeatPlayer in Legal, which currently just returns nil immediately)
[ ] Blackjack should support multiple rounds
[x] WaitForEnoughPlayers has to know to NOT wait for seated players in contexts where players can't be seated (e.g. when not run in a server context. Of course, if a golden was recorded in a server context it needs to know to wait for it, since it WILL come. Maybe the move reaches in and checks via FetchInjectedData for ContextWillSendSeatPlayer
[x] Make sure Blackjack actually seats players as expected
[ ] Verify that if you do surgery on a game and put in a new move, that the game actually runs. (the logic in AdvanceToNextInitiatorMove needs to keep a diff of moves in record versus golden correspondence)
[x] Update package doc for golden remastering with any new best practices (e.g. "only need to update the Initator of the immediately after one")
[x] Add the seat idioms to the stub tutorial output (and index.EnsureValid())
[x] Create a pre-created moves.Serial() that's the activateinactive players, wait for enough players, inactiveemptyseats in one easy to include group since it's so common as a kind of weird idiom to do at the start of a round.
[x] Need a way to test injectable moves in golden. If the moves are wedged, before giving up, see if the next move in the golden, when fetched from manager, has a IsSeatMove(). If so, inject via the backdoor the targetplayerindex
[x] The server uses the new behavior to figure out when to seat players, if the game has a SeatPlayer move at any point (in the hook that we use after a run of moves has been applied, or when a player tries to join).
[ ] Server should stop trying to fill seats if there are no more seats that are open.
[x] Client should show a message to player when seating them is pending
[ ] Use new moves and behaviors idiomatically in all examples and games
[x] Make sure the TUTORIAL is fully updated about seats vs players.
[ ] Figure out some way to handle if SeatPlayer isn't included in the game logic, but SeatFilled is. An error? have everyone wait in a lobby before it starts? This might be easier once #752 is done, because there might be some kind of group selection / lobby UI to use

Actually, changing Core engine's NumPlayers --> NumSeats is a HUGE change, and is actually arguably more confusing throughout. For example, playerState is still a thing, not SeatState. Best to just leave it, and have people who handle "SeatPlayer" logic puzzle through it.

How to inject the SeatPlayer move? The approach was going to be a game.InjectMove that runs the move ASAP as soon as it's legal to run. But that logic gets REAL funky, where the SeatPlayer move could be in a FixUp chain that it's not really part of. And the InjectMove logic gets REALLY wonky and weird.

Another approach is to have SeatPlayer just be a fixUp move, that reaches into a special stash space for players to seat in a rendevoous point in the storage manager. StorageManager would have FetchCustomDataForGame(game, datatype string) interface{}, and then ServerStorageManager would override it and look for github.com/jkomoros/boardgame/server/api.PlayerToSeat and fetch the next player record to seat.

That latter approach is better because it doesn't require a weird InjectMove and timing weirdness. But it does require one oddity: a CheckFixUpMoves() that can run even when a player move isn't applied. Without this exception, fix up moves can only be newly legal if the state changes, an the state can only change if a move is applied. That's... a tiny bit weird.

There's a new bug to work through. When the first player is automatically seated, because there's behaviors.Seat AND behvaiors.InactivePlayer, the player is inactivated. But that means that any PlayerIndex properties that were defaulting to 0 are now (temporarily) invalidated. This means not just CurrentPlayer, but also things like RRStartPlayer and others.

I had thought the solution was to special case allow any playerIndex when none can be valid, but in that case only the very first user is invalid, the others are still valid.

In a perfect world, if you didn't care about the current value of a player selector, you'd set it to AdminPlayerIndex. But there's no obvious place to set it to the non-zero-value, and in many cases you do want it to default to a reasonable default.

The underlying problem is that it used to be you could assume 0..numPlayers were all valid indexes, and now they can magically become invalid through spooky other action at any moment, invalidating things that used to be valid.

Why do we check for validity of playerIndexes at save? Mainly to ensure that you didn't make the mistake of just doing playerIndex++ to get to the next player in your logic, which would be confusing to find later, and we want to error early to make it easier to catch the bug in your userland logic.

One option is to stop checking whether it's valid when you save, and insted have a ReadValid() method on playerIndex, that things can use to verify the value they're reading is valid, returning the current value if the current value is allowed to be active, and calling Next() to get to the next valid value otherwise.

Note: we currently panic in Next() if no index is currently valid, but that could totally happen. In that case we should just leave it at the one it started at.

Another option: make PlayerIndex an opaque type, that's actually a struct. When serialized to/from JSON it would pretend to be just an int, but in the actual live structs it would be a struct that has a pointer to the (possibly nil) state it's in. That would disallow arithmetic on it naturally, and also let things like Next / Prev operate in place. We use playerIndex for a LOT of things in other packages, how much would need to change?

Other interesting things: AdminPlayerIndex and ObserverPlayerIndex would become pointers (that serialize to -1 and -2 in JSON)

Captured this in #763

With SeatPlayer, goldens need to be tweaked. The server context can/will SeatPlayers. But other contexts never will, so things that wait for it (like moves.WaitForEnoughPlayers) shouldn't even bother waiting.

In some cases you'll have a golden recorded from an environment that didn't include SeatPlayers. In others you'll have them recorded where SeatPlayers was called by the context.

Game logic should be valid when run in different contexts (e.g. server context, but also other ones).

This issue is almost entirely done, except for a few random TODOs here and there, leaving open just for that

jkomoros / boardgame

Rationalize notions of Seats, Active players #755