Make function for playerHasInsufficientMaterial.
#702
Conversation
| InsufficientMatingMaterial.pawnBlockedByPawn(pawn, position) && cannotAttackBishop | ||
|
|
||
| private def AllOnSameColourSquares(position: Position): Boolean = | ||
| position.white.squares.forall(_.isLight) == position.black.squares.forall(_.isLight) |
There was a problem hiding this comment.
the name of this function is misleading.
In a position where both players have pieces on light and dark squares - like the initial position - then AllOnSameColourSquares returns true.
There was a problem hiding this comment.
Good point, missed that
| position.white.squares.forall(_.isLight) == position.black.squares.forall(_.isLight) | ||
|
|
||
| private def justOneKnightEach(position: Position): Boolean = | ||
| position.onlyKnights && position.white.squares.size == 1 && position.black.squares.size == 1 |
There was a problem hiding this comment.
instead of requesting the list of squares then counting the list, you could do a much faster bit count
…nt that does not override it.
…to player-insufficient-material
| InsufficientMatingMaterial(position, !position.color) | ||
|
|
||
| def playerHasInsufficientMaterial(position: Position): Boolean = | ||
| throw Exception("`playerHasInsufficientMaterial` is not implemented for this variant") |
There was a problem hiding this comment.
We usually don't throw exceptions. If this function is only applicable to Antichess, maybe we should have this as public function in Antichess.
But tbh, I'm not sure. Do you think the issue we had in lichess-org/lila#17114 only happen in Antichess? I haven't verified but I think it could happen to other variants as well.
There was a problem hiding this comment.
@lenguyenthanh All other variants have a king on the board though, so reasoning about tempi would have to account for possible triangulations.
EDIT - in Horde.scala, it seems horde stalemates are treated as insufficient material cases. So there turn may matter:
If imminent fortresses in Horde were considered by lichess, where a king is running after a pawn that will inevitably be stalemated, then turn would matter there too.
Co-authored-by: Thanh Le <lenguyenthanh@hotmail.com>
Co-authored-by: Thanh Le <lenguyenthanh@hotmail.com>
…r and use `opponentHasInsufficientMaterial`.
…to player-insufficient-material
| hasInsufficientMaterial(position.board, !position.color) || hordeClosedPosition(position) | ||
|
|
||
| override def playerHasInsufficientMaterial(position: Position): Boolean = | ||
| throw Exception("`playerHasInsufficientMaterial` is not implemented yet for Horde") |
There was a problem hiding this comment.
We can't have Variant offer a playerHasInsufficientMaterial(position: Position): Boolean function that sometimes throws an exception.
This would either create bugs, or force us to code defensively by catching exceptions. It's not how we do things 'round here.
If Variant.playerHasInsufficientMaterial can't always return a Boolean, then it must return an Option[Boolean] to let the type system know what's up, and force callers to act accordingly.
There was a problem hiding this comment.
@ornicar Updated to do this. Before merging, do you think it'd be good to add tests in each variant for playerHasInsufficientMaterial?
- In most variants, this would just look like making sure the result is the same as
opponentHasInsufficientMaterial(with colours swapped, not preserving turn). Maybe these two checks could be grouped in a function? - In Horde, a test would just make sure the function returns None.
- In Antichess, there are already tests from this PR.
There was a problem hiding this comment.
making sure the result is the same as opponentHasInsufficientMaterial (with colours swapped, not preserving turn).
That's exactly what the function does so I don't think the test would add any value.
We don't need to test it, but are we 100% sure that this affirmation is true?
For all variants except Antichess and Horde, considering turn isn't needed
If you are certain that it's true for standard chess especially, then I think the PR is good to go.
There was a problem hiding this comment.
Variant.playerHasInsufficientMaterial could return Boolean with false mean we don't know.
We have few insufficientMaterial functions. I think we should think about their semantic. I'm thinking about something like:
isInsufficientMaterial == opponentHasInsufficientMaterial && playerHasInsufficientMaterial
There was a problem hiding this comment.
Variant.playerHasInsufficientMaterial could return Boolean with false mean we don't know.
I disagree, I think there's a useful distinction between "no" and "I don't know", that Option[Boolean] provides here. The caller decides whether to treat them differently or not.
There was a problem hiding this comment.
Variant.playerHasInsufficientMaterial could return Boolean with false mean we don't know.
I disagree, I think there's a useful distinction between "no" and "I don't know", that Option[Boolean] provides here. The caller decides whether to treat them differently or not.
Correct me if I'm wrong but I believe both isInsufficientMaterial and opponentHasInsufficientMaterial return false negative sometime as we only check it with our best effort. So we already use the semantic of false means we know it's false or we don't know.
There was a problem hiding this comment.
I believe for these *HasInsufficientMaterial we want to guarantee two things:
- No false positive
- They have to be consistent between each other
There was a problem hiding this comment.
We don't need to test it, but are we 100% sure that this affirmation is true?
For all variants except Antichess and Horde, considering turn isn't needed
If you are certain that it's true for standard chess especially, then I think the PR is good to go.
I'm not sure this is 100 % true either
…return `None` instead of throwing an exception.
Just something that'll be useful in making my insufficient material stuff (from lichess-org/lila#17114) handle antichess properly.