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

vampirc-uci doesn't recognize negative times #16

Closed
vlad902 opened this issue Sep 15, 2020 · 8 comments · Fixed by #18
Closed

vampirc-uci doesn't recognize negative times #16

vlad902 opened this issue Sep 15, 2020 · 8 comments · Fixed by #18
Assignees

Comments

@vlad902
Copy link

vlad902 commented Sep 15, 2020

Cute Chess lets human players go below 0 seconds without flagging and will then send the player's time as negative, e.g. 'go wtime -4061 btime 56826 movestogo 90'. The current parser only accepts positive numbers. To be honest, I'm not sure this is a vampirc-uci bug since by some definition negative numbers really shouldn't show up there, but in practice Cute Chess is probably one of the top chess interfaces so it may be worth supporting. Unfortunately it doesn't seem like there's a very well-defined specification to sort out whether this should be fixed on that parser end, or having Cute Chess do something like what xboard does in this situation (report opponent as having 1ms.)

@vlad902
Copy link
Author

vlad902 commented Sep 15, 2020

Note that according [1] to using the timemargin setting with cutechess-cli to allow going over by some time amount will also allow negative times.

[1] https://groups.google.com/g/lczero/c/hcw-GttDRAY

@MadMatt04
Copy link
Member

Hm, I must admit I did fail to consider this possibility. I guess if CuteChess allows for it, we should at least consider it.

It does present a problem, though. Some versions ago I switched the representation of these values to the Rust standard library's Duration type, which does not allow for "negative" durations:

pub struct Duration { secs: u64, nanos: u32, // Always 0 <= nanos < NANOS_PER_SEC }

So I'm going to have to think about how to support this. It's probably going to have to be an API breaking change.

@MadMatt04
Copy link
Member

The chrono crate allows for negative Duration. We could consider switching to that.

It does introduce another dependency, though. An optional feature is probably not warranted here.

@MadMatt04 MadMatt04 self-assigned this Sep 16, 2020
@vlad902
Copy link
Author

vlad902 commented Sep 16, 2020

You could consider parsing negative value into a Duration::new(0, 0). Strictly speaking, it doesn't reflect the data returned; however, a negative duration is no better than a duration of 0 when you're using it to compute how long you should play for and is easier to test for (and more expected.)

@MadMatt04
Copy link
Member

I feel that just quietly changing the negative value to 0 might prove to be unexpected behaviour in the eyes of the users of the library. In addition to that, I feel that in certain cases knowing how much over the clock someone is could be useful information.

I think I'm gonna switch the representation to chrono::Duration. Seems to be the best possible solution.

This was referenced Sep 16, 2020
@MadMatt04
Copy link
Member

Implemented in #18

@MadMatt04 MadMatt04 linked a pull request Sep 16, 2020 that will close this issue
@vlad902
Copy link
Author

vlad902 commented Sep 16, 2020

I synced against master and verified that this change works in Cute Chess, thanks for the quick fix!

@vlad902 vlad902 closed this as completed Sep 16, 2020
@MadMatt04
Copy link
Member

Now also available as a crate release v0.11.0.

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 a pull request may close this issue.

2 participants