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

Improve UCI position command support #18

Merged
merged 4 commits into from
Aug 4, 2021

Conversation

thearst3rd
Copy link
Contributor

This pull request improves the functionality of the "position" command, enough to get the program running with Cute Chess resolving #7 and #15 (I think?)

Basically, the position command works like:

position <initial position> [moves <moves ...>]

The token "initial position" is either startpos or fen <fen string>. Then, it can optionally say "moves" followed by zero or more moves. I tested this using python-chess's UCI engine interface, as well as Cute Chess on Windows and Linux and it all works as expected.

There could still be more error checking - for example, if you pass in an invalid FEN, or an invalid move list, the uci engine should just ignore it. Here, it will probably just crash the python script. But a real chess GUI should never do that so it's probably acceptable for now

I also made it print less stuff to stderr, since that isn't expected by any chess GUI and it was cluttering up stuff when running with python-chess's UCI interface. I kept the debug info though, using the standard "info" tag (though not following the standard for how you're supposed to fill it out. Not that it really matters for this)

@healeycodes
Copy link
Owner

Hi @thearst3rd!

Thanks for the PR. This looks like a sensible change that improves UCI coverage.

Could you take a look at the failing test please? And add test/s to cover any additional features that this PR adds.

You can run the tests locally with python -m unittest discover test/

Happy to help if you get stuck 👍

@thearst3rd
Copy link
Contributor Author

Hi @healeycodes - I got the tests working again, it was just a matter of trimming away the "info" text which I moved from stderr to stdout :)

I also added some more coverage, specifically testing both position startpos and position fen with moves present and not present. That should basically cover all the cases. I also slightly improved how whitespace insensitivity is handled (again though it shouldn't really matter too much).

Let me know if these tests seem ok to you, or if I'm being too specific :)

Now there is one more thing I was thinking about adding - right now you specify the depth using a command line parameter. I think it would be cool to do one of two things - make it so you can tell it what depth to use using the uci standard go depth N command, though it worries me that a lot of GUIs will default to a depth like 15 or 20, when Andoma gets pretty slow even at depth 5 :) So that might not be a good situation to run into.

The other way I was thinking is to add it as a UCI option. So, when it receives uci it will also print something like

option name Depth type spin default 3 min 1 max 25

then respond to the setoption command to set the depth. That means you can use a slider in chess GUIs to select which depth to use, which is pretty neat.

Let me know if either of those sound interesting to you can I can make another PR :)

@healeycodes
Copy link
Owner

Let me know if these tests seem ok to you, or if I'm being too specific :)

These tests look great 👍

Let's get this merged 🚀

@healeycodes
Copy link
Owner

@thearst3rd I like the UCI option you've described here. I agree that it's best to avoid chess GUIs defaulting to large depths due to the high-level nature/performance of this engine.

I'll play back what I think this feature would look like (to check I understand correctly):

  • Without configuration, the engine will continue to use the default depth of 3
  • The engine will still accept depth via command-line (for ease of CLI-play)
  • The engine will provide a UCI option for chess GUIs to alter the depth

☝️ that all sounds great to me and you should open a PR if you're interested in taking it on :)

@healeycodes healeycodes merged commit 8c1793c into healeycodes:main Aug 4, 2021
@thearst3rd
Copy link
Contributor Author

Yep, that's exactly how it will work! I'll get a PR up probably pretty soon :)

@thearst3rd thearst3rd mentioned this pull request Aug 5, 2021
@thearst3rd thearst3rd deleted the better-uci branch August 5, 2021 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants