Add new game: Checkers #861

Jazeem · 2022-06-10T09:58:41Z

No description provided.

lanctot · 2022-06-10T11:34:31Z

Thanks!

Seems like the checkers test is causing a seg fault. This usually means there's a bad memory access (out of bounds indexing into an array).

To find it, I recommend building in debug mode (compiler flags -O0 and possibly -g as well) and run the test using valgrind: https://stackoverflow.com/questions/5134891/how-do-i-use-valgrind-to-find-memory-leaks.

lanctot · 2022-06-10T11:38:13Z

Setting the BUILD_TYPE environment variable to Debug should have all the flags you need, see here https://github.com/deepmind/open_spiel/blob/c1fbaca644aaf6c7dd82eea4f62cccd9f6469a5d/open_spiel/CMakeLists.txt#L43. So you need to rebuild from scratch: (remove the build directory entirely, then set the env var and rerun the build) then run it manually through valgrind.

Jazeem · 2022-06-10T13:47:38Z

Thanks. Let me look into it!

Jazeem · 2022-06-13T06:51:56Z

Hi @lanctot, something I've noticed in clobber. In this line, shouldn't the rank of the TensorView always be 3 instead of kCellStates?

Maybe I'm missing something.

https://github.com/deepmind/open_spiel/blob/c1fbaca644aaf6c7dd82eea4f62cccd9f6469a5d/open_spiel/games/clobber.cc#L393

lanctot · 2022-06-13T11:53:42Z

Hi @lanctot, something I've noticed in clobber. In this line, shouldn't the rank of the TensorView always be 3 instead of kCellStates?

kCellStates is always 3: one for empty, one for white, and one for black: https://github.com/deepmind/open_spiel/blob/c1fbaca644aaf6c7dd82eea4f62cccd9f6469a5d/open_spiel/games/clobber.cc#L33

We used kCellStates here to be explicit that there's one plane per cell state (empty, white, black) and also for consistency with the other games that do it this way.

lanctot · 2022-06-13T11:55:24Z

Thanks for fixing. I have not looked at the code yet. Curious: how did you handle the case of multiple captures?

Jazeem · 2022-06-13T13:20:00Z

Thanks for fixing. I have not looked at the code yet. Curious: how did you handle the case of multiple captures?

I spread it across multiple turns to keep it simple. If a player makes a capture and the same piece has another capture move possible, then the turn will not go to the opponent. The player will get another turn with only the remaining capture move available (all capture moves are anyways mandatory).

But since you've mentioned it. I've noticed an edge case I've missed for multiple jumps. I'll update the code, add test cases and push again.

Jazeem · 2022-06-13T13:31:37Z

https://github.com/deepmind/open_spiel/blob/c1fbaca644aaf6c7dd82eea4f62cccd9f6469a5d/open_spiel/games/clobber.cc#L393

We used kCellStates here to be explicit that there's one plane per cell state (empty, white, black) and also for consistency with the other games that do it this way.

My doubt was, to do this wouldn't you have replaced kNumPlayers + 1 with kCellStates inside the braces?

Suppose if I were to add another dimension to the Tensor, a boolean to indicate whether the cell is on the boundary or not. Wouldn't that increase the rank from 3 to 4, although kCellStates value is 3?

Maybe I've got this jumbled up and should go through tensor_view.h

lanctot · 2022-06-13T13:46:19Z

Sorry -- you are indeed correct. I had forgotten the use of TensorView, but it refers to the number of dimensions and should not be confused with the cell states. (It just happens to be 3 in this case, so it works.)

This should read TensorView<3> in clobber. (Do you want to submit a separate PR? If not, very easy to change on our side).

Yeah, using Tensorview does make encoding the observation tensors easier and less error-prone. See the one in breakthrough for example: https://github.com/deepmind/open_spiel/blob/0ffd9584c8be560f4e0f659b3ad93933d9078b48/open_spiel/games/breakthrough.cc#L326

Jazeem · 2022-06-13T13:52:27Z

This should read TensorView<3> in clobber. (Do you want to submit a separate PR? If not, very easy to change on our side).

I can create a separate PR.

P.S. I was checking the contributor graph and noticed 1 commit against my name, even though when I click on it I can see several more. Curious to know if I'm missing any git configurations to have caused this discrepancy.

lanctot · 2022-06-13T14:02:07Z

P.S. I was checking the contributor graph and noticed 1 commit against my name, even though when I click on it I can see several more. Curious to know if I'm missing any git configurations to have caused this discrepancy.

Hmm, yeah.. I see that but I'm not really sure what the cause is.

I think our import/export tool (copybara) squashes all the commits from one PR into a single commit when it gets imported. But there has to be more to it than that, because you also updated games.md in a separate PR. Let's see what it looks like after this next PR and Checkers. Could also be how the views get summarized/aggregated on the github side.

lanctot · 2022-07-03T13:32:39Z

open_spiel/games/checkers.h

+
+// Implementation of the board game Checkers.
+// https://en.wikipedia.org/wiki/Checkers
+//


I noticed your implementation has rows and columns as paramters. We usually list them all here (see headers of other games)

lanctot · 2022-07-03T13:33:08Z

open_spiel/games/checkers.h

+//
+// Some notes about this implementation:
+// - Drawing: 
+//     Game is drawn if no pieces have been removed in 40 moves


40 Seems a bit arbitrary-- is this a commonly chosen value for this game? If so maybe mention that in the comments here?

lanctot · 2022-07-03T13:34:57Z

open_spiel/games/checkers.h

+
+  Player current_player_ = 0;  // Player zero (White, 'o') goes first.
+  Player outcome_ = kInvalidPlayer;
+  int multiple_jump_piece_ = 0; // Piece in the board who can do multiple jump. Represented by row * rows_ + column


Can you say a bit more in the comments here about how this variable is used or why it's needed?

This variable is used to indicate that the last moved piece can make an additional jump (multiple jumps). In that case LegalActions is filtered to only represent moves available for this piece.

I'm not entirely happy with the implementation since I had to use a global variable, if you have other thoughts, do share them

Ok great. But, what I meant was: could you elaborate on that in the comment here.

Also, can we please clarify what version of the rules you are using (at the top of the header file)? I wasn't aware of forced multi-captures, e.g. I don't see anything about it mentioned on https://en.wikipedia.org/wiki/Checkers. Maybe this is a common convention used in computer Checkers? If so, please clarify that in the documentation at the top of the file. Also e.g. the choice of 40 moves without capture, had not heard of it.. but maybe this is part of a specific rule set or checkers convention?

It's mentioned in the Wiki link that

When capturing an opponent's piece is possible, capturing is mandatory in most official rules.

That's why I added those. About the 40 move rule, I could find some mentions of it online

http://www.flyordie.com/games/help/checkers/en/games_rules_checkers.html

tewalds

Thanks for submitting this game! It's a great addition!

Feel free to push back on some of the more design type comments/suggestions.

It's also worth taking a look at the Google C++ style guide. You followed most of it, but in particular the 80 char line length limit needs a closer look.

tewalds · 2022-07-01T16:16:20Z

open_spiel/games/checkers.h

+namespace open_spiel {
+namespace checkers {
+
+inline constexpr int kNumPlayers = 2;


Marking these as inline seems redundant.

That's true. Removed it.

tewalds · 2022-07-01T16:17:26Z

open_spiel/games/checkers.h

+
+  Player current_player_ = 0;  // Player zero (White, 'o') goes first.
+  Player outcome_ = kInvalidPlayer;
+  int multiple_jump_piece_ = 0; // Piece in the board who can do multiple jump. Represented by row * rows_ + column


We've got an 80 char line limit.

Have fixed these

tewalds · 2022-07-01T16:18:25Z

open_spiel/games/checkers.h

+
+  Player current_player_ = 0;  // Player zero (White, 'o') goes first.
+  Player outcome_ = kInvalidPlayer;
+  int multiple_jump_piece_ = 0; // Piece in the board who can do multiple jump. Represented by row * rows_ + column


Is there only one piece that can jump multiple times?

Yes. This variable is used to identify a piece with a multiple jump possibility that has completed it's first jump. Hence there'll be only one such piece.

tewalds · 2022-07-01T16:21:05Z

open_spiel/games/checkers.cc

@@ -0,0 +1,546 @@
+// Copyright 2019 DeepMind Technologies Limited


tewalds · 2022-07-01T16:24:28Z

open_spiel/games/checkers.cc

+// Index 1: Direction is diagonally up-right.
+// Index 2: Direction is diagonally down-right.
+// Index 3: Direction is diagonally down-left.
+constexpr std::array<int, kNumDirections> kDirRowOffsets = {{-1, -1, 1, 1}};


Is it worth having this as
std:array<std::array<int, 2>, kNumDirection>
that includes both row and column?

Is it even worth having a Point/Location type struct with x/y or row/column values, possibly with a + operator.
Here are two that I wrote:
https://github.com/deepmind/open_spiel/blob/master/open_spiel/games/havannah.h#L67
https://github.com/deepmind/open_spiel/blob/master/open_spiel/games/quoridor.h#L54
though they might be overcomplicated for this use case. Feel free to push back on this if you don't think it's a good idea.

Keeping this as is improves readability IMO

tewalds · 2022-07-04T11:28:35Z

open_spiel/games/checkers.cc

+  const int start_column = values[1];
+  const int direction = values[2];
+  const int move_type = values[3];
+  const int captured_piece_type = values[4];


Ah, I see, this is why the piece types are in there. Is it better to put more details in the history and undo based on that instead of including it in the action?

Great suggestion. Have introduced a TurnHistoryInfo struct to do this. Now actions have reduced to 1/4th.

tewalds · 2022-07-04T11:32:26Z

open_spiel/games/checkers_test.cc

+void BasicSerializationTest() {
+  std::shared_ptr<const Game> game = LoadGame("checkers");
+  std::unique_ptr<State> state = game->NewInitialState();
+  std::unique_ptr<State> state2 = game->DeserializeState(state->Serialize());


It seems worth testing a more complicated state.

Added one more test with a board after a few moves

tewalds · 2022-07-04T11:32:42Z

open_spiel/games/checkers_test.cc

+}
+
+void BasicCheckersTests() {
+  testing::LoadGameTest("checkers");


Can you test different sizes as well?

tewalds · 2022-07-04T11:33:44Z

open_spiel/games/checkers.cc

+    absl::StrAppend(&result, RowLabel(rows_, r));
+
+    for (int c = 0; c < columns_; c++) {
+      absl::StrAppend(&result, StateToString(BoardAt(r, c)));


Is it worth distinguishing between empty black/white squares, as in one that pieces can be on vs ones that are inaccessible? Maybe with dot vs space?

I didn't understand this correctly. Can you elaborate?

tewalds · 2022-07-04T11:38:26Z

open_spiel/games/checkers.cc

+      SetBoard(end_row, end_column, CellState::kEmpty);
+      CellState captured_piece = OpponentState(PlayerToState(player));
+      SetBoard((start_row + end_row) / 2, (start_column + end_column) / 2, 
+        captured_piece_type == PieceType::kMan ? captured_piece : CrownState(captured_piece));


This wrapping is a bit odd.

Have introduced a function here to improve readability

PiperOrigin-RevId: 462677216 Change-Id: I2fc4b0b25d1265d7ff12df6c5b50a6defebbd496

lanctot · 2022-07-25T10:27:15Z

Merged in 808e54e

Jazeem added 18 commits June 10, 2022 15:25

Cloned Checkers from Clobber

355f7da

Changed board initial configuration and checker moving logic

43bf661

Capturing logic added

2460ab8

Added stringing multiple captures into one move

4466be4

Added white and black crowned elements

94e2625

Bonus move logic fixed, Allowed to capture crowned pieces

abfb0b0

outcome_ logic fixed

1591ff0

ObservationTensor updated

8a14ef5

Added basic tests

fb51f99

Code cleanup

b3e72d3

UndoAction and RandomSimTestWithUndo added

b902484

Drawing rule added

d933dd1

MultipleJumpTest added

1af643b

CrownedPieceCanMoveBackwardsTest added

427cd8d

Code cleanup

8dbf9d6

Comments updated

3724ae9

Formatting fixed

8c7e907

games.md updated

efffce1

Fixed SegFault in checkers_test

d8076f1

lanctot requested changes Jul 3, 2022

View reviewed changes

tewalds requested changes Jul 4, 2022

View reviewed changes

Jazeem added 22 commits July 4, 2022 23:38

Comments added to header

b2c3b20

Forcibly end move after piece is crowned

9bbc79b

Lines restricted to 80 char

3af0fe9

Removed redundant inline

f3d29ba

Removed unnecessary StrCat

de57ef5

Removed wrong usage of kDefaultRows

a5dfd54

Indentation fix

2e9fab0

RowLabel arithmetic simplified

d9c6a01

Renamed WhiteCrowned, BlackCrowned to WhiteKing, BlackKing

40ab50c

StringToState argument changed to char

616ec96

Asserting current_player_ is valid in SetCustomBoard

c790597

Early returns in CrownStateIfLastRowReached

d6f2614

CheckersAction struct introduced

9365c16

Multiple jump documentation added

969f4b3

moves_for_last_moved_piece changed from vector to bool

684641e

multiple_jump bool removed

d9ee71f

TurnHistoryInfo added

708f369

RandomSerializationTest added

18fe545

10x10, 12x12 board tests added

cf873b9

GetPieceStateFromTurnHistory function added

36d4685

Capturing comment added

9560acf

lanctot added imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! merged internally The code is now submitted to our internal repo and will be merged in the next github sync. labels Jul 20, 2022

Conform to 80 char line limit

b05c5c0

OpenSpiel pushed a commit that referenced this pull request Jul 25, 2022

Merge pull request #861 from Jazeem:checkers

808e54e

PiperOrigin-RevId: 462677216 Change-Id: I2fc4b0b25d1265d7ff12df6c5b50a6defebbd496

lanctot closed this Jul 25, 2022

		@@ -0,0 +1,546 @@
		// Copyright 2019 DeepMind Technologies Limited

Add new game: Checkers #861

Add new game: Checkers #861

Conversation

Jazeem commented Jun 10, 2022

lanctot commented Jun 10, 2022

lanctot commented Jun 10, 2022

Jazeem commented Jun 10, 2022

Jazeem commented Jun 13, 2022

lanctot commented Jun 13, 2022 • edited

lanctot commented Jun 13, 2022

Jazeem commented Jun 13, 2022

Jazeem commented Jun 13, 2022

lanctot commented Jun 13, 2022

Jazeem commented Jun 13, 2022

lanctot commented Jun 13, 2022

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

lanctot Jul 5, 2022 • edited

Choose a reason for hiding this comment

Jazeem Jul 9, 2022 • edited

Choose a reason for hiding this comment

tewalds left a 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

Choose a reason for hiding this comment

lanctot commented Jul 25, 2022

lanctot commented Jun 13, 2022 •

edited

lanctot Jul 5, 2022 •

edited

Jazeem Jul 9, 2022 •

edited