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

Make action IDs relative to pawn position #1221

Closed
wants to merge 2 commits into from

Conversation

tacertain
Copy link
Contributor

@tacertain tacertain commented May 4, 2024

The original implementation of Quoridor mapped action IDs to absolute board positions.
This meant that moving north had a different ID for every pawn position. This commit
adds the option to make the action IDs be consistent for the same move relative to
pawn position.

This commit fixes #1158.

The vector for legal moves is initialized with reserved space to hold the maxiumum number of possible legal moves. It sets max_moves to 5 initially as the max number of pawn moves, including jumps. However, that's only the max in a 2-player game. Normally, you can move in any of the 4 compass directions only. However, if there is an opponent pawn in one of those adjacent squares and a wall behind the pawn, you can choose to "mid-air jump" into one of the two squares diagonally-adjacent to the current pawn position on either side of the opponent. In a 2p game, there's only one possible opponent, so at most you can split one of the possible compass moves into two moves. However, with two or more opponents, it would be possible for two opposing adjacent squares to have opponents in them with walls behind and the other two squares to be open, leading to six possible moves.

This situation is unlikely to happen in a real game, and even if it does, all it means is that there will be one extra memory allocation to expand the vector, but it confused me, so I'm submitting a fix.
The original implementation of Quoridor mapped action IDs to absolute board positions.
This meant that moving north had a different ID for every pawn position. This commit
adds the option to make the action IDs be consistent for the same move relative to
pawn position.
@tacertain
Copy link
Contributor Author

I still need to add some tests with the relative_moves flag set to true, but opening the PR now to get feedback on the method. I have tested it by hand running interactive games.

@tacertain
Copy link
Contributor Author

tacertain commented May 4, 2024

Also, thoughts on making the default relative? I'm generally against breaking changes, and as pointed out in issue #1158, there may be already-trained models that expect absolute action IDs. But in this case, if relative is really much better for training, maybe it should be made the default and try to make it easy for prior models to understand what's happened.

Is there any sort of OpenSpiel version number written into the models that could be checked?

@tacertain
Copy link
Contributor Author

I realized that there's no reason not to have the action IDs for absolute and relative be the same, so I'll update that.

@tacertain
Copy link
Contributor Author

I'm ready to submit the rest of the PR, but want to get advice on what the default should be.

@tacertain tacertain mentioned this pull request May 10, 2024
@tacertain tacertain closed this May 15, 2024
@tacertain tacertain deleted the tacertain-patch-2 branch May 15, 2024 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quoridor Movement Action IDs keep changing
1 participant