search

Disservin / chess-library

C++ chess library
https://disservin.github.io/chess-library/
MIT License
71 stars 22 forks source link

correct pgn parsing #37

Closed robertnurnberg closed 10 months ago

robertnurnberg commented 10 months ago

This should fix all the parsing issues.

I am not 100% if the buffer[i-2] access is always well-defined, even across chunks. So a thorough review is warranted I think.

I bumped the version to 0.5.0, hope that is fine.

Also added a pgn test with black to move first, and with 0-0-0 in it.

Now ./out -ts="PGN StreamParser" works without problems.

Disservin commented 10 months ago

I think this just happens to work... it could be that when we are reading from the bufffer, it's splitted in such a way that 0 is the first thing in the buffer at this point we lost information about the previous tokens, similarly for the end of the buffer we dont know what the next one is (and one would have to make sure that the buffer index is < readbytes). I think the best hack around this would be to "cache/store" the previous 2 tokens.

robertnurnberg commented 10 months ago

Yeah, I was not sure if i was ever zero, apart from reading the beginning of a file. If it can be zero mid-file, then a static buffer would do the trick. Let me know if this is needed.

Btw, do any of these pgn parsing issues also affect fastchess?

Disservin commented 10 months ago

fastchess uses the same parser, so persumably yes. i will be zero multiple times, currently the logic is that, we have a buffer of n bytes, we read from the istream and fill it completly or up to the last byte of the istream. Then we loop over the buffer till we reach the end of the buffer, then we fill the buffer again. Static variables wont work when having a multithreaded app.

robertnurnberg commented 10 months ago

So if i can be zero while reading the moves of a game, then this PR won't work for 0-0 and 0-0-0 moves that are parsed while i switches to 0.

The PR could still be merged, as it deals with more pgns correctly than master. Up to you.

Disservin commented 10 months ago

can you try saving the two previous chars ? default can be just empty i guess and then

char c = buffer[i];
// code 
c2 = c1;
c1 = c;

smth like that?

robertnurnberg commented 10 months ago

Pushed a fix.

Disservin commented 10 months ago

nice, now i am still confused as to why my ci doesnt run...

Disservin commented 10 months ago

Do you mind checking if the actions are enabled in your fork settings and work in your fork?

robertnurnberg commented 10 months ago

no problem, but do you mind guiding me (maybe via dm on discord?)

Disservin commented 10 months ago

Forgot to say: Thanks! ;)