Sinadmode #713

22Chaos · 2024-10-21T19:30:46Z

This pull-request introduces Sinad mode, a toggle-able setting in the menu that modifies the mission sizes of 6 player Avalon. If mission 1 and 2 both succeed, and mission 3 fails, and if mission 3 was not a superset of m2, (i.e. m3!=m2+1)
The mission size for Mission 4 becomes a 4-man team, and the final mission 5 becomes a 3-man team.

The author hopes that this mode will make 6p games more interesting.

vck3000 · 2024-10-22T12:22:58Z

src/gameplay/game.ts

-  [3, 4, 4, 5, 5],
-  [3, 4, 4, 5, 5],
-  [3, 4, 4, 5, 5],
-];


Could you keep this here but instead conditionally use the sinad versions in the pickingTeam phase?

Hmm, alternatively, if you want to have the GUI update live, you can directly mutate this array when you need to. Const arrays can still be modified.

What about keeping two global arrays, and having a reference to one of the two arrays as a field of the Game class? updateMissionSizesForSinad() can then update that reference to point to the other array.

Hmmm, actually, something I didn't think about was that this const object is shared across all games, so modifying it directly will cause issues. We'll need to either do what you did and move it onto the Game object itself as a member, or we'll have to make a static function that is given sinadMode and returns one or the other.

I might slightly prefer your current solution. Let me think a bit :)

vck3000 · 2024-10-22T12:33:38Z

src/gameplay/game.ts

@@ -566,6 +570,9 @@ class Game extends Room {
    if (this.disableVoteHistory) {
      this.sendText('The game has vote history disabled.', 'gameplay-text');
    }
+    if (this.enableSinadMode) {
+      this.sendText('The game has Sinad Mode enabled.', 'gameplay-text');
+    }


Can you also check that enableSinadMode can only be true if it's 6P here?
https://github.com/vck3000/ProAvalon/blob/master/src/gameplay/game.ts#L362-L378

This way if the host misconfigures the game, the server will tell them when they try to start the game.

My current solution to this.

I wanted to make this check inside static checkOptions(options: string[]), but I wasn't sure because it looks like the options array only has Role enum members, and I think startGame(options: string[]) relies on this. Alternatively, I could try making checkOptions non-static to access this.enableSinadMode directly, but looks tricky.

I see. Feel free to make that non-static so you can put it together.

The design of Room and Game is pretty poor. Ideally these kinds of checks are done within the Game.

vck3000 · 2024-10-22T12:38:48Z

src/gameplay/phases/common/votingMission.ts

@@ -270,6 +275,61 @@ class VotingMission implements IPhase {
  getProhibitedIndexesToPick(indexOfPlayer: number): number[] {
    return [];
  }
+
+  updateMissionSizesForSinad(): void {


The check for Sinad can actually be done as a separate system, but it's not currently a thing.

https://github.com/vck3000/ProAvalon/blob/master/src/gameplay/game.ts#L758-L809

See this section^. Essentially, every role and card gets an opportunity to move the game into a different phase/state after each phase has acted in this function call: checkRoleCardSpecialMoves().

We can generalise this behavior to non-role and non-card specific actions such as a game setting like Sinad. If you're interested we can pursue this. Alternatively, happy for you to leave this implementation here for now.

vck3000 · 2024-10-22T12:40:28Z

src/gameplay/phases/common/votingMission.ts

+      this.thisRoom.missionHistory[1] === 'succeeded' &&
+      this.thisRoom.missionHistory[2] === 'failed' &&
+      this.thisRoom.missionNum === 4 &&
+      this.thisRoom.pickNum === 1


We should have a separate boolean tracking whether this has activated. This will help us avoid duplicate runs into this function. Although this code is only reached when a mission has voted, it'd be simpler and safer to just check for m1 success, m2 success, m3 fail, and then sinad hasn't activated yet to enter this function.

Added a new method and field to track this. Hope this works.

vck3000 · 2024-10-22T13:12:12Z

src/gameplay/phases/common/votingMission.ts

+      for (const player in VH) {
+        if (VH.hasOwnProperty(player)) {
+
+          const playerVH = VH[player];
+          const playerM2 = playerVH[1][playerVH[1].length -1];
+          const playerM3 = playerVH[2][playerVH[2].length -1];
+
+          if (typeof playerM2 === 'string' && playerM2.includes("VHpicked")) {
+            setOfPlayersOnM2.add(player);
+          }
+          if (typeof playerM3 === 'string' && playerM3.includes("VHpicked")) {
+            setOfPlayersOnM3.add(player);
+          }
+        }
+      }
+
+      let isM2ASubsetOfM3: boolean = [...setOfPlayersOnM2].every(player => setOfPlayersOnM3.has(player));


Let's pull this code out to another file. Let's also add some tests.

E.g. I quickly spun this up. Should be good reference :).
#714

Note I cheated a bit and used a node v22 feature (Set.difference()). Please find another solution for this, like the one you have here.

Wrote function isSubsetOf(setA, setB) in game.ts. Test case passes.

…need to code sinad mode logic.

- Split above func into getPlayersOnMission and subsetOf. - Room now prevents starting sinad mode without 6p. - Test cases are WIP. - rename NUMS_PLAYER_ON_MISSION to camelCase

- bug fix in isSubsetOf func - add more sinad testcases for new funcs.

- refactor sinad test cases - more isSubsetOf test cases - cleanup comments - better formatting - BUILD ERROR. DO NOT MERGE.

22Chaos · 2024-10-29T03:13:51Z

src/gameplay/tests/sinad.test.ts

+      game.voteHistory = vh; 
+      game.enableSinadMode = true;
+      game.playersInGame = ['Piplup', 'Torchic', 'Mudkip', 'Turtwig', 'MewTwo', 'Charmander'];
+      game.gameMode = GameMode.AVALON;
+      game.missionHistory = ['succeeded','succeeded','failed'];


Build error:

Property 'voteHistory' does not exist on type 'Game'. Property 'enableSinadMode' does not exist on type 'Game'.

Not sure why this would happen, since the other fields (missionHistory, playersInGame) did not throw this error.
What's weirder is, $ yarn test works perfectly fine with this. For instance if you change game.enableSinadMode to false here, then yarn test will throw an error.

vck3000 · 2024-11-20T04:35:33Z

src/gameplay/game.ts

    this.roomCreationType = gameConfig.roomCreationType;
    this.getTimeFunc = gameConfig.getTimeFunc;
-


Can you add this back in

vck3000 · 2024-11-20T04:38:39Z

src/gameplay/game.ts

+
+  getPlayersOnMission(missionNum: number): Set<string> {      
+    const set = new Set<string>();
+    const VH = this.voteHistory; //brevity


Suggested change

const VH = this.voteHistory; //brevity

const vh = this.voteHistory;

vck3000 · 2024-11-20T04:39:01Z

src/gameplay/game.ts

@@ -2283,8 +2300,73 @@ class Game extends Room {
    }
    return newRating;
  }
+
+  getPlayersOnMission(missionNum: number): Set<string> {      
+    const set = new Set<string>();


Suggested change

const set = new Set<string>();

const playersOnMission = new Set<string>();

vck3000 · 2024-11-20T04:39:24Z

src/gameplay/game.ts

+    const VH = this.voteHistory; //brevity
+
+    if(VH === undefined) {
+      return set; // empty set.


Don't need comment

vck3000 · 2024-11-20T04:39:48Z

src/gameplay/game.ts

+    }
+
+    for (const player in VH) {
+      //TODO: write check to ensure player really is in this.PlayersInGame.


It's ok, we can assume that the players in VH are actual players in the game.

Can remove this comment.

vck3000 · 2024-11-20T04:40:03Z

src/gameplay/game.ts

+
+    for (const player in VH) {
+      //TODO: write check to ensure player really is in this.PlayersInGame.
+      if (VH.hasOwnProperty(player)) {


Did you need this if statement?

vck3000 · 2024-11-20T04:40:57Z

src/gameplay/game.ts

+      //TODO: write check to ensure player really is in this.PlayersInGame.
+      if (VH.hasOwnProperty(player)) {
+        if(VH[player].length < missionNum) { 
+          //in case mission hasn't happened yet.


Suggested change

//in case mission hasn't happened yet.

// If the mission hasn't happened yet.

vck3000 · 2024-11-20T04:41:17Z

src/gameplay/game.ts

+          return set;
+        }
+
+        const missionNumVH = VH[player][missionNum - 1];


Suggested change

const missionNumVH = VH[player][missionNum - 1];

const missionVh = VH[player][missionNum - 1];

vck3000 · 2024-11-20T04:42:00Z

src/gameplay/game.ts

@@ -2283,8 +2300,73 @@ class Game extends Room {
    }
    return newRating;
  }
+
+  getPlayersOnMission(missionNum: number): Set<string> {      


Suggested change

getPlayersOnMission(missionNum: number): Set<string> {

getPlayersWhoWentOnMission(missionNum: number): Set<string> {

For some extra little bit of clarity

vck3000 · 2024-11-20T04:45:20Z

src/gameplay/game.ts

+         this.getPlayersOnMission(2)
+        ,this.getPlayersOnMission(3)
+      )) {
+        this.numPlayersOnMission[6 - MIN_PLAYERS] = [2,3,4,4,3];


Can you run prettier to format the file pls?

https://prettier.io/docs/en/cli.html

It should be installed when you run yarn, but if not, do yarn prettier . --write

vck3000 · 2024-11-20T04:45:33Z

src/gameplay/game.ts

+    console.log(this.getPlayersOnMission(2)) ;
+    console.log(this.getPlayersOnMission(3)) ;
+    console.log(!isSubsetOf(this.getPlayersOnMission(2),this.getPlayersOnMission(3)));


Please clear these guys out when you're done debugging.

vck3000 · 2024-11-20T04:46:56Z

src/gameplay/phases/common/votingMission.ts

+        if(!this.thisRoom.hasSinadRun && this.thisRoom.shouldSinadRun()) {
+          this.thisRoom.updateMissionSizesSinad();


Can you rename this to this.thisRoom.checkSinad(), and then move the hasSinadRun check, and shouldSinadRun check into checkSinad()?

That way it's a single entrypoint, making it easier for clients to use.

Then you can remove this.thisRoom.hasSinadRun = true; below as well

vck3000 · 2024-11-20T04:47:23Z

src/gameplay/game.ts

+    this.hasSinadRun = true;
+  }
+
+  hasSinadRun: boolean = false;


Can you move this to the top of this class with the other members

vck3000 · 2024-11-20T04:48:25Z

src/gameplay/game.ts

@@ -2310,6 +2392,20 @@ function generateAssignmentOrders(num) {
  return rolesAssignment;
 }

+export function isSubsetOf(setA, setB): boolean {


Can you provide the types here for setA and setB please

vck3000 · 2024-11-20T04:51:10Z

src/gameplay/game.ts

@@ -2283,8 +2300,73 @@ class Game extends Room {
    }
    return newRating;
  }
+
+  getPlayersOnMission(missionNum: number): Set<string> {      


Can you make this method private please. Outside code shouldn't call into this.

vck3000 · 2024-11-20T04:51:45Z

src/gameplay/tests/sinad.test.ts

+      expect(mockGame.shouldSinadRun()).toEqual(false);
+  });
+
+  it('should not run if m1 failed', () => {


Can you move these tests to the existing game.test.ts?

That way you don't have to use mocks and you can get better test coverage.

vck3000 · 2024-11-20T04:52:50Z

src/gameplay/tests/sinad.test.ts

+  it('should pass these Sets declared the old fashioned way', () => {
+    const setA = new Set<string>(['a', 'b', 'c']);
+    const setB = new Set<string>(['a', 'd', 'c','b']);
+    expect(isSubsetOf(setA,setB)).toEqual(true);
+    const setC = new Set<string>(['a','e','d','c']);
+    expect(isSubsetOf(setA,setC)).toEqual(false);
+    expect(isSubsetOf(undefined,setC)).toEqual(false);
+    expect(isSubsetOf(setA,undefined)).toEqual(false);
+    expect(isSubsetOf(new Set<String>(),undefined)).toEqual(false);
+    expect(isSubsetOf(undefined,new Set<String>())).toEqual(false);
+    expect(isSubsetOf(undefined, undefined)).toEqual(false);
+
+  });


We should test these through the Game's API (i.e. at a higher level) in game.test.ts.

If you want to test isSubsetOf() specifically like you've done here, please split that function to a separate file, and move these tests to just isSubsetOf.test.ts or similar.

vck3000 · 2024-11-20T04:53:21Z

src/sockets/sockets.ts

@@ -1933,6 +1939,14 @@ function updateRoomDisableVoteHistory(disableVoteHistory) {
  }
 }

+function updateRoomEnableSinadMode(enableSinadMode) {


Suggested change

function updateRoomEnableSinadMode(enableSinadMode) {

function updateRoomEnableSinadMode(enableSinadMode: boolean) {

vck3000 · 2024-11-20T04:53:55Z

src/models/gameRecord.js

@@ -41,6 +41,7 @@ const gameRecordSchema = new mongoose.Schema({
  numFailsHistory: [Number],
  voteHistory: Object,
  disableVoteHistory: Boolean,
+  enableSinadMode: Boolean,


Did you add this to the game finish method in game.ts?

vck3000 reviewed Oct 22, 2024

View reviewed changes

22Chaos added 5 commits October 22, 2024 19:25

added sinadMode option in menu. fully functional button, now we just …

c24eb1f

…need to code sinad mode logic.

Added Sinad mode

3a10b1c

Refactor sinad func and add sinad mode testcases.

09eece6

- Split above func into getPlayersOnMission and subsetOf. - Room now prevents starting sinad mode without 6p. - Test cases are WIP. - rename NUMS_PLAYER_ON_MISSION to camelCase

add a boolean that tracks whether sinad ran.

4e3eb81

- bug fix in isSubsetOf func - add more sinad testcases for new funcs.

move sinad functions inside Game

af4781a

- refactor sinad test cases - more isSubsetOf test cases - cleanup comments - better formatting - BUILD ERROR. DO NOT MERGE.

22Chaos force-pushed the sinadmode branch from 76bcd18 to af4781a Compare October 28, 2024 19:07

22Chaos commented Oct 29, 2024

View reviewed changes

vck3000 reviewed Nov 20, 2024

View reviewed changes

src/gameplay/game.ts

const VH = this.voteHistory; //brevity

if(VH === undefined) {

return set; // empty set.

Copy link

Owner

vck3000 Nov 20, 2024

Choose a reason for hiding this comment

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

Don't need comment

vck3000 reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sinadmode #713

Sinadmode #713

22Chaos commented Oct 21, 2024

vck3000 Oct 22, 2024

vck3000 Oct 22, 2024

22Chaos Oct 22, 2024

vck3000 Oct 22, 2024

vck3000 Oct 22, 2024

22Chaos Oct 29, 2024

vck3000 Nov 20, 2024

vck3000 Oct 22, 2024

vck3000 Oct 22, 2024

22Chaos Oct 29, 2024

vck3000 Oct 22, 2024

22Chaos Oct 29, 2024

22Chaos Oct 29, 2024 •

edited

Loading

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

vck3000 Nov 20, 2024

		this.roomCreationType = gameConfig.roomCreationType;
		this.getTimeFunc = gameConfig.getTimeFunc;

	const VH = this.voteHistory; //brevity
	const vh = this.voteHistory;

	const set = new Set<string>();
	const playersOnMission = new Set<string>();

	//in case mission hasn't happened yet.
	// If the mission hasn't happened yet.

	const missionNumVH = VH[player][missionNum - 1];
	const missionVh = VH[player][missionNum - 1];

	getPlayersOnMission(missionNum: number): Set<string> {
	getPlayersWhoWentOnMission(missionNum: number): Set<string> {

		if(!this.thisRoom.hasSinadRun && this.thisRoom.shouldSinadRun()) {
		this.thisRoom.updateMissionSizesSinad();

	function updateRoomEnableSinadMode(enableSinadMode) {
	function updateRoomEnableSinadMode(enableSinadMode: boolean) {

Sinadmode #713

Are you sure you want to change the base?

Sinadmode #713

Conversation

22Chaos commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

22Chaos Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

22Chaos Oct 29, 2024 •

edited

Loading