PGN Export

[addcninblue]

Hi!

Thanks for the wonderful chess program.

I was wondering how much work it would be to be able to export games to PGN after they're done (or perhaps continuously during the game)? Perhaps this can be implemented in savestates.yml, with a new line like slot_1_pgn or something. If it's not too much work but is nontrivial, I'll have time over the summer to see if I can try implementing it. Pointers would be appreciated!

Thanks again!

[LinusCDE]

Hi,

I'm not exactly sure how pgn worked, but I think it was like recording which piece moved where (kinda like a diff). This should be pretty to find out when comparing 2 states of a board or just taking the move action.

I'm currently pretty busy as well so I might or might not have time to do this over the coming days.

Here are some pointers:

• src/bin/chessmarkable/scene/game_scene.rs only contains the ui stuff and is pretty messy
• The game logic was separated out into the files related to the lib part of the crate (in src/).
• The UI interacts with game.rs using proto.rs in the same folder. It was refactored this way to make adding multiplayer easier, but I never got to that.
• The main logic should be in src/game.rs

You can probably hook the move_piece methods to get the individual moves. These could be recorded in a vec of the same struct and then maybe later offered as export or auto-saved once the game has an outcome (aka it ended).

Otherwise you should be able to get a FEN of the board pretty much everywhere. You can use it to create a pleco::Board and maybe then diff it manually to get a move.

@rmadhwal Did the pgn viewer and can probably help better regarding the format or whether the lib can also be used to encode a game as a pgn.

Edit: Fixed wrong file linked in pointers

[LinusCDE]

The game also logs the fen of each board change by default. So if you wish to make a first hacky attempt at generating a pgn based on individual FENs, you can extract them from the logs of your launcher (e.g. journalctl -fu tarnish, journalctl -fu remux or running the command chessmarkable over ssh).

Maybe there are already same "Fen to Pgn converters" that could make this automatically.

[rmadhwal]

Maybe there are already same "Fen to Pgn converters" that could make this automatically.

FEN only gives you state of the current board while PGN is a historic overview of how you got to that state. So a FEN is essentially a subset of a PGN and you've lost some information if you only have the former. For example, there could be multiple ways to get to the same FEN and all those PGNs would represent different games which might not be too helpful.

You can probably hook the move_piece methods to get the individual moves. These could be recorded in a vec of the same struct and then maybe later offered as export or auto-saved once the game has an outcome (aka it ended).

Currently we allow the pleco board to be created via a FEN, so this change might not be sufficient unless we also store PGN and recreate the board with both FEN and PGN, but in that case we might as well only use the PGN.

Hence, I think we should extend pleco's board to be PGN based along with FEN based and only used the PGN based version for our code. That way when the game is over we can just generate the PGN from the board itself, it also seems more intuitive to me that the underlying board remembers the history of moves.

This change will probably be non-trivial though. Storing a state of moves internally like @LinusCDE suggested would probably be easier and involve less tinkering. (We could change save state to include the PGN and load that into the internal representation in game.rs and continue with the FEN based representation in Pleco, it just seems sorta hacky to me)

Also they currently have an open issue with help wanted for something similar pleco-rs/Pleco#71 but this probably doesn't include changing internal implementation.

[addcninblue]

I figured out most of it; went with storing the state of moves.

Quick question: why does game_scene handle reading and writing savestate files? Why is it not handled in game.rs and proto.rs (which seems to be the "server" side of this server-client relationship)?

At the moment, the issue I'm running into is that

1. I implemented the state of moves in game.rs, but I need this state to serialize to savestate
2. I can get this by sending it on every move (adding a pgn tag everywhere we see fen in the protos), but
3. This gives us quadratic time, since pgn increases in length every step.

Any thoughts would be greatly appreciated. Thanks!

Edit:

You can see my changes here: https://github.com/addcninblue/chessmarkable/tree/feature/implement-pgn
Diff: https://github.com/addcninblue/chessmarkable/compare/master...addcninblue:feature/implement-pgn?expand=1 (I went ahead and rust formatted everything in one commit)

[rmadhwal]

Quick question: why does game_scene handle reading and writing savestate files? Why is it not handled in game.rs and proto.rs (which seems to be the "server" side of this server-client relationship)?

I didn't write this code, so I'm not entirely sure why it is this way, but I don't think you should think of them as a server-client, they just seem to have a different set of responsibilities and in this case, since both of them contained a board (which theoretically should be able to tell us what state the game is at), the responsibility of saving the game seems to have been delegated to the game_scene because the save state is also triggered from there.

Doing it the other way I suggested would have circumented this problem since the board would be aware of the current PGN.

I can get this by sending it on every move (adding a pgn tag everywhere we see fen in the protos)

Why would you need to send the entire PGN on every move ? Wouldn't just the current move's notation be enough ? game_scene can keep track of moves that have been played so far.

[rmadhwal]

On second thought you can diff the FEN between moves so you don't even need to send a PGN

[LinusCDE]

Quick question: why does game_scene handle reading and writing savestate files? Why is it not handled in game.rs and proto.rs (which seems to be the "server" side of this server-client relationship)?

Originally, everything was in the game_scene: drawing, game logic, saving, ...

I split this at some point to separate the responsibilities and make game_scene more readable.

At that time, I also wanted to add multiplayer, this is why communication is done with messages. The fen is used as a means to serialize the board and send it to clients. The fact that the game_scene (basically a client and the ui) is receiving the fen twice I because it is playing both white and black (at least in PvP). In true multiplayer, messages would get (de)-serialized and white and black could be different clients.

For the same reason I also implemented the savestate in game_scene and not the server, since I wanted to keep the lib portable to make integration in a server as easy and single-responsibility as possible (had actix-web with websockets in mind back then).

@rmadhwal already elaborated on most. Thinks I can probably add:

• The pleco board seems to have an internal history. Currently the client overrides it board with each update, thus never having any, but the server-board has it (used for undo's).
• Maybe we can export the pleco board history (BitMoves??) and convert them to pgn? Then we could probably add a "PgnRequest" and "PgnResponse" packet to get a pgn from the server. Maybe then do a request on "Save & Quit" or with a dedicated button and append it to some pgn archive so it can be loaded from the pgn viewer.
  • Or even entirely just using bit move updates instead of sending the entire board each time (that way the client could convert each move to pgn and append it internally).

I'm not sure what to specifically watch out for when sending moves (like how e.g. castling is communicated), but if that works well, we could probably use "full updates" (fen) and "partial updates" (pgn part/move) from the server. Maybe that would also make a pgn impl easier.

[rmadhwal]

Maybe we can export the pleco board history (BitMoves??) and convert them to pgn?

I thought about this when the change was suggested, but I don't think this would be possible. Not because I looked into the code, but because a pleco board can be created purely with FEN (for example, when loading from a saved state). Given this, I doubt that it stores a history of moves (or even if it does, it's possible this is not an authoritative copy).

Unless you're suggesting extending the pleco board to do so.

[LinusCDE]

I thought about this when the change was suggested, but I don't think this would be possible. Not because I looked into the code, but because a pleco board can be created purely with FEN (for example, when loading from a saved state). Given this, I doubt that it stores a history of moves (or even if it does, it's possible this is not an authoritative copy).

You're correct about that problem. When starting a game from scratch the "server" will have the complete board with the history, but the "client" (game_scene) usually just re-creates the board on each change from the new fen (thus never having any history).

If the game is continued from a fen, the server will also not have any history prior to game start. This is easy to notice when undo's don't work. Not sure if the history can be queries or is pub, but that may make it possible to check if the board is authoritative.

We could solve that by simply also allowing a savestate as pgn and defaulting to that on newer version. But for that we must be able to import/export a pleco board from pgn reliably. For people loading a fen, we could then just detect the "non authoritative board" and forbid saving a pgn as it would be broken/incomplete.

[addcninblue]

Got it, thanks all.

I'm leaning towards replacing savestate with just PGN. I'm curious about the usecase for FENs, since it seems like that's only good for analyzing next moves (typically with puzzles, etc).

[LinusCDE]

I would be also for it. That way we also keep the history in addition to the state.

We could probably just use a pgn where we save a fen now. When loading we can fall back to fen for older save states but in time people would automatically migrate to pgn savestates for new games.

[addcninblue]

While implementing this, I realized that it seems that the UI is handling events twice during PVP: once for white and once for black. Can we just drop all events for e.g. black and handle them once with white?

snipe edit: ah, it appears that some messages wouldn't work (eg. possiblemoves). nvm.

[LinusCDE]

I think I mentioned that above already a bit. That is because of the intention of a server-client-model. game_scene is probably more client, client, ui in this case.

I'm generally for it, but am not sure if this would cause problems with the bot mode (the bot is more like a remote player for the UI) and have a kinda broken server. Maybe adding this to the protocol would make sense, like the server not sending some info or the client just ignoring most for one player?

As you can see I'm not sure either.

[addcninblue]

On implementation, I noticed that communication is done through sending Squares instead of BitMoves. I assume this is to allow for future portability?

Conversion to bitmoves lets us reduce on a lot of logic (like iterating through all possible moves to figure out which was the intended move every time, etc). It also makes the PGN logic much simpler, as it would just be a list of bitmoves (which can be converted into pgn trivially).

I tried converting everything to bitmoves to see where that got me, but I haven't finished yet. You can see below where I'm at below.

I wanted to get some thoughts on this before I go further into this approach. It seems that, in retrospect, it would be possible to get the pgn converted from a list of historical board moves, but I think this approach seems marginally cleaner (at the expense of portability with other languages).

https://github.com/addcninblue/chessmarkable/compare/master...addcninblue:refactor/use-bitmove-instead-of-fen?expand=1

[rmadhwal]

I think it's a good idea and while I have some concerns about the implementations (most notably in getting rid of ChessUpdate.PlayerSwitch entirely), @LinusCDE is an expert on their own code and would be better placed in suggesting changes/okaying it.

Though I think it's useful to note that in game.last_move(), you're simply using self.board.last_move() and in game.total_moves(), we currently use self.board.moves_played(). As mentioned before, if you construct a pleco board from FEN (for example, loading a saved game), these will not have the information you'd expect them to have, so you'll have to change these implementations to use information from the saved PGN (if one exists).

[LinusCDE]

Tbh, I wrote it 2 years ago, so I'm not really an expert on it anymore as well.

Since I never got to actually do multiplayer, the protocol layer is just unneccesary complexity for no reason tbh. I'm not sure if ditching the message queues could cause problems with waiting on bots without blocking (especially on hard, a bot can take about a minute), but there are certainly redundant types that could be removed as serialization would not need to be taken into account.

@rmadhwal Regarding ChessUpdate.PlayerSwitch, I'm not really sure what that meant (currently only on my phone), but as long as the game still works fine (or even gets less complex) I'm okay with that.

In general, removing the protocol layer would probably be a good thing to reduce overall complexity. I still believe that having some separation between UI and game logic would be benifitial, but this could be exercised more directly when the UI e.g. has a instance of ChessGame and can then for example directly use self.game.board instead of having to talk over messages and synchronizing a separate copy of it.

I'm currently pretty deep into sorting out what exact topics to do for my bachelor, so I'll most likely not find the time doing a bih refactor.

---

Btw @rmadhwal , would you mind if I promote you to being collaborator on this project? I haven't really done any meaningful things over the last years, and chess was not ever really a hobby of mine tbh. You seem to be for deeper into chess than me and me having to okay things would only slow things down imo.

[rmadhwal]

@rmadhwal Regarding ChessUpdate.PlayerSwitch, I'm not really sure what that meant (currently only on my phone), but as long as the game still works fine (or even gets less complex) I'm okay with that.

From what I understand ChessUpdate.PlayerSwitch seems to serve two purposes: 1) Indicating a game has started and 2) indicating a player can make a new move (arguably 1 is a subset for 2, but while 2 can be replaced by simply indicating a move has been made, 1 can't and seems like we'll need something like ChessUpdate.GameStarted to replace it).

I don't think it's a bad idea to replace it, I think more consideration might be required to see how to do that exactly.

I think having a communication layer is a good idea, because eventually it should indeed support multiple devices, having said that I don't think @addcninblue's change would affect portability since all existing information is preserved.

I also am not 100% about making ChessUpdate.Board a prerequisite for ChessUpdate.PlayerMoved since it's possible for messages to arrive out of order and hence ending up with a board that isn't quite in the state we expect it to be. In theory this can be circumvented by only triggering ChessUpdate.PlayerMoved once ChessUpdate.Board has been received, but a lack of testing makes this hard to enforce.

Btw @rmadhwal , would you mind if I promote you to being collaborator on this project?

Sure, that would be fine with me!

[LinusCDE]

Edit: Btw, this message was out of order again. Didn't the the above response then writing this.

Sorry I didn't noticed your reply before @addcninblue (my phone only showed me the last one but I noticed this when going through my K-9 Mail client).

On implementation, I noticed that communication is done through sending Squares instead of BitMoves. I assume this is to allow for future portability?

One reason was probably the easy to serialize it. But I think I never really understood/looked into the BitMove, so I probably just wanted to let pleco deal with things like castling in case I would overlook any similar special cases.

Conversion to bitmoves lets us reduce on a lot of logic (like iterating through all possible moves to figure out which was the intended move every time, etc). It also makes the PGN logic much simpler, as it would just be a list of bitmoves (which can be converted into pgn trivially).

I tried converting everything to bitmoves to see where that got me, but I haven't finished yet. You can see below where I'm at below.

Seems reasonable. The code seems nice imo. I didn't get to test it yet, but as long as the game works in the end, I'm for it as well.

When disregarding serializability, it would hopefully also make things easier as custom types are not a requirement anymore then.

[LinusCDE]

I also am not 100% about making ChessUpdate.Board a prerequisite for ChessUpdate.PlayerMoved since it's possible for messages to arrive out of order and hence ending up with a board that isn't quite in the state we expect it to be. In theory this can be circumvented by only triggering ChessUpdate.PlayerMoved once ChessUpdate.Board has been received, but a lack of testing makes this hard to enforce.

Yeah, I believe my approach was to always send the absolute state. This makes remote communication and picking up a paused game a lot easier. I also didn't know about PGN back then.

Having the proto only has the advantage of having easy integration possibilities with multiplayer. Technically that should already work if the senders were to just serialize to/from json with serde_json and one would be the server (or totally separate). The missing parts would be session handling and UI for offering that. The huge disadvantage is the added complexity as seen for this project.

I'm personally up to anything. I started the refactor with the game.rs and proto.rs with the intention to make it multiplayer at some point but never got to that. If you still think this is a thing that should/can be added, it can probably be kept. Otherwise at least starting to disregard the need for sent objects to be (de)serializable with serde would make things a lot easier when adding features for now (with the end goal of probably removing it completely at some point). Feel free to decide how you wish to proceed @rmadhwal .

Sure, that would be fine with me!

Done. Welcome to the project as a collaborator! 👏

I'm still confused how people put up with this messy code base. I'm truly humbled.

[rmadhwal]

Done. Welcome to the project as a collaborator! 👏

Thanks! Also the code base is pretty decent :)

Feel free to decide how you wish to proceed @rmadhwal .

I would prefer to not add too much extra complexity (especially as we don't have sturdy tests). @addcninblue unless you have a better suggestion to resolve the ordering conflict mentioned above, would it be okay if we kept both the FEN (along with the PlayerSwitch) and BitMove for now ? We can look at getting rid of the FEN in a future change (with more consideration for where we want the codebase to be in terms of communication etc)

[LinusCDE]

Regarding the tests, I'm on that rn. For now I guess just basic cargo check. Also found a nice tool called audit-check. Will probably update some dependencies to get rid of the security problems.

If I find the time, having automated ipkg packaging as in oxide would be nice as well. Will see if I get to that today as well. A simple binary should be enough for now.

[LinusCDE]

Should be done. Apologies for commit spam on the master. Wanted to get this done fast and not drag along for a while. Also a bit rusty on GH Actions and had to test a bit.

While updating, I also updated some dependencies. I hope the changes are minimal, but there might be some small merge conflicts due to that.

[addcninblue]

@rmadhwal I'm not entirely sure there would be an ordering conflict? If the protos are delivered via something like TCP, this would never occur, right?

Nevertheless, the states can get desynced if they disconnect. The idea I had in mind to resolve any conflicts is for the client to

1. Check the consistency of their state when they receive a new player action, and either
2. Accept the action (if their state is inconsistent), or
3. Reject the action, and ask for the full history of the game from the server (in which the new action will be contained).

I think under this scheme, sending the FEN each time won't be necessary.

[rmadhwal]

@rmadhwal I'm not entirely sure there would be an ordering conflict? If the protos are delivered via something like TCP, this would never occur, right?

Asynchronous message passing in general makes no guarantees about the order of message delivery, they (sometimes) guarantee that messages will be delivered not necessarily in what order. To guarantee order you'd have to use either a synchronous system or some sort of synchronizer like an acknowledgement based method. TCP only preserves orders of packets in a single connection, there are no guarantees across multiple connections (which may happen with parallelised connections). Though I agree that it's highly unlikely for this to occur in practice with most modern TCP libraries, I still think it would be better to stick with caution and not having hidden dependencies between updates intuitively seems like good practice to me.

Check the consistency of their state when they receive a new player action, and either

That's reasonable. Though how would you check for consistency in the create_bot function in proto.rs ? For example, if a ChessUpdate::Board message arrives after the player move update. From what I can see, there is no way to tell that the board in its current state is "inconsistent"

[LinusCDE]

As far as the bot concerned, that only cares that it has the current board state and knows which player it is moving.

I think so far we only need to worry about "Continuation". Meaning either a potential client (re)connect or starting from a past partial game. When we then just bulk send the partial pgn/total moves a client should be up to date.

I think we can assume that any protocol during the game would have all messages arriving guaranteed and in order. Otherwise the channel/connection would fail and the reconnect would do the rest.

Async in rusts context only means "async execution". The channels are not handling messages in async order or akin to udp because of that.

[rmadhwal]

Hmm I see okay, since the communication logic hadn’t been implemented I was just assuming a normal distributed network with async communication. If the order is guaranteed then that’s cool and I have no reservations against this approach.