MahjongRepository / mahjong

Implementation of riichi mahjong related stuff (hand cost, shanten, agari end, etc.)
MIT License
377 stars 38 forks source link

Add Chinese support (partially) and detailed honba and tsumi bon bonus points calculations #26

Closed profthecopyright closed 3 years ago

profthecopyright commented 3 years ago

Hope I got it:)

Nihisil commented 3 years ago

Thanks for the contribution. I will review it tomorrow.

The current system with new attr for Chinese version is fine.

We can add internationalization later if needed. I don't think that there will be more languages to support.

profthecopyright commented 3 years ago

Actually I am working on localization now. I just don't think it is a good idea with too many modifications in a single pull request. I will finish it in my next version.

Nihisil commented 3 years ago

Got it, thanks!

profthecopyright commented 3 years ago

Oh I didn't realize any push to my branch would directly come here. Sorry for the delay and I added some new modifications here.

  1. There was some confusions in the original version with Daichikurin and Daisuurin. Actually Daichikurin means Bamboo Forest in Japanese, which should correspond to sou (looking like bamboos), and Daisuurin means Big Numbers which should be man (10, 000). I have changed the yaku definition and the test cases accordingly.

  2. For fu calculation, I introduced a DOUBLE_VALUED_PAIR reason with 4 fu in lieu of the two VALUED_PAIRs in playerwind == roundwind, as it is a more conventional statement in majhong (連風4符).

  3. For locales and string reporting, I introduce now a TextReporter class and a set of locale dictionaries, since yaku is not the only thing to be translated if we would like to introduce a systematic language support. This could be a intermediate step, to dissociate the language strings from the main code. Hopefully I can transfer it further to the standard gettext locale solutions.

  4. I found a bug in the example of renhou as yakuman. We cannot directly change the number of han for renhou to 13, since this would lead to yakuman han addition to non yakuman han, which does not comply with most mahjong rules, including tenhou. BTW, tenhou.net does not admit renhou as even a yaku, let alone yakuman. (See [url]https://tenhou.net/man/ for detailed rules if you can read Japanese).

  5. For errors in hand config, the original version is not an exhaustive one. There are many cases when certain "occasional yakus" should never be together, like haitei and houtei, haitei and channkan, riichi and daburu riichi, houtei and rinshan, etc. I am hesitating whether we should report a different error string for each of the cases, or just implement a "sanity_check()" method in HandConfig and report only whether or not there are conflicts in the input.

Nihisil commented 3 years ago

Thanks again for your contribution.

There was some confusions in the original version with Daichikurin and Daisuurin.

:+1:

For fu calculation, I introduced a DOUBLE_VALUED_PAIR reason with 4 fu in lieu of the two VALUED_PAIRs in playerwind == roundwind, as it is a more conventional statement in majhong (連風4符).

Yeah, I agree that double fu reason looks better here.

For locales and string reporting, I introduce now a TextReporter class and a set of locale dictionaries, since yaku is not the only thing to be translated if we would like to introduce a systematic language support.

Actually all these error texts were here mainly for developers, not for users. But we can translate them as well, why not. Ideally we can return error code with error text as well, to be more friendly for developers.

I found a bug in the example of renhou as yakuman. We cannot directly change the number of han for renhou to 13, since this would lead to yakuman han addition to non yakuman han, which does not comply with most mahjong rules, including tenhou. BTW, tenhou.net does not admit renhou as even a yaku, let alone yakuman. (See [url]https://tenhou.net/man/ for detailed rules if you can read Japanese).

Yep, I know that renhou is not the yaku on tenhou (and on majsoul as well).

Actually I thought bout renhou more like about additional yaku, not like yakuman.

http://arcturus.su/wiki/Renhou

But it looks like it could be both. Adding han as yaku or be yakuman. And in the second case I agree that we don't need to sum it with other hand.

For errors in hand config, the original version is not an exhaustive one. There are many cases when certain "occasional yakus" should never be together, like haitei and houtei, haitei and channkan, riichi and daburu riichi, houtei and rinshan, etc. I am hesitating whether we should report a different error string for each of the cases, or just implement a "sanity_check()" method in HandConfig and report only whether or not there are conflicts in the input.

For the start there could be one error message for such cases, and later we can improve it to return separate errors.

Nihisil commented 3 years ago

And I noticed that you moved yaku files to non_yakuman package. Was it necessary? I think it could break comparability with old version, in case if someone imported yaku directly in their code.

Theoretically we can release next 1.2.0 version without compatibility with previous version, but I would like to avoid it for now.

I have bunch of improvements in list that we can do for version that will break compatibility, but don't have time for that now :)

Improvements like name tile with index prefix tile_34 or tile_136 and etc,. so it will be clear what to pass to the function. Same for melds and other cases.

Also, I have a need to improve lib performance (to be able run mahjong bots battle faster) which could lead to changing interfaces as well.

Maybe in next a couple of months I will find time for these changes.

profthecopyright commented 3 years ago

OK Sorry I made some mess in changing the names.

I will move non-yakuman directly back to the yaku folder.

And also, I notice that kokushimuso 13menmachi (double kokushi) and pure 9 gates (double chuuren) are listed as 26 hans, instead of 13 hans. This actually is an optional rule that allows for multiple yakumans in a single yaku. tenhou doesnot apply this rule so that double kokushi, double chuuren, 4 big winds and 4 closed triplets single wait are all considered 13 hans. But majsoul (good to know you play that!) considers these as double yakuman (26 hans). I notice this lib is advertised to be the same as tenhou rule, so I think better change the default of these yaku to 13 han and maybe apply some customized ruleset to change it.

Nihisil commented 3 years ago

Actually, the lib should be compatible with all popular rulesets, I didn't want to make it work only with tenhou. This is because the lib has an optional ruleset mechanism.

I have sent you contribution invitation, so you can create new branches and send pull requests without fork now.

The PR overall looks good. I merge it now.

Do you need new version of lib to released to pypy? If yes, I can do that tomorrow. If not, we can do release later, after other changes (if there would be any).

profthecopyright commented 3 years ago

Actually, the lib should be compatible with all popular rulesets, I didn't want to make it work only with tenhou. This is because the lib has an optional ruleset mechanism.

I agree. And I think there should be a systematic way for optional rules (including adding other optional yakus such as tsubagaeshi (ron others' riichi declaration tile), 12-down (4 open melds and single wait), etc). I could work on these in the next version

I have sent you contribution invitation, so you can create new branches and send pull requests without fork now.

The PR overall looks good. I merge it now.

Sounds good.

Do you need new version of lib to released to pypy? If yes, I can do that tomorrow. If not, we can do release later, after other changes (if there would be any).

Not now since I still want to add/revise some new features. After all these are done we can release it to PyPI.