jordanbray / chess

A rust library to manage chess move generation
https://jordanbray.github.io/chess/
MIT License
234 stars 54 forks source link

Feature request: allow easier board setup beside from_fen() #18

Closed fanzier closed 5 years ago

fanzier commented 5 years ago

When I want to set up a certain position, the only way I can see to do this, is to use Board::from_fen(). However it would be nice to be able to set up a position without creating a FEN string first (since this is slow and tedious). I would suggest a function like the following:

pub fn setup(
    pieces: impl Iterator<Item=(Square, Piece)>,
    side_to_move: Color,
    white_castle: CastleRights,
    black_castle: CastleRights,
    en_passant: Option<Square>)
    -> Option<Board> {}

The implementation could be the same as for Board::from_fen (except for the string handling). Do you agree that this is a good idea? Would you accept a pull request for this?

jordanbray commented 5 years ago

Yeah, I like that idea. I'd want to change the following things though:

  1. Use intoiterator instead of iterator.
  2. Add color to the pieces
  3. For en_passant, use Option instead of Option. The actual square could be computed in the function (because we know the side to move), but it would require less validation and (I think) be more intuitive.
  4. Maybe (?) add constants for each square, like Square::D4. This is probably my biggest headache when using the library for simple testing, and addresses the same concern.
  5. I don't like simply copy-pasting of from_fen(...). Instead, I think from_fen(...) to still do the parsing, but call the new function with the computed fields.

On Thu, May 16, 2019, 7:09 PM Fabian Zaiser notifications@github.com wrote:

When I want to set up a certain position, the only way I can see to do this, is to use Board::from_fen(). However it would be nice to be able to set up a position without creating a FEN string first (since this is slow and tedious). I would suggest a function like the following:

pub fn setup( pieces: impl Iterator<Item=(Square, Piece)>, side_to_move: Color, white_castle: CastleRights, black_castle: CastleRights, en_passant: Option) -> Option {}

The implementation could be the same as for Board::from_fen (except for the string handling). Do you agree that this is a good idea? Would you accept a pull request for this?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jordanbray/chess/issues/18?email_source=notifications&email_token=AADJ5POFMHVOUEVWKFTDMFDPVXSQZA5CNFSM4HNQ2FJ2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GUJE3BQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AADJ5PKZX6TKGZE34YYLX73PVXSQZANCNFSM4HNQ2FJQ .

fanzier commented 5 years ago

I completely agree with your points! (And of course we need the color of a piece, that was a silly mistake by me.) So we could make it the following function:

pub fn setup(
    pieces: impl IntoIterator<Item=(Square, Color, Piece)>,
    side_to_move: Color,
    white_castle: CastleRights,
    black_castle: CastleRights,
    en_passant: Option<File>)
    -> Option<Board> {}

Another thing I noticed that doesn't arise with FENs is that it could happen if the iterator produces the same square more than once. But then we can just return None.

jordanbray commented 5 years ago

Yeah. I thought about that. I even thought you could do a Map<Square, (Color, Piece)>, but it may make the API more difficult to use overall. There's not trait implementation for Map<...> which would mean we'd have to pick a data structure for input. That said, I think either approach is fine.

fanzier commented 5 years ago

I prefer the iterator solution because then one can simply pass a slice, vector or even a (hash) map. However for the map iterator would have item type (Square, (Color, Piece)), so we probably should use that instead.

jordanbray commented 5 years ago

I didn't see any activity on these and I really liked the idea, so I added an Fen structure myself. It seems to handle the use cases described pretty well. I also added Square::{A1-H8} implementations. It will be released with 3.0.3, hopefully tomorrow depending on the amount of free time I have.

There is now an:

None of this is on https://crates.io/crates/chess/, yet. I need to document and test Fen better. I also need to decide if I'm really going to deprecate Board::from_fen(). I kind of think so, mostly because it returns an Option<Board> instead of a Result<Board, Error>, which kind of hurts idiomatic use cases. I also noticed Game::new_from_fen(), but didn't touch it outside of making it use the new structure. It should really return Result<Game, Error> as well, but I obviously didn't want to break the API.

fanzier commented 5 years ago

Thanks for implementing this! I intended to do it but then things came up... I haven't had a chance to test it yet but I really like the new API. :)

I'm just wondering why you want to deprecate Board::from_fen() but not Game::new_from_fen() since they seem quite similar to me. I don't care much about it, I was just wondering. Also, deprecations should happen in a minor release (not just patch release), if you want to adhere to semantic versioning (https://semver.org/#how-should-i-handle-deprecating-functionality). (You are, of course, free not to follow it.)

jordanbray commented 5 years ago

Both are good points. I haven't really decided on either. If I deprecate one, I should deprecate both, and impl TryFrom<Fen> for Game as well.

I didn't know the semver rules on this, and I should follow semver, so maybe I'll bump the version to 3.1.0.

jordanbray commented 5 years ago

This structure was renamed to BoardBuilder and follows the builder pattern. Otherwise, all the functionality is the same. Board and Game now implement FromStr to convert FEN strings directly into them without the need for an intermediate object.

This was released in 3.1.0.