Changes to isDraw and isGameOver (to fix #112) and fixes for games that include put/remove
#417
Conversation
Needed to install this dependency for __tests__/utils.ts to import from
Updated parameter to be an optional object of shape `{ strict?: boolean }` to better align with conventions already established elsewhere in the library.
|
Thanks for the PR @gavin-lb. I'll do my best to review these changes (and familiarize myself with the latest rule updates) sometime this weekend. Since we're technically pre-1.0.0, I'm comfortable breaking backwards compatibility for some of these edge-case draws - meaning we might not need to include the strict parameter on |
|
Well I think it's probably more common for people to want auto claiming of draws on threefold repetition or 50-move rule anyway, so still makes sense for that to be the default (for example, I'm pretty sure Lichess does it that way). But for those that want to implement a stricter adherence to the FIDE rules as written, the option and bug fix are necessary. |
Since this method is very likely going to be called after every single move (for end of game detection) I think it makes sense to optimise it a little bit. Instead of replaying the entire game every turn to determine whether the position has been repeated, we simply keep track of position counts as the game progresses. We just need to make sure to properly update the count when a move is undone or the position reset.
The `load` method used to unnecessarily call `_updateCastlingRights`, `_updateEnPassantSquare`, `_updateSetup` and `fen` methods for every piece it placed on the board (because `put` method called these methods). Optimised `load` by implementing a private `_put` method which doesn't call these methods, then changed publicly exposed API method `put` to be a wrapper around `_put` that calls these methods if `put` was successful.
|
Added a couple optimisations to this PR, but let me know if you would rather they be a separate PR instead. |
|
Fixed a bug with the new However, it is a little unclear to me what the behaviour should be with regards to repetitions and |
|
Using I think the only obvious way to amend the aforementioned problem with This will allow This change would require some alterations across the whole library and would technically affect backwards compatibility. However, it would only affect those users using |
Yep, you're right. I remember now. I'm in agreement with you here. As for the API changes to
@gavin-lb Let's split those into a new PR and I'll merge them today.
My opinion is that |
Required updating history, undo, pgn and loadPgn methods to work with the new _history structure:
- undo method now accepts an optional argument of shape { untilMove?: boolean } (defaults to { untilMove: true }). When true (ie. default behaviour) it will automatically undo all put/removes until the previous move (inclusive). When false, it will only undo the last move/put/remove.
- history method now accepts an onlyMoves property of its options (defaults to true). When true, history will return only the previous moves (ie. its previous behaviour). When false, history will return all previous moves, puts and removes (where each element is an object of shape { historyType: 'move' | 'put' | 'remove', move: Move | Put | Remove } to indicate which type of history entry it is)
- pgn method now generates PGNs with noted put/removes as PGN comments. Although, technically these are still not valid PGNs as per the spec, they make more sense than the previous implementation and will be read properly by loadPgn which will perform the necessary put/removes as the game is loaded.
There are lots of places where eslint annoying thinks something might be null when it definitely isn't (eg. the return from .pop() in a while loop where .length > 0), so it is very useful to be able to tell the linter that it isn't null.
That's certainly another viable approach. I'm not sure which is preferable really. I suppose passing a flag to the constructor would be less flexible but would promote greater consistency.
I've submitted a new PR with only the optimisations (although, the bug fix is a dependency of the I will repurpose this PR for changes to the API methods
Ah that would be a good way to deal with it as it would create no ambiguity at all. But I suppose it is less powerful since clearing the history removes a lot of functionality from the user. I will push some commits to this PR that show how the solution of keeping track of |
gavin-lb
changed the title
isDraw and isGameOver (to fix #112) and fixes for games that include put/remove
|
Latest commits show how |
Changes
isThreefoldRepetition)isFivefoldRepetition,isFiftyMoveRule,isSeventyFiveMoveRuleisDrawandisGameOvermethods to accept an optionalstrictparameter (defaults to false to preserve backwards compatibility). When true, they will strictly adhere to the lastest FIDE rules regarding drawn games (see Article 9 of the Laws of Chess) fixing issue Drawn positions not determined correctly (technically) #112