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

Musketeer Chess #32

Closed
gbtami opened this issue Sep 26, 2019 · 20 comments
Closed

Musketeer Chess #32

gbtami opened this issue Sep 26, 2019 · 20 comments
Labels
enhancement New feature or request variant Support for additional variant requested won't This will not be worked on

Comments

@gbtami
Copy link
Member

gbtami commented Sep 26, 2019

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 ianfab added the enhancement New feature or request label Sep 27, 2019
@ianfab
Copy link
Member

ianfab commented Sep 27, 2019

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

  • Setup phase
    • This is a completely new move type, so I need to first come up with a possible solution before I can judge whether this is practicable.
  • Musketeer gating
    • This should not be too problematic as it is a subset of S-Chess gating.
  • new piece types
    • some of the piece types are already there (dragon/amazon, chancellor, archbishop)
    • 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.
  • Promotion to gating pieces
    • Limiting the promotion to the gating pieces (and not all new piece types) is tricky, since it needs to be part of the FEN or alternatively might only be available if the history is there, i.e., when the engine receives the full position startpos moves ... command.

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).

@ianfab ianfab added the variant Support for additional variant requested label Oct 14, 2019
@musketeerchess
Copy link

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

@musketeerchess
Copy link

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

@ianfab
Copy link
Member

ianfab commented Mar 27, 2020

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.

@ianfab ianfab added the WIP Work in progress label Mar 27, 2020
@musketeerchess
Copy link

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
Copy link

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
Copy link

alexobviously commented Oct 12, 2020

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
Copy link
Member

ianfab commented Oct 12, 2020

The removal of limited distance sliders was quite some time back during the refactoring of piece definitions, see bb2e88d. 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
Copy link

Thanks, I've implemented this: alexobviously@9a2a363. 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
Copy link
Member

ianfab commented Oct 21, 2020

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.

@ianfab ianfab added this to the next milestone Oct 24, 2020
@alexobviously
Copy link

alexobviously commented Nov 4, 2020

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
Copy link

musketeerchess commented Nov 4, 2020 via email

@ianfab
Copy link
Member

ianfab commented Nov 6, 2020

@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
Copy link

@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 :)

@ianfab ianfab removed this from the next milestone Jan 14, 2021
@ianfab ianfab added this to the next milestone Apr 18, 2021
@musketeerchess
Copy link

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
Copy link
Member

ianfab commented Jun 2, 2021

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
Copy link

musketeerchess commented Jun 2, 2021 via email

@alexobviously
Copy link

alexobviously commented Jun 5, 2021

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
Copy link

Hi friends
What are the advances in merging both codes?

@alexobviously
Copy link

#322

@ianfab ianfab closed this as completed Jun 27, 2021
@fairy-stockfish fairy-stockfish locked and limited conversation to collaborators Jun 27, 2021
@ianfab ianfab added won't This will not be worked on and removed WIP Work in progress labels Jun 28, 2021
@ianfab ianfab removed this from the next milestone Jun 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request variant Support for additional variant requested won't This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants