Add new boards to Baduk variant #255

merowin · 2024-05-09T11:17:17Z

Add Benjitos Graph class (from another branch)
Add a wrapper for this class -> uses above graph class internally, but adds conversions between (number <-> coordinate) such that this wrapper implements Fillable<CoordinateLike, Color>, which we then work with in Baduk variant.
Add function to Board factory with this return type.
Refactor variant Baduk s.t. it works with the new boards, by using the fillable interface.
Refactor variants that inherit Baduk:
- Drift, Keima, Pyramid can't work with arbitrary boards
  1. Validate in constructor that we work with a grid board config
  2. Use type assertion and work with Grid as before
- Refactor other variants s.t. they work with the fillable interface

Still needs to be done:

adapt the board components in vue-client
add config input forms for the variants that now can work with arbitrary boards
test all kinds of stuff!
improve the config typing: currently config may not have a board property (-> handled as grid config for backwards compatibility), and if it has a board property, then the outer properties width, height are ignored currently

benjaminpjones · 2024-05-09T12:52:06Z

packages/shared/src/variants/baduk.ts

  }

  override exportState(): BadukState {
    return {
-      board: this.board.to2DArray(),


I'd really like to keep the exports "JSON-compatible". What I mean by that is information should not be lost by converting to JSON:

let grid: Grid<number> = ...; let arr: number[][] = ...; grid = JSON.parse(JSON.stringify(grid)); // not actually a Grid arr = JSON.parse(JSON.stringify(arr)); // a 2D Array, just like the original

Motivation: We need to keep in mind that game state needs to travel over the wire at some point (#48, #174), and we need a way to losslessly encode the state.

In my mind, the way to do that is to return simple types from exportState() with helper methods like Grid.prototype.to2DArray(), then use the JSON utils to encode and decode. (If you need a Graph-compatible way to do this, feel free to alias that method to Grid.prototype.serialize()).

And let me know if you'd propose another way to pass state over the wire that doesn't simplification inside exportState!

benjaminpjones · 2024-05-09T13:10:28Z

This is so great by the way, I'm looking forward to playing some more graph Go! And I really appreciate the effort you put into de-duplicating implementations - this will help maintainability immensely.

benjaminpjones · 2024-05-09T13:25:50Z

One thought to keep this PR small and atomic - if you introduce a GridBaduk adapter, you won't need to change all the downstream stuff (vue boards, config forms, etc) at the same time. Something like (I think the types work.. ts playground):

// Ideally, this has exactly the same interface as the "old" Baduk, so it can be used anywhere
// the old Baduk was
class GridBaduk extends Baduk {
    // ! is not typesafe, but we know board is "definitely assigned" in super()
    board!: Grid<Color>;
    constructor(config: BadukConfig) {
        if (!isGridConfig(config)) {
            throw("GridBaduk only accepts grid configs!");
        }
        super(config);
    }
}

This will also remove the need to assert the type of board at the access-site in variants like Keima and Pyramid (this.board as Grid<Color>)

JonKo314 · 2024-05-09T14:56:00Z

packages/shared/src/lib/graph.ts

+  return a.map(copyArray);
+}
+
+export class GraphWrapper<T> implements Fillable<CoordinateLike, T> {


I'm envisioning a version without GraphWrapper at some point. I've thought about passing decodeMove(move: string): TKey to a Baduk<TKey> class that has a board: Fillable<TKey, Color> via the constructor.

But it's not important.

What do you think about introducing a new interface that extends fillable? It could be our board interface and have methods like serialize, reduce etc. that maybe don't fit fillable, but make sense for our board interface?
Then this interface could also handle the encoding / decoding of moves.

JonKo314 · 2024-05-09T14:57:04Z

packages/shared/src/variants/baduk.ts

 }

 export type BadukMove = { 0: string } | { 1: string };

+export declare type BadukBoardType<TColor> = Fillable<CoordinateLike, TColor>;


Maybe we could just call this BadukBoard instead of BadukBoardType?

renamed to BadukBoard

JonKo314 · 2024-05-09T14:59:19Z

packages/shared/src/variants/baduk.ts

+    let black_points = 0;
+    board.forEach(
+      (intersection) => (black_points += intersection === Color.BLACK ? 1 : 0),
+    );
+
+    let white_points = 0;
+    board.forEach(
+      (intersection) => (white_points += intersection === Color.WHITE ? 1 : 0),
    );


Why stop using reduce()?

reduce() is not a method of the Fillable interface.

If we remove it, we should get rid of count_color function below.

OTOH, I have no problem adding reduce to the Fillable interface either. (At this point the interface has grown to much more than "something that can be flood filled" 😂)

Alternatively, we could also have Fillable return a one-dimensional array and use reduce on that.

I'm fine with any of these options, so I'd like to leave the decision to you.

We can also undo the changes here and continue using this type instead of fillable:

export declare type BadukBoard<TColor> = Grid<TColor> | GraphWrapper<TColor>;

... which allows using board.reduce()

Oh sure if that allows us to use reduce again, typing as Grid | GraphWrapper would be nice!

In the end I don't have a strong preference - I do want to keep reduce so we can re-use the reducer and keep variables like white_points const, but otherwise I don't feel strongly about how we do it.

JonKo314 · 2024-05-09T14:59:42Z

packages/shared/src/variants/baduk.ts

    );
-    const white_points: number =
-      board.reduce(count_color<Color>(Color.WHITE), 0) + this.config.komi;


Looks like komi is gone.

Oops, thank you for catching that! Fixed

JonKo314 · 2024-05-09T15:00:49Z

packages/shared/src/variants/baduk.ts

-export function groupHasLiberties(group: CoordinateLike[], board: Grid<Color>) {
+export function groupHasLiberties(
+  group: CoordinateLike[],
+  board: Fillable<CoordinateLike, Color>,


Should this be BadukBoard<Color> instead of Fillable<...>?

changed type to BadukBoard

JonKo314 · 2024-05-09T15:07:19Z

packages/shared/src/variants/baduk.ts

+
+export type GridBadukConfig = {
+  width: number;
+  height: number;
+  komi: number;
+  board?: GridBoardConfig;
+};


Can we use something like

export type GridBadukConfig = { width: number; height: number; komi: number; } | { board: GridBoardConfig; komi: number; }

otherwise there's room for trouble:

{ komi: 6.5; width: 9; height: 9; board: { type: "grid"; width: 19; height: 19; } }

JonKo314 · 2024-05-09T15:09:33Z

packages/shared/src/variants/drift.ts

+  Color,
+  groupHasLiberties,
+  isGridBadukConfig,
+} from "./baduk";

 export interface DriftGoConfig extends BadukConfig {


Can this extend GridBadukConfig instead?

changed to type and extend GridBadukConfig

JonKo314 · 2024-05-09T15:11:50Z

packages/shared/src/variants/drift.ts

  private typedConfig: DriftGoConfig;

  constructor(config?: DriftGoConfig) {
+    if (config && !isGridBadukConfig(config)) {
+      throw Error("config for drift must be for board type grid");
+    }
    super(config);
    this.typedConfig = config ?? this.defaultConfig();
  }


I think we can change the type of the board to a Grid here.

See

class Board { foo() { return "graph"; } } class SpecificBoard extends Board { foo() { return "grid"; } bar() { return "asdf"; } } class GameParent { constructor(protected board: Board) { } public foo() { return this.board.foo(); } } class GameChild extends GameParent { constructor(protected board: SpecificBoard) { super(board); } public bar() { return this.board.bar(); } } const game = new GameChild(new SpecificBoard()); console.log(game.foo()); // prints "grid" console.log(game.bar()); // prints "asdf"

Then we wouldn't need the as Grid type assertions.

benjaminpjones · 2024-05-10T01:44:08Z

packages/shared/src/variants/quantum.ts

@@ -23,7 +23,7 @@ class BadukHelper {
    this.badukGame.playMove(player, "pass");
  }

-  play(player: number, move: string): Coordinate[] {


Nit: I like the rule "Return the most specific type, accept the most generic type". In other words, I usually accept CoordinateLike as argument, and return Coordinate. That way, callers have access to all the utility methods of the Coordinate class, but only need to pass the bare minimum ({x: 1, y: 2}) as a parameter.

Changed the types in quantum.ts thusly

Revert to 2D array board and introduce GridBaduk class

benjaminpjones · 2024-05-10T02:11:17Z

packages/shared/src/variants/quantum.ts

+  currBoard: BadukBoardType<Color>,
+): CoordinateLike[] {
+  const captures: CoordinateLike[] = [];
+  prevBoard.forEach((color, coordinate) => {


Nice generic refactor!

benjaminpjones · 2024-05-10T02:14:15Z

packages/shared/src/variants/tetris.ts

@@ -12,7 +11,7 @@ export class TetrisGo extends Baduk {
  }
 }

-function getGroup(pos: CoordinateLike, board: Grid<Color>) {
+function getGroup(pos: CoordinateLike, board: BadukBoardType<Color>) {


This implementation won't work for graph boards, even with the GraphWrapper. Luckily, I think this is just a grid version of group_utils.getGroup() and can be replaced altogether.

Right, thanks for pointing this out! Replaced by group_utils.getGroup()

merowin · 2024-05-10T10:04:28Z

Ok, I think I'm at a point where we can (hopefully) merge this. Here's an overview of what I did since yesterday:

Merge in Bens Baduk adapter class - Thank you Ben!
change type of Baduk config such that it uses the same structure as the board factory expects, or has the old type
-> The goal is to get rid of the legacy type with a DB migration in future, and support both in the meantime
Fix the introduced type errors with a function that extracts width, height from a GridBadukConfig (hopefully we can also simplify this after a DB migration).
The config forms now emit a config for the new type structure, so this will impact the structure of newly created games in the DB.
Try to adress the code review remarks
Test that the site doesn't crash

benjaminpjones

Hey looks good, no blocking comments. I haven't tested the UI changes, but I trust you will at least run through the game creation flow with one of the affected variants.

Thanks for doing this!

benjaminpjones · 2024-05-10T18:07:32Z

packages/shared/src/variants/baduk.ts

+// This was used before... will need to figure out where it went
+// eslint-disable-next-line @typescript-eslint/no-unused-vars


Suggested change

// This was used before... will need to figure out where it went

// eslint-disable-next-line @typescript-eslint/no-unused-vars

I added these comments to suppress the linter. Should be good to remove now that we reverted the reduce(count_color()) code

removed comments

benjaminpjones · 2024-05-10T18:09:15Z

packages/shared/src/variants/baduk.ts

@@ -190,19 +216,48 @@ export class Baduk extends AbstractGame<BadukConfig, BadukState> {
    this.phase = "gameover";
  }

-  defaultConfig(): BadukConfig {
+  defaultConfig(): GridBadukConfig {
    return { width: 19, height: 19, komi: 6.5 };


If top level width and height are now considered legacy, we may want to update this return value.

changed to return new type structure

benjaminpjones · 2024-05-10T18:15:56Z

packages/shared/src/variants/quantum.ts

    for (const pos of group) {
      this.badukGame.board.set(pos, Color.EMPTY);
    }
  }
 }

-export class QuantumGo extends AbstractGame<BadukConfig, QuantumGoState> {
+export class QuantumGo extends AbstractGame<GridBadukConfig, QuantumGoState> {


Doesn't have to change in this PR, but I think QuantumGo can be adapted to graph boards with only moderate effort.

That's the plan eventually! I believe this will work as soon as I changed the vue-client components for rendering the board for Baduk. But that's for another PR.

benjaminpjones · 2024-05-10T18:20:24Z

packages/vue-client/src/components/boards/MulticolorGridBoard.vue

@@ -15,7 +15,7 @@ import { positionsGetter } from "./board_utils";
 const props = defineProps<{
  board: (MulticolorStone | null)[][];
  background_color?: string;
-  config: { height: number; width: number };
+  config: { width: number; height: number };


Perhaps change this property name from config to dimensions since it's no longer representative of the game config.

changed props name to board_dimensions

merowin added 2 commits May 9, 2024 12:54

refactor baduk and inheriting variants to use board interface fillable

184245b

cleanup

5c1856d

merowin requested review from benjaminpjones and JonKo314 May 9, 2024 11:17

fix error messages

abb6445

benjaminpjones reviewed May 9, 2024

View reviewed changes

JonKo314 reviewed May 9, 2024

View reviewed changes

benjaminpjones reviewed May 10, 2024

View reviewed changes

benjaminpjones and others added 3 commits May 9, 2024 22:20

Revert to 2D array board and add GridAdapter.

bd2a0b1

Merge pull request #256 from govariantsteam/grid-baduk-adapter

17b5a17

Revert to 2D array board and introduce GridBaduk class

change config type, add method for width, height

4084a28

benjaminpjones reviewed May 10, 2024

View reviewed changes

merowin added 5 commits May 10, 2024 05:29

remove unused code, use group utils in tetris

637bbc1

refactor types

b8d2613

use function in board components to get width and height

9f7978f

use grid baduk temporarily, fix config forms

9a2fb9d

fix errors introduced by new config type

b6a8843

merowin marked this pull request as ready for review May 10, 2024 09:51

add reduce in graph wrapper and readd usage in baduk

9989b55

benjaminpjones approved these changes May 10, 2024

View reviewed changes

rename props, default to new config

fdb2b4c

merowin merged commit 089225e into main May 11, 2024
3 checks passed

		// This was used before... will need to figure out where it went
		// eslint-disable-next-line @typescript-eslint/no-unused-vars

Add new boards to Baduk variant #255

Add new boards to Baduk variant #255

Conversation

merowin commented May 9, 2024 • edited

Still needs to be done:

benjaminpjones May 9, 2024 • edited

Choose a reason for hiding this comment

benjaminpjones commented May 9, 2024

benjaminpjones commented May 9, 2024 • edited

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

merowin May 10, 2024 • edited

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

merowin May 10, 2024 • edited

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

JonKo314 May 9, 2024 • edited

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

benjaminpjones May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merowin commented May 10, 2024 • edited

benjaminpjones left a comment

Choose a reason for hiding this comment

benjaminpjones May 10, 2024 • edited

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

benjaminpjones May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminpjones May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merowin commented May 9, 2024 •

edited

benjaminpjones May 9, 2024 •

edited

benjaminpjones commented May 9, 2024 •

edited

merowin May 10, 2024 •

edited

merowin May 10, 2024 •

edited

JonKo314 May 9, 2024 •

edited

benjaminpjones May 10, 2024 •

edited

merowin commented May 10, 2024 •

edited

benjaminpjones May 10, 2024 •

edited

benjaminpjones May 10, 2024 •

edited

benjaminpjones May 10, 2024 •

edited