Activate the Mechanical Marquise

[tylerhuntley]

No description provided.

[rottencowz]

Thanks Tyler! Couple minor suggestions if you have time to take a look at them. Lmk what you think. Thanks for adding this!

[rottencowz]

This will force the element to be checked right? Not sure I like this, I think the checking of the box should be done by the user.

[tylerhuntley]

Yeah, I figured it's the only way to play with one player, so might as well force it instead of generating an invalid game. I guess that's better handled via reach limit enforcement though.

[rottencowz]

The bot player object has this data stored into it. Perhaps this scope should:

1. Select a random bot player
2. get the faction from the player's field
   rather than hard code a faction

something like:

if (useBot.checked) {
    const bot = /* select a bot from DATA.PLAYERS/DATA.BOT_PLAYERS */;
    selectFaction(bot.faction);
}

[tylerhuntley]

Better yet, allow for arbitrary bot or player choices, by just picking N random factions from the pool, minus those already chosen.

[haswellr]

Sorry, my comments will involve some work and refactoring!! But I think it is important stuff so that we can easily extend upon this.

[haswellr]

For consistency, I think you should set up a listener on the checkbox and update the State when it changes, saving it locally. That way when users load the page they get saved player lists and also bot choices, instead of only saving part of their inputs. It would also clean up this function so that it doesn't access the DOM and instead focuses purely on logic, while the listener handles the view-state interface.

Better yet, make this an integer, "number of bots", then randomly pick up to that number!

[rottencowz]

Clicking randomize should not change state on whether something is selected or not. Randomization should be 'functional' based on provided state, and allowed to fail if the input state is invalid. We fail if we provide too many players (which is reasonable), and it seems clearer to fail if we try to generate for one player (which is current behavior).

I think there's a clearer way to represent this entire idea: have an "Add Bot" button at the same level as "Add Player" which could add a bot into the player list. Easy for user to add the desired number of players (0, 1, 2, 3...bots) and remove them. Bot players can be differentiated with color and a [BOT] tag. This means we can leverage the existing visual structure to communicate selected player state.

Faction selection for the bots would happen randomly from the currently available bot factions. This allows us to not have to modify this state as weirdly - randomizeFactions now just takes two arguments and doesn't need to change weird UI state.

In my opinion, this will be the cleanest way to allow users to add bots as it will also allow users to customize bots counts when playing with other players too.

[rottencowz]

I'm fine with reworking the box into "add bot player" button in a separate PR that leverages what this uses, but I would love to hear your opinions. @tylerhuntley @haswellr

e: I do feel strongly about not changing state here though was looking at old code - still getting used to github

[tylerhuntley]

Yeah I was leaning toward the "add bot player" option, with perhaps a dropdown to pick factions, that just requires reworking the state and makes the players/contenders even more redundant. Agreed Randomize shouldn't change state itself though, that was just me cutting corners.