fix: avoid impossible ukeire draws in shanten efficiency - #172
Conversation
|
Thanks for the report! I was able to reproduce the issue you pointed out. I'll review the fix diff now. FYI. I'm planning to spend this weekend setting up a validation pipeline using Tenhou game logs and adding test cases as needed. I'll create subtasks under #116 and start working on them. |
smly
left a comment
There was a problem hiding this comment.
A few separate pre-existing issues also became visible while looking into this, but I think we should go ahead and merge this fix for now. This feature, which is part of feat_v2, had been deferred on our side, and we’ll verify that it works correctly for v0.5.0. Thanks for reporting this bug!
| if new_hand_counts[tile_type as usize] >= 4 { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
(note) The new new_hand_counts[tile_type] >= 4 guard makes sense and should prevent the panic.
That said, ukeire += 4 - visible_counts[tile_type as usize] still seems to over-count remaining tiles, because visible_tiles only includes discards / melds / dora indicators and does not include tiles already present in new_hand.
I checked the callers (encode_shanten_into in both observation/encode.rs and observation_3p/encode.rs) and all_visible is built from discards, melds, and dora indicators, so the player's own hand is not included.
For example, if new_hand already has 3 copies and visible_counts is 0, this counts 4 remaining copies even though only 1 can still exist.
This looks like a pre-existing correctness issue rather than something introduced by this PR, so handling it separately would also be reasonable.
| let mut new_hand_counts = [0u8; TILE_MAX]; | ||
| for &tile in &new_hand { | ||
| let tile_type = (tile / 4) as usize; | ||
| if tile_type < TILE_MAX { | ||
| new_hand_counts[tile_type] += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
(nit) new_hand_counts is rebuilt from scratch inside the outer for (idx, _) in hand_tiles.iter().enumerate() loop, even though new_hand differs from hand_tiles by exactly one tile. Building a base count array once before the loop and adjusting it per iteration would be simpler and a bit more efficient:
let mut base_counts = [0u8; TILE_MAX];
for &tile in hand_tiles {
let tile_type = (tile / 4) as usize;
if tile_type < TILE_MAX {
base_counts[tile_type] += 1;
}
}
for (idx, _) in hand_tiles.iter().enumerate() {
let removed_type = (hand_tiles[idx] / 4) as usize;
let mut new_hand_counts = base_counts;
new_hand_counts[removed_type] -= 1;
// ...
}The runtime difference is negligible for a ~14-tile hand, so this is just a cleanup suggestion.
There was a problem hiding this comment.
Pull request overview
Fixes a shanten-efficiency panic by preventing ukeire search from considering illegal “5th copy” draw candidates, and adds regression coverage plus a local repro harness for the reported Tenhou Houou replay.
Changes:
- Skip draw candidates in
calculate_best_ukeire()/_3p()when the post-discard hand already contains 4 copies of that tile type. - Add 4P/3P regression tests ensuring
encode_shanten_efficiency()handles a quad-on-draw state without crashing. - Add a standalone script to reproduce/scan the panic from a local Tenhou JSON paipu via
mjai-reviewer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
riichienv-core/src/shanten.rs |
Filters out impossible draw tile types during ukeire evaluation to avoid illegal hypothetical hands. |
tests/env/test_apply_event.py |
Adds 4P/3P regression tests for quad draw states exercising encode_shanten_efficiency(). |
scripts/repro_encode_shanten_efficiency_houou.py |
Adds a local repro/scanning harness for the Tenhou Houou panic scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| steps_scanned += 1 | ||
| try: | ||
| obs.encode_shanten_efficiency() | ||
| except BaseException as exc: # PanicException inherits BaseException |

Summary
calculate_best_ukeire()andcalculate_best_ukeire_3p()when the post-discard hand already contains four copies of a tileencode_shanten_efficiency()on a quad-containing draw stateProblem
encode_shanten_efficiency()could panic on a real Tenhou replay because the ukeire search was evaluating illegal five-of-a-kind hypothetical hands. The panic surfaced from the nyanten lookup inriichienv-core/src/shanten.rs.Repro
Before this fix, the following replay reproduced the panic locally:
2024010100gm-00a9-0000-1d4dbec0round_index=0,decision_index=90,seat=1Verification
uv run maturin developuv run pytest tests/env/test_apply_event.py -k 'encode_shanten_efficiency_handles_quad_draw'uv run pytest tests/test_shanten.py tests/env/test_sanma.py -k 'shanten'uv run python scripts/repro_encode_shanten_efficiency_houou.py