Produce valid FEN strings for game struct

[WalterSmuts]

Fixes #46 and fixes #36

[jordanbray]

Hey, @WalterSmuts , I'm going to need quite a few changes before I am willing to merge this.

I have several issues.

1) The added fields to Board are not needed. This should have been implemented in Game. 2) There are a lot of changes to the Board struct that seem unrelated. Some stuff with the en_passant square, for example. I'm not sure those are correct. In particular, you no longer validate the ep square, you trust the FEN absolutely. This can cause an invalid board to be generated. 3) You changed to_fourth_rank to return the third rank. This massively breaks the API compatibility.

If you would like to implement this, the better way to do this is by adding a function to the Game structure.

[WalterSmuts]

If you would like to implement this, the better way to do this is by adding a function to the Game structure.

I think you're right :)

There are a lot of changes to the Board struct that seem unrelated. Some stuff with the en_passant square, for example. I'm not sure those are correct.

These changes are to conform with the standard meaning of the en passant square. I.e fixing #36. But I suspect a en_passant_target field is the way to go. See below...

In particular, you no longer validate the ep square, you trust the FEN absolutely. This can cause an invalid board to be generated.

Looks like I removed too much yeah. My intention was just to remove the logic enforcing Only set self.en_passant if the pawn can actually be captured next move.. But as you mention, changing this breaks API compatibility. I guess the way forward here is to introduce a new en_passant_target field with a new public function get_en_passant_target. This duplicates required state a bit but won't break backwards comparability with the way you defined get_en_passant. Would you be okay with duplicating this state? I.e. having two variables in the Board function: en_passant and en_passant_target. The definition of en_passant would stay the same but the en_passant_target would conform to the FEN string definition. Thoughts?

Note: There are two differences between the en_passant and en_passant_target fields:

• The rank they refer to, 3rd vs 4th
• The en_passant_target is always set on a double pawn push while the en_passant field is only set when it matters (i.e. only when it allows for an en passant capture the next move)

[WalterSmuts]

I've gone ahead and factored the PR into smaller commits to make it easier to review. Here are the commit messages:

commit 5cff4b423ddd7f2386bb366f34316902dbef985d
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Tue Mar 9 22:26:09 2021 +0200

    Add test to enforce FEN strings for game

    The test was found as a sample on:
    https://www.chessprogramming.org/Forsyth-Edwards_Notation

    The test exercises special-cases of:
    * En passant moves
    * Full move counter
    * Half move clock

commit 4fe679b813370a15597f41c27c1733dce68572f9
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Fri Jun 4 08:43:56 2021 +0200

    Implement Display trait for Game struct

commit 1d1a79ed29859fab09a3ab0f0f14404d4ecfdc42
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Fri Jun 4 08:41:32 2021 +0200

    Factor board_builders Display trait

    ...into a separate get_pseudo_fen fn. This is in preparation for
    implementing the Display trait on the Game struct. We'll be using the
    get_psuedo_fen logic in the Dispay trait implementation.

commit 06a528f89d5730c3a0bb04782708a39e98fbd9b1
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Fri Jun 4 08:29:28 2021 +0200

    Factor get_half_move_clock logic into separate fn

    In preparation for implementing the Display trait on the Game struct, we
    need to factor out the get_half_move_clock logic to avoid logic
    duplication.

commit d12551c457758af01b2ce9958c552f3bd1257c5a
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Fri Jun 4 08:22:24 2021 +0200

    Add en_passant_target to BoardBuilder struct

    To keep the one to one correspondence between the Board and BoardBuilder
    structs we need to duplicate the en_passant_target definition for the
    board_builder struct.

commit ef6e09b0ec35597e58013aa89ba72157cf514044
Author: Walter Smuts <smuts.walter@gmail.com>
Date:   Fri Jun 4 08:20:48 2021 +0200

    Add en passant target to Board struct
    This is in preparation for implementing the Display trait on the Game
    struct. The en_passant_target field is uesd in standard FEN notation and
    differs from the en_passant field in the following ways:

    * The rank they refer to, 3rd for en_passant vs 4th en_passant_target

    * The en_passant_target is always set on a double pawn push while the
      en_passant field is only set when it matters (i.e. only when it allows
      for an en passant capture the next move)

[jordanbray]

I can already tell this is much better than the original patch, I still have some nit-picking to do, but this is much closer to how the implementations should be.

This is really two MRs, one for the FEN stuff, and one for the EP stuff.

En-passant

I think we can remove en_passant_target from the Board structure. This seems like:

1) Extra data to copy around 2) Duplicate data - the data is already present in the board.

Instead, I think en_passant_target() function should do the computation to get the correct piece. I also believe we can remove the set_en_passant_target() function, as that provides a source of unexpected behavior, as a user must know he needs to set the en_passant in two places.

The same argument can be made for BoardBuilder. There is no reason for the duplicate information.

FEN

I'm missing documentation on the get_pseudo_fen function. I'm also not sure why it exists as its own function, rather than in the impl fmt::Display. It may make sense to have it be its own function for some reason, but I'm not seeing why right now.

In the impl fmt::Display for Game, you can use self.current_position() rather than calling get_pseudo_fen().

Next up, the big one: it appears to me you've only implemented half of the move counter stuff. You seem to more-or-less correctly serialize everything (I haven't tested it myself yet), but you don't correctly _de_serialize the move counter yet. So, reading an FEN per that spec. is currently not supported. This should be implementable by adding a start_half_move and start_full_move integer to the Game structure. I believe you can just initialize your counter in the get_*_move() functions to the correct counter and continue counting from there.

[WalterSmuts]

I think we can remove en_passant_target from the Board structure.

I'm not so sure. There is no context to infer en_passant from only the Board struct. This is because en_passant_target is set on more occasions than en_passant is. From my first commit message:

• The en_passant_target is always set on a double pawn push while the en_passant field is only set when it matters (i.e. only when it allows for an en passant capture the next move)

Meaning there will be occasions when we need to have en_passant_target set when en_passant is not set. So the following will not be possible:

Instead, I think en_passant_target() function should do the computation to get the correct piece.

I'm missing documentation on the get_pseudo_fen function.

I'll add some. Good idea.

I'm also not sure why it exists as its own function, rather than in the impl fmt::Display. It may make sense to have it be its own function for some reason, but I'm not seeing why right now.

Since we're using it in two places, it needs to be factored out. The same logic is used to to implement the Display trait for both the Board and the Game structs.

In the impl fmt::Display for Game, you can use self.current_position() rather than calling get_pseudo_fen().

I'm not 100% sure what you're suggesting here but it sounds like you're suggesting that the Board struct's Display trait implementation should change to produce a psuedo_fen string instead of a FEN string. This may break expectations people have on the behaviour of the display trait for Board and would require a major version bump. If this is the way you'd prefer to go then the documentation for Board would need to be updated. Let me know.

Next up, the big one: it appears to me you've only implemented half of the move counter stuff. You seem to more-or-less correctly serialize everything (I haven't tested it myself yet), but you don't correctly _de_serialize the move counter yet. So, reading an FEN per that spec. is currently not supported. This should be implementable by adding a start_half_move and start_fullmove integer to the Game structure. I believe you can just initialize your counter in the get*_move() functions to the correct counter and continue counting from there

Interesting idea. Let me see if it works :)

[WalterSmuts]

Made the get_pseudo_fen function crate local and implemented proper deserialization and updated the test to enforce that.

[WalterSmuts]

Bump

[WalterSmuts]

Bump

[jimmyhmiller]

Just wanted to say, I had to use @WalterSmuts fork with this change in order to get my app working. I've built a personal app that connects a UCI bot to my chessnut air. I was running into issues with not being able to play en passant and just using this fork fixed the issue. No code changes need on my part.

I was pretty surprised to find that the chess crate was producing invalid fen.

If there is something hold back this from being merged, I'd definitely be happy to help fix it to prevent people from running into the same issue I did.

[petersn]

I had to do the same thing, @jimmyhmiller. Thanks @WalterSmuts.

For beginners finding this issue who might not know what to do, you can just switch to

[dependencies]
chess = { git = "https://github.com/WalterSmuts/chess" }

in your Cargo.toml to at least get non-broken FEN behavior.