fairy-stockfish / Fairy-Stockfish

chess variant engine supporting Xiangqi, Shogi, Janggi, Makruk, S-Chess, Crazyhouse, Bughouse, and many more
https://fairy-stockfish.github.io/
GNU General Public License v3.0
607 stars 189 forks source link

Musketeer Chess #32

Closed gbtami closed 3 years ago

gbtami commented 4 years ago

Dr Zied Haddad asked me about adding his variant to pychess site (or maybe a new server for him), but I think maintaining two *-Stockfish Python binding is the latest thing I want in my life. Not to mention the problems of WASM port needed for client side analysis planned in the future. What do you think about merging musketeer support to Fairy from your Musketeer-Stockfish?

ianfab commented 4 years ago

I briefly thought about that when adding Seirawan chess, but did not thoroughly analyze the feasibility so far, so I will try it now:

All in all I think it is feasible, but definitely quite a lot of work for a single variant. In any case it would be a re-implementation, since the Musketeer-Stockfish implementation can not really be re-used here (or otherwise this project would become unmaintainable).

musketeerchess commented 4 years ago

Hi Fabian have you begun thinking about how to merge the Fairy Stockfish with Musketeer Stockfish?

musketeerchess commented 4 years ago

Dear Fabian I Wonder if you made some progress to merge Musketeer-Stockfish and Fairy-Stockfish.

ianfab commented 4 years ago

I have some experimental code in https://github.com/ianfab/Fairy-Stockfish/tree/musketeer, but this code mainly outlines the idea but is far from a working implementation.

musketeerchess commented 4 years ago

Hi Fabian I see that you are wroking hard on Fairy Stockfish. Hope you succeeded in merging Musketeer and Fairy Stockfish or at least you made porgress. Thanks alot Zied

musketeerchess commented 4 years ago

Hi Fabian Have you made any progress?

Please let me know if i can help in anyway? I'm not a good programmer but i have ideas (of course i'm a french guy)!!

alexobviously commented 3 years ago

I would probably for the beginning not implement all piece types, since I removed the limited-distance slider feature at some point, because it was unused, so it would need to be re-implemented.

Since this needs to come back in if musketeer is going to be added, could you show me how far back in commits I need to go to find your old code for limited sliding, and would it still be easy to just plug it back in or have things changed significantly? If not, I can rewrite it, it'd just make more sense to do it the way you did before.

By the way, I have most things more or less working now - just some loose ends to clean up (like actually implementing all of the pieces, haha)

ianfab commented 3 years ago

The removal of limited distance sliders was quite some time back during the refactoring of piece definitions, see https://github.com/ianfab/Fairy-Stockfish/commit/bb2e88d2a7eef1a7780e346af89cc82a8cd40a99. Unless I am missing something, the only changes that should be required are to consider the limited distance in the initialization of the PseudoMoves/PseudoAttacks tables at https://github.com/ianfab/Fairy-Stockfish/blob/d825bd71ff4ece390d1cfcd4491da97f4591c899/src/bitboard.cpp#L285-L309 and introducing the corresponding new integer value as part of the PieceInfo in order to get the maximum distance for each piece type there.

Implementing the piece types IMO has a relatively low priority, since that should be the easy part and it basically is completely independent of the implementation of the gating mechanism. However, it needs to be monitored how much of an impact the addition of new piece types would have on performance.

alexobviously commented 3 years ago

Thanks, I've implemented this: https://github.com/alexobviously/Fairy-Stockfish/commit/9a2a363f388958d5e906a2194b0040b9285bb2d9. I just made it a single value for quiet and capture moves, and no different values for different directions, because that's all Musketeer requires. I considered a more sophisticated implementation but honestly I've not seen any variants with more complex piece behaviour anyway, so there's probably no use for it.

Implementing the piece types IMO has a relatively low priority, since that should be the easy part and it basically is completely independent of the implementation of the gating mechanism. However, it needs to be monitored how much of an impact the addition of new piece types would have on performance.

Yeah I know, I was just getting it out of the way. I have vague implementations of most things now, I'm just consolidating it all into one place (this project started out kind of confusingly).

I do have a question about this part though: how exactly does pieceToCharTable work? I've looked at the code where it's used but I can't make total sense of it (what are all the full stops for, how to determine how many to put and where, how to determine where to put each piece?)

I have been a bit busier than expected on other projects this week, but I did get something done on gating. Just the data structure and parsing from FEN - I stuck with your proposed format of ao******/rnbqkbnr/pppppppp/8/8/8/8/PPPPPPPP/RNBQKBNR/AO******.

So now for gating I need to change: Position.do_move Position.undo_move Something in move generation that flags a move as gating (we can just use the existing flag I guess?) if there is a committed piece in the relevant position

Are there any other places it should be mentioned you can think of? Something in UCI maybe? I don't think the zobrist hash needs to take it into account, right?

ianfab commented 3 years ago

Having it a single value should be fine. You might just use FILE_MAX as the default, then you do not need the extra logic to check if it is > 0.

The pieceToCharTable is only relevant for play in Winboard/XBoard, and it has no effect on the engine whatsoever, it just tells the GUI which images to use for the pieces. You should find the corresponding list of images in the XBoard documentation.

FEN handling (generation/parsing), move generation/validation, and do/undo_move should be the core part of the implementation. Things like the UCI move string (unnecessary gating piece) or Zobrist hash (potentiial collisions) might not be perfect without changes, but that should not impact basic functionality.

alexobviously commented 3 years ago

Hey, I wonder if you have an idea about the best way of doing something. I just realised that each Move will have to store information about the move being a 'commit gates' move, otherwise undo_move will not work. This could honestly just be a bool because we'd just be moving the piece on the starting square back into the gate. Since Move is just an int, not an object, I can't just add extra variables to it though. I was thinking I could reuse this part: bit 12-13: promotion piece type - 2 (from KNIGHT-2 to QUEEN-2) - if I store the piece that's being gated in here it'd be easy to just copy it back in undo_move. I just worry that this would produce unexpected behaviour. As far as I can see, bits 12-13 are only ever accessed when type_of(m) == PIECE_PROMOTION, and since the gating move is never going to be a promotion, it shouldn't be a problem. Just wanted to check with you first and see if you had any ideas of why that wouldn't work.

By the way, I have not set commit gates up to use drop moves because there's never any choice/user input after the setup phase, so I figured it's conceptually different and it would be confusing and messy to do it that way. (do you agree? - I might be missing something here)

musketeerchess commented 3 years ago

hi When Gating à piece or simply after Piece Selection Phase, if the pieces are not among the usual classic chess pieces, the new pieces join the list of possible pieces that could be promoted.

So basically, the bits 12-13 could be a good “location” to remember the Piece Selection Choice!

If I understood well what you were asking for.

Best regards

Zied

Le 4 nov. 2020 à 12:42, Alex Baker notifications@github.com a écrit :

 Hey, I wonder if you have an idea about the best way of doing something. I just realised that each Move will have to store information about the move being a 'commit gates' move, otherwise undo_move will not work. This could honestly just be a bool because we'd just be moving the piece on the starting square back into the gate. Since Move is just an int, not an object, I can't just add extra variables to it though. I was thinking I could reuse this part: bit 12-13: promotion piece type - 2 (from KNIGHT-2 to QUEEN-2) - if I store the piece that's being gated in here it'd be easy to just copy it back in undo_move. I just worry that this would produce unexpected behaviour. As far as I can see, bits 12-13 are only ever accessed when type_of(m) == PIECE_PROMOTION, and since the gating move is never going to be a promotion, it shouldn't be a problem. Just wanted to check with you first and see if you had any ideas of why that wouldn't work.

By the way, I have not set commit gates up to use drop moves because there's never any choice/user input after the setup phase, so I figured it's conceptually different and it would be confusing and messy to do it that way.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

ianfab commented 3 years ago

@alexobviously Side effects of moves, such as changes in castling or gating rights, should not be encoded in the Move. The Move is an unambiguous description of a move given the current board position and should be sufficient to apply it. If additional information is required to undo the move, this should be stored in the StateInfo object.

alexobviously commented 3 years ago

@alexobviously Side effects of moves, such as changes in castling or gating rights, should not be encoded in the Move. The Move is an unambiguous description of a move given the current board position and should be sufficient to apply it. If additional information is required to undo the move, this should be stored in the StateInfo object.

Okay thanks, I thought you might say something like this and now I understand what StateInfo is supposed to do :)

musketeerchess commented 3 years ago

Hi Alex made a version of Musketeer Chess handled by Fairy Stockfish and working online. Is it possible to review the code and add Musketeer Chess to the PyChess server?

Another request: Maybe have an NNUE Musketeer Stockfish engine to train.

Thanks Zied

ianfab commented 3 years ago

If there is code that is supposed to be reviewed/merged, please create a pull request, then it can be checked. If the code was not updated with my master branch since the start of the implementation, extensive conflict resolution might be needed though since there have been big changes in the meantime. This will get more clear once a merge request is created.

Merging the code will of course be a prerequisite to NNUE training, but there are also still some other open points to be able to support NNUE for variants with many piece types (>6). This generally affects many variants, so I am anyway already working in this direction, but progress is slow.

musketeerchess commented 3 years ago

Hi Fabian This is a message for Alex, add the code of your work to the master branch so you, Fabian and Tamas can work together.

Thanks Zied

Le mer. 2 juin 2021 à 16:44, Fabian Fichter @.***> a écrit :

If there is code that is supposed to be reviewed/merged, please create a pull request, then it can be checked. If the code was not updated with my master branch since the start of the implementation, extensive conflict resolution might be needed though since there have been big changes in the meantime. This will get more clear once a merge request is created.

Merging the code will of course be a prerequisite to NNUE training, but there are also still some other open points to be able to support NNUE for variants with many piece types (>6). This generally affects many variants, so I am anyway already working in this direction, but progress is slow.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ianfab/Fairy-Stockfish/issues/32#issuecomment-853089061, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIIE4HN5OPEZ3LNENABUC23TQY7UHANCNFSM4I27KYXA .

alexobviously commented 3 years ago

If there is code that is supposed to be reviewed/merged, please create a pull request, then it can be checked. If the code was not updated with my master branch since the start of the implementation, extensive conflict resolution might be needed though since there have been big changes in the meantime. This will get more clear once a merge request is created.

Merging the code will of course be a prerequisite to NNUE training, but there are also still some other open points to be able to support NNUE for variants with many piece types (>6). This generally affects many variants, so I am anyway already working in this direction, but progress is slow.

Correct, the two versions are now very far apart from each other. I've been sort of following your updates, and did a quick assessment, and although there have been extensive changes, apparently there are no conflicts. But I'll have to make sure, and it might actually be more sensible to recreate the changes in a new fork. So I'll do that over the next couple of days and I'll find you on discord to go through it before I submit a PR

musketeerchess commented 3 years ago

Hi friends What are the advances in merging both codes?

alexobviously commented 3 years ago

322