Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new game: Kalah #841

Closed
wants to merge 16 commits into from
Closed

Add new game: Kalah #841

wants to merge 16 commits into from

Conversation

Jazeem
Copy link
Contributor

@Jazeem Jazeem commented May 21, 2022

Mancala is one of the oldest games that's still played with small stones and rows of holes in a board. The objective is to capture the most stones (https://en.wikipedia.org/wiki/Mancala)

Even though the rules are simple, players can develop complex strategies in Mancala. It seems like a good addition for openspiel.

@lanctot
Copy link
Collaborator

lanctot commented May 21, 2022

I agree, this will make a great addition to OpenSpiel. Thanks!

Copy link
Collaborator

@lanctot lanctot left a comment

Choose a reason for hiding this comment

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

I made a few minor comments on style, and you need to add some tests before we can import it, but otherwise looks good!

(This is nicely in time for the release we're planning this or next week..!)


void BasicMancalaTests() {

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some basic tests. At the bare minimum, you should do the simulation tests (see e.g. breakthrough_test.cc).

It's always better to have also have a few Mancala-specific tests too (see e.g. backgammon_test.cc), especially f there are any tricky rules situations that might occur.

}

//capturing logic
if(board_[current_pit] == 1 && IsPlayerPit(current_player_, current_pit) && board_[GetOppositePit(current_pit)] > 0){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Space after the bracket here: e.g.

if (...) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Similarly elsewhere throughout the file)

}

std::string MancalaState::ActionToString(Player player,
Action action_id) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove two spaces

}
absl::StrAppend(&str, "\n");


Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove extra blank line, similarly below

}

void MancalaState::UndoAction(Player player, Action move) {
//Yet to implement Undo
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either remove the comment, or implement the function. Many of the games do not have Undo, so you can just remove the function if it's not correct.

If you leave it, please add one of the simulation tests that tests the undo: https://github.com/deepmind/open_spiel/blob/63e8a99b70a4ea2eb934740d8fb1d2add2dd258b/open_spiel/tests/basic_tests.h#L57

@lanctot lanctot mentioned this pull request May 23, 2022
@Jazeem Jazeem requested a review from lanctot May 23, 2022 12:35
Copy link
Collaborator

@lanctot lanctot left a comment

Choose a reason for hiding this comment

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

Hi @Jazeem,

Thanks for the changes. I just remembered we already have Oware (see here) and Mancala is a family of games. What variant or specific version does this one implement? Is it different from Oware?

@Jazeem
Copy link
Contributor Author

Jazeem commented May 24, 2022

Hi @lanctot,

I was unaware of this and have done a bit of reading regarding the same. This specific variant seems to be called Kalah (https://en.wikipedia.org/wiki/Kalah).

Also, since there's already a Mancala game in OpenSpiel, it might be better to use the same terminology in the code regarding game elements, eg: Houses, Seeds.

@lanctot
Copy link
Collaborator

lanctot commented May 24, 2022

Ok so would you say Kalah and Oware are actually different in terms of rules?

(If so, maybe best to rename your game to kalah?)

If they are indeed equivalent then we should probably not have two identical versions of the same game.

@Jazeem
Copy link
Contributor Author

Jazeem commented May 24, 2022

Yes. Kalah and Oware are different in terms of boards used and rules involved. But have a few common elements too.

It would be better to rename this to Kalah. But Mancala is the more popular name, so this confusion that Mancala is actually a family of games would still remain I guess.

@lanctot lanctot added the imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! label May 26, 2022
@Jazeem Jazeem changed the title Add new game: Mancala Add new game: Kalah May 26, 2022
@Jazeem
Copy link
Contributor Author

Jazeem commented May 26, 2022

@lanctot Renamed game to Kalah, and renamed game elements house, seed, store to match with Oware

@lanctot
Copy link
Collaborator

lanctot commented May 26, 2022

@lanctot Renamed game to Kalah, and renamed game elements house, seed, store to match with Oware

Oh no, I've already imported it and I made more changes from internal review. I think it's ok to keep the name as "mancala" and I just added a note that the rule set is Kalah (and added clarifications in both headers to point to Oware and Mancala).

If you want, you can submit a follow-up PR when I sync with github and it gets merged on Monday for the renaming. I think it is also fine to leave it as it is, though, too.

Thanks again for this game!

@Jazeem
Copy link
Contributor Author

Jazeem commented May 26, 2022

Hi @lanctot,

Let's leave it is as is then.
I saw your call for new games. I can contribute to one of them also if needed. Let me know if there's any that's not been taken up yet.

@lanctot
Copy link
Collaborator

lanctot commented May 26, 2022

I saw your call for new games. I can contribute to one of them also if needed. Let me know if there's any that's not been taken up yet.

Cool! I'll probably code crib myself (but slowly over time). From that list: if you're looking for easier ones, I think Nim and Nine Men's Morris are not being worked on. For the more difficult, Checkers would be really good to have.

But also feel free to use that thread to suggest more, yourself, too!

@Jazeem
Copy link
Contributor Author

Jazeem commented May 27, 2022

Hi @lanctot,

Okay, I'll work on checkers. Since you already have Clobber, I'll start by extending that.

OpenSpiel pushed a commit that referenced this pull request May 28, 2022
PiperOrigin-RevId: 451201970
Change-Id: If3c191ab34283618115baad8b450a91620127450
@lanctot lanctot closed this May 28, 2022
@lanctot
Copy link
Collaborator

lanctot commented May 28, 2022

Ok it has been merged into master.

However, I just noticed-- I forgot to ask you to add it to the games list. Can I ask you to submit another PR for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants