add dou dizhu #969

rezunli96 · 2022-12-05T04:02:46Z

Hi, I implemented dou dizhu.

After some thinking process, there are a few places that I implemented differently from what Wikipedia described. Specifically, for the hand category airplane+solo, I decided to permit cases like 333-444-555-7-7-7. E.g., pairs, trios can be kickers. For more details, please see my blog.

Also, I plan to implement mahjong in the future if nobody had claimed it yet.

open_spiel/games/CMakeLists.txt

lanctot · 2022-12-05T09:06:22Z

Nice one!!!

I put a comment in the makefile to help with the failing test.

lanctot · 2022-12-05T09:07:39Z

Also, I plan to implement mahjong in the future if nobody had claimed it yet.

It is unclaimed, so feel free. Thanks!

finbarrtimbers

I have not reviewed the .cc files yet; I will wait for you to make changes to the headers & tests first if that's ok.

open_spiel/games/dou_dizhu.h

open_spiel/games/dou_dizhu/dou_dizhu_utils_test.cc

open_spiel/games/dou_dizhu_test.cc

open_spiel/games/dou_dizhu/dou_dizhu_utils_test.cc

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

open_spiel/games/dou_dizhu.cc

…ossible

rezunli96 · 2022-12-08T04:38:59Z

@finbarrtimbers Thanks for the review! Here is one issue I am uncertain how to deal with: in several functions I use std::vector& as input type because during the procedures it may dynamically enlarge the vector. It cannot be known how long this vector will be beforehand. Do you know how to use absl::Span in case like this?

lanctot · 2022-12-08T09:12:07Z

@finbarrtimbers Thanks for the review! Here is one issue I am uncertain how to deal with: in several functions I use std::vector& as input type because during the procedures it may dynamically enlarge the vector. It cannot be known how long this vector will be beforehand. Do you know how to use absl::Span in case like this?

In this case you can pass it in via a pointer, std::vector<type>* var and then use pointer semantics (only use reference in function arguments when they are const.. this is a Google C++ style requirement).

absl::Span is meant-for read-only (so, not for cases like the one you're describing), and it often adds a lot of complexity for not much gain-- I would use it sparingly.

rezunli96 · 2022-12-08T15:41:51Z

@lanctot Thanks!
@finbarrtimbers Please take a look. Currently I change those read-only to const &, and those who will be overwritten to *. Please let me know if it is the right way!

finbarrtimbers · 2022-12-08T17:08:45Z

Thanks for the review! Here is one issue I am uncertain how to deal with: in several functions I use std::vector& as input type because during the procedures it may dynamically enlarge the vector. It cannot be known how long this vector will be beforehand. Do you know how to use absl::Span in case like this?

Yes, sorry, I might have mentioned Span in the wrong context.

One slight difference to what Marc said- Span is fine when you're modifying the objects within the container, just not when you're modifying the container itself (i.e. resizing it).

If you're not modifying the container or any of the objects within it, specify the type as absl::Span<const T>, and it should automatically be converted (no need to do anything).

If you're modifying the objects within the container, then the type should be specified as absl::Span<T> and you need to call absl::MakeSpan on the container.

Span is an abstraction around fixed-size, contiguous blocks of memory (vector<T>, array<T>, etc.). It can be thought of as a convenience wrapper around the {pointer, length} tuple. So if you're resizing the underlying container, you shouldn't use Span (and I shouldn't have suggested it), as this invalidates the underlying pointer.

finbarrtimbers · 2022-12-08T17:13:44Z

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

+  else SpielFatalError("Non valid rank");
+}
+
+std::string FormatSingleHand(const std::array<int, kNumRanks>& hand){


So, this is an example of something that should be a Span. It should convert automatically.

finbarrtimbers · 2022-12-08T17:14:38Z

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

+}
+
+// given a single-rank hand, map it to action id
+int SingleRankHandToActionId(const std::array<int, kNumRanks>& hand){


Another example of Span, as you're not modifying the hand container (or any of the members).

finbarrtimbers · 2022-12-08T17:15:21Z

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

+// given an arbitrary hand, search for possible single-rank hands
+// if prev_action = kInvalidAction, search for all possible such hands
+// otherwise, only search for those that are ranked higher than prev_action
+void SearchSingleRankActions(std::vector<Action>* actions, const std::array<int, kNumRanks>& hand, int prev_action = kInvalidAction){


This signature is a good example too- actions is great as a pointer, while hand should be a Span.

finbarrtimbers · 2022-12-08T17:19:30Z

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

+// and the result hand is stored in ans_hand reference
+bool dfs_airplane_kicker(int chain_length, int depth,
+              int target_count, int& count, int max_search_rank,
+      int* used_rank, int* ans_hand, 


I think I was unclear/did a bad job reviewing this.

Originally, these were std::array<int, kNumRanks>&. I think that the type here should be absl::Span<int>.

You would convert them into a span like this:

dfs_airplane_kicker(chain_length, depth+1, target_count, count, rank, absl::MakeSpan(used_rank), absl::MakeSpan(ans_hand), kicker_type)

finbarrtimbers · 2022-12-08T17:22:48Z

open_spiel/games/dou_dizhu/dou_dizhu_utils.cc

+// a dfs backtrack algorithm that found the action ids of all possible airplane combination
+// the action ids are stored in action_ids reference
+void dfs_add_all_airplane_kickers(int chain_head, int chain_length, int depth, int max_search_rank,
+      int* used_rank, const int* ans_hand, 


Similarly, here, I think that you should make this signature:

absl::Span<int> used_rank, absl::Span<const int> ans_hand, std::vector<Action>* action_ids

(and call absl::MakeSpan(used_rank) on the calling code).

finbarrtimbers · 2022-12-08T17:28:35Z

I went through and made some comments about specific signatures- please let me know if that helps or if you have any questions.

Thanks for working with me on this review- I think you're 95% of the way there, just a few changes to make, mostly just swapping some function signatures.

rezunli96 · 2022-12-08T18:10:23Z

Thanks that helps a lot! Some final questions.

For a multidimensional array, should we transform it to absl::Span<absl::Span> or absl::Span<std::array>? If it is the former, is there a convenient constructor similar to absl::MakeSpan that can transform all inner array to Span? Or should we just write a loop and deal with each inner array explicitly?
So using absl::Span is only required for function arguments right? For return type is it ok to use std::array<std::array>?

finbarrtimbers · 2022-12-08T19:05:01Z

For multidimensional arrays, I'd just not use Span, as those are very tied to the specific container types, and there's no convenient c'tor like MakeSpan.
For your code, I think the only places I would use them are for inputs. But more generally, for outputs, I would use them only in place of const&. E.g. if you have a class which returns const std::vector<int>&, it should return absl::Span<const int>. But only in that case, as Span doesn't transfer ownership, so if you're returning vector<int>, you should not use Span.

rezunli96 · 2022-12-08T19:19:59Z

Thanks! The arguments should now all be changed to absl::Span if they can.

finbarrtimbers · 2022-12-08T20:47:28Z

This looks great! I'll be merging this internally, and it should be merged at head soon. Thanks for working with me, @rezunli96!

lanctot · 2022-12-08T20:58:22Z

Thank you @finbarrtimbers for such a thorough review and great advice!

rezunli96 added 2 commits December 4, 2022 22:53

add dou dizhu

c8ee9ab

remove binary executable

8095c83

lanctot reviewed Dec 5, 2022

View reviewed changes

open_spiel/games/CMakeLists.txt Show resolved Hide resolved

lanctot mentioned this pull request Dec 5, 2022

Call for New Games #843

Open

rezunli96 added 5 commits December 5, 2022 09:21

add dou_dizhu_utils in CMAKEFILE

e2cce79

add playthrough. add player id check in ObservationString

f5f6c8b

fix bugs in SingleTrioComb hands

12e4474

add back util tests

ecbd7f9

fix first player & player order bugs

2936adc

finbarrtimbers self-requested a review December 6, 2022 19:55

finbarrtimbers suggested changes Dec 6, 2022

View reviewed changes

rezunli96 added 3 commits December 6, 2022 17:25

clarify game rules; move constants to .cc files

2f4b059

refine hand initialization

3c73484

remove commented #include

7a9541b

finbarrtimbers reviewed Dec 7, 2022

View reviewed changes

open_spiel/games/dou_dizhu/dou_dizhu_utils_test.cc Outdated Show resolved Hide resolved

finbarrtimbers suggested changes Dec 7, 2022

View reviewed changes

make variable const possible, make function arguments absl::Span if p…

d2185da

…ossible

change reference to pointer

c82cbf6

add back util test

571130d

finbarrtimbers reviewed Dec 8, 2022

View reviewed changes

transform to absl::Span

2452eb3

finbarrtimbers approved these changes Dec 8, 2022

View reviewed changes

finbarrtimbers closed this Dec 8, 2022

finbarrtimbers reopened this Dec 8, 2022

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 Dec 9, 2022

OpenSpiel merged commit b8aabe4 into google-deepmind:master Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add dou dizhu #969

add dou dizhu #969

rezunli96 commented Dec 5, 2022

lanctot commented Dec 5, 2022

lanctot commented Dec 5, 2022 •

edited

finbarrtimbers left a comment

rezunli96 commented Dec 8, 2022

lanctot commented Dec 8, 2022

rezunli96 commented Dec 8, 2022

finbarrtimbers commented Dec 8, 2022

finbarrtimbers Dec 8, 2022

finbarrtimbers Dec 8, 2022

finbarrtimbers Dec 8, 2022

finbarrtimbers Dec 8, 2022

finbarrtimbers Dec 8, 2022

finbarrtimbers commented Dec 8, 2022

rezunli96 commented Dec 8, 2022 •

edited

finbarrtimbers commented Dec 8, 2022 •

edited

rezunli96 commented Dec 8, 2022 •

edited

finbarrtimbers commented Dec 8, 2022

lanctot commented Dec 8, 2022

add dou dizhu #969

add dou dizhu #969

Conversation

rezunli96 commented Dec 5, 2022

lanctot commented Dec 5, 2022

lanctot commented Dec 5, 2022 • edited

finbarrtimbers left a comment

Choose a reason for hiding this comment

rezunli96 commented Dec 8, 2022

lanctot commented Dec 8, 2022

rezunli96 commented Dec 8, 2022

finbarrtimbers commented Dec 8, 2022

finbarrtimbers Dec 8, 2022

Choose a reason for hiding this comment

finbarrtimbers Dec 8, 2022

Choose a reason for hiding this comment

finbarrtimbers Dec 8, 2022

Choose a reason for hiding this comment

finbarrtimbers Dec 8, 2022

Choose a reason for hiding this comment

finbarrtimbers Dec 8, 2022

Choose a reason for hiding this comment

finbarrtimbers commented Dec 8, 2022

rezunli96 commented Dec 8, 2022 • edited

finbarrtimbers commented Dec 8, 2022 • edited

rezunli96 commented Dec 8, 2022 • edited

finbarrtimbers commented Dec 8, 2022

lanctot commented Dec 8, 2022

lanctot commented Dec 5, 2022 •

edited

rezunli96 commented Dec 8, 2022 •

edited

finbarrtimbers commented Dec 8, 2022 •

edited

rezunli96 commented Dec 8, 2022 •

edited