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

Adding game TwixT #1120

Merged
merged 9 commits into from
Apr 22, 2024
Merged

Adding game TwixT #1120

merged 9 commits into from
Apr 22, 2024

Conversation

stevens68
Copy link
Contributor

@stevens68 stevens68 commented Sep 28, 2023

TwixT is a 2-player, deterministic, perfect information game, played on a 24x24 grid. See Wikipedia. This PR adds it to open_spiel. The board size can be adjusted from 5x5 to 24x24

@google-cla
Copy link

google-cla bot commented Sep 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lanctot
Copy link
Collaborator

lanctot commented Sep 29, 2023

Awesome, thanks! We'll take a look soon.

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.

Thanks for the game!

I have a few comments about style before we take a look at the logic.

I will point out a few things, and the reference is the Google C++ style guide. The C++ code style should be consistent with the rest of the games; you can look at them for reference. For more detail, see this: https://google.github.io/styleguide/cppguide.html

open_spiel/games/twixt/twixt_test.cc Show resolved Hide resolved
open_spiel/games/twixt/twixtboard.h Outdated Show resolved Hide resolved
open_spiel/games/twixt/twixtboard.h Outdated Show resolved Hide resolved
open_spiel/games/twixt/twixtboard.h Outdated Show resolved Hide resolved
open_spiel/games/twixt/twixtcell.h Outdated Show resolved Hide resolved
open_spiel/games/twixt/twixtcell.h Outdated Show resolved Hide resolved
open_spiel/games/twixt/twixtcell.h Outdated Show resolved Hide resolved
@stevens68
Copy link
Contributor Author

Thanks for your quick feedback, lanctot! I will apply the guidelines, add tests and do some simplifications.
Will push the changes within the next 2 weeks.

open_spiel/games/twixt/twixt_test.cc Outdated Show resolved Hide resolved
@lanctot
Copy link
Collaborator

lanctot commented Oct 21, 2023

Thanks for your quick feedback, lanctot! I will apply the guidelines, add tests and do some simplifications. Will push the changes within the next 2 weeks.

Great, thanks. I will be away for 10 days as of today so there will be a delay on my end. Please reply to (and resolve) each of the comments when they are done.

@lanctot
Copy link
Collaborator

lanctot commented Dec 20, 2023

Hi @stevens68, I will take a look over the holidays. Thanks!

@lanctot
Copy link
Collaborator

lanctot commented Jan 28, 2024

Hi @stevens68, time got away from me. Could you pull and merge changes from master to resolve the conflicts, and then push the merge commit to your branch?

@stevens68
Copy link
Contributor Author

Hi @stevens68, time got away from me. Could you pull and merge changes from master to resolve the conflicts, and then push the merge commit to your branch?

Hi @lanctot, thanks for looking into it! Merged the changes and resolved the conflicts.

@lanctot lanctot added ready to import Ready to import 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 Apr 16, 2024
@lanctot
Copy link
Collaborator

lanctot commented Apr 19, 2024

@stevens68, it's been merged and should be included in the next sync to github (probably Monday) and included in the list of additions in the next release. Sorry for the long delay!

BTW, I was delighted to see the ANSI graphics in the ToString, nicely done! 🥰 👍

image

@lanctot lanctot merged commit 56d816e into google-deepmind:master Apr 22, 2024
2 checks passed
@pilot68
Copy link

pilot68 commented Apr 30, 2024

...thx for your reviewing efforts and comments. I learned a lot! take care!

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! merged internally The code is now submitted to our internal repo and will be merged in the next github sync. ready to import Ready to import
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants