Disable 960 castling in analysis + add 960 in board editor for issue #12926 #13453
Conversation
filters user inputed fen in ctrl.ts to take away castling rights if rooks or kings not on standard starting squares
…ulriverarojas/lila into disable_960_castling_analysis
|
this is a lot of code to add :-/ It looks a bit forced and I'm concerned about long-term maintenance. But maybe with some changes it can be made better? |
| @@ -128,6 +128,11 @@ export default class AnalyseCtrl { | |||
| makeStudy?: typeof makeStudyCtrl, | |||
| ) { | |||
| this.data = opts.data; | |||
| if (opts.data.game.variant.key === 'standard' || opts.data.game.variant.key === 'fromPosition') { | |||
| const new_fen = this.returnStandardCastlingFen(opts.data.game.fen); | |||
There was a problem hiding this comment.
please mind the project naming conventions. new_fen => newFen. More occurences in the pull request.
| if (opts.data.game.variant.key === 'standard' || opts.data.game.variant.key === 'fromPosition') { | ||
| const new_fen = this.returnStandardCastlingFen(opts.data.game.fen); | ||
| opts.data.game.fen = new_fen; | ||
| opts.data.treeParts[0].fen = new_fen; |
There was a problem hiding this comment.
overwriting the fen coming from the server... Maybe it should be fixed on the server side instead, then?
| setup.unmovedRooks = parseCastlingFen(setup.board, castlingPart).unwrap(); | ||
| new_fen = makeFen(setup); | ||
| return new_fen; | ||
| } |
There was a problem hiding this comment.
yikes. That code certainly deserves its own file and tests. Should probably be in chessops too, as you mentioned.
| @@ -33,6 +33,7 @@ export default class EditorCtrl { | |||
| epSquare: Square | undefined; | |||
| remainingChecks: RemainingChecks | undefined; | |||
| rules: Rules; | |||
| variant: string; | |||
There was a problem hiding this comment.
it seems that the unique point of this variant member is to tell whether we have chess960 when rules=chess.
So maybe it should be a boolean?
| return 'Racing Kings'; | ||
| default: | ||
| return paramVariant.charAt(0).toUpperCase() + paramVariant.slice(1); | ||
| } |
There was a problem hiding this comment.
I don't see why we need to re-hardcode all variant names here.
| const option = (e.target as HTMLSelectElement).item( | ||
| (e.target as HTMLSelectElement).selectedIndex, | ||
| ); | ||
| if (option) ctrl.setVariant(option.text.slice(10)); |
There was a problem hiding this comment.
this is verbose and fragile. Instead we could have a ['chess960', 'Chess 960'] option and set the ctrl.rule and ctrl.isChess960 flags accordingly
|
I unfortunately dont have time to make these changes rn, probably for at least a month. If anyone is willing please feel free to, if not I will come back later to implement them. Thanks! |
Implements a fen filterer on analysis page for variants from position and standard to take away castling rights if rooks and kings are not on standard starting squares. Adds 960 to the board editor with the only difference from standard being the redirect link to analysis page including '...chess960/...'. Had to work around chessops lichessRules() method not including 960 to not risk breaking existing functionality but adding 960 to the official rules would be a possible change to consider to cut down on code length here.