Contains atypical PGN parser, difficult to debug

[ChaoticNeutralCzech]

The following PGN was exported from Lichess.org (line breaks added by me to highlight moves 42 and 54):

1. Nf3 Nc6 2. Nc3 Nf6 3. Rb1 Rg8 4. Rg1 Rb8 5. Rh1 Ra8 6. Ra1 Rh8 7. Nb1 Ng8 8. Ng1 Nb8 9. Nf3 Nc6 10. Nh4 Na5 11. b4 g5 12. bxa5 gxh4 13. Ba3 Bh6 14. Bd6 Be3 15. fxe3 cxd6 16. h3 a6 17. Nc3 Nf6 18. Nb5 Ng4 19. hxg4 axb5 20. Rb1 Rg8 21. Rb4 Rg5 22. Rf4 Rc5 23. d4 e5 24. dxc5 exf4 25. Qd2 Qc7 26. Kd1 Kf8 27. Kc1 Kg8 28. Kb1 Kh8 29. Ka1 h6 30. a4 h5 31. Qe1 Qd8 32. a6 h3 33. a7 h2 34. Rg1 Rb8 35. axb8=Q hxg1=Q 36. Qxc8 Qxf1 37. Qb1 Qg8 38. a5 h4 39. a6 h3 40. a7 h2 41. a8=Q h1=Q 
42. Qa2 Qh7 
43. e4 d5 44. e5 d4 45. e4 d5 46. e6 d3 47. e7 d2 48. c6 f3 49. e5 d4 50. c4 f5 51. c5 f4 52. Qf5 Qfc4 53. Qfc2 Qcf7 
54. Qbc1 Qgf8 
55. Kb2 Kg7 56. Ka3 Kh6 57. e8=Q d1=Q 58. Qa8 Qh1 59. Qaa4 Qhh5 60. e6 d3 61. e7 d2 62. e8=Q d1=Q 63. Qea8 Qdh1 64. Kb4 Kg5 65. Kc3 Kf6 66. Kd2 Ke7 67. g5 b4 68. g4 b5 69. g6 b3 70. g5 b4 71. g7 b2 72. g8=Q b1=Q 73. Qgg6 Qbb3 74. Qgd3 Qbe6 75. g6 b3 76. g7 b2 77. g8=Q b1=Q 78. Qda3 Qeh6 79. Qgg6 Qbb3 80. Qgd3 Qbe6 81. c7 f2 82. c8=Q f1=Q 83. Qca6 Qfh3 84. Kc3 Kf6 85. Kb2 Kg7 86. Ka1 Kh8 87. Qc2b1 Qf7g8 88. Qcb2+ Qfg7 89. c6 f3 90. c7 f2 91. Qdb3 Qeg6 92. Qaf1 Qhc8 93. Qfd1 Qce8 94. Qad5 Qhe4 95. c8=Q f1=Q 96. Qcc1 Qff8 97. Qdc5 Qef4 98. Q5c2 Q4f7 99. Qbd4 Qge5 100. Qdb2 Qeg7 101. Qbd4 Qge5 102. Qdb2 Qeg7 1/2-1/2

When run with this input file, your code outputs no GIF and the exception handler gives the following, rather cryptic message:

Traceback (most recent call last):
  File "C:\path-to-pgn2gif\pgn2gif-script.py", line 33, in <module>
    sys.exit(load_entry_point('pgn2gif==1.2', 'console_scripts', 'pgn2gif')())
  File "C:\path-to-python\python3.8\site-packages\pgn2gif-1.2-py3.8.egg\pgn2gif\pgn2gif.py", line 167, in main
    creator.create_gif(pgn, Path(args.out) / f)
  File "C:\path-to-python\python3.8\site-packages\pgn2gif-1.2-py3.8.egg\pgn2gif\pgn2gif.py", line 127, in create_gif
    game.next()
  File "C:\path-to-python\python3.8\site-packages\pgn2gif-1.2-py3.8.egg\pgn2gif\chess.py", line 228, in next
    origin = self._find_non_pawn(move, dest, pt)
  File "C:\path-to-python\python3.8\site-packages\pgn2gif-1.2-py3.8.egg\pgn2gif\chess.py", line 147, in _find_non_pawn
    return next(
StopIteration

By shortening the file using trial-and error, I determined that the crash happens at Black's move 54. If Qgf8 and all following text is removed, a GIF is generated. However, unlike the output of all online PGN players and converters I tried, it does not end with a black queen in column g (there should have been one at G8). Turns out, your program moved the G8 queen in move 42, "42. .. Qh7". Either queen (G8 or H1) could move there, and it was not specified. This ambiguity is likely a mistake on Lichess' part, and I am not blaming you for handling it wrong. However, it is annoying that your PGN parser behaved differently from all others tested. The game can be fixed by rewriting Move 42 to 42. Qa2 Qhh7 to remove the ambiguity.

For reference, here is the chessboard state after White's move 42 (just before the parsing "mistake") and White's move 54 (just before the crash), respectively:

  a b c d e f g h  |    a b c d e f g h
8     Q       q k  |  8             q k
7   p   p     P    |  7   p     P q   q
6       p          |  6     P          
5   p P            |  5   p P   P      
4           p P    |  4       p   p P  
2         P        |  3           p   
2 Q   P   P   P    |  2 Q   Q p     P  
1 K Q       q   q  |  1 K   Q          

Also, FEN of the above positions for import into a viewer:

2Q3qk/1p1p1p2/3p4/1pP5/5pP1/4P3/Q1P1P1P1/KQ3q1q b - - 1 42
6qk/1p2Pq1q/2P5/1pP1P3/3p1pP1/5p2/Q1Qp2P1/K1Q5 b - - 5 54

So, how could you improve the user experence? In either of two ways:

• Use a PGN parser that behaves the same as others, such as the Lichess.org Analysis Board Import feature. Thus, your program will behave as expected from the users of Lichess.org and other parsers: the set of accepted PGN files will be the same for you and them, and they might offer hints on error locations in case the file cannot be accepted. You could possibly import someone's open-source pre-converter that completely de-ambiguises the PGN, so that e.g. 1. e4 Nf6 becomes 1. e2e4 Ng8f6 (if such software exists).
• Add error reporting that aids in identifying the ambiguous move, so that the user can fix the PGN file.

Such error reporting could look like this:

Ambiguous white move 42 "Qa2": Multiple queens can move to A2; resolved as A8→A2
Ambiguous black move 42 "Qh7": Multiple queens can move to H7; resolved as G8→H7
Invalid black move 54 "Qgh7": No queen in column g
Please retry with a valid PGN file. Exiting...

Note that in Move 42, White's ambiguous move was resolved as expected while Black's wasn't.

[dn1z]

Hi, firstly thanks for the detailed issue description. I had a look at the game and in 42nd move, as you mentioned, while queens on both g8 and h1 can move to h7, due to the fact that queen on the g8 is pinned by white queen in c8, Qh7 is apparently the right way to show the move. However, there is no problem with PGN parser itself. The parser only parses the list of moves using regex and feeds the ChessGame object with these moves which creates the board state by applying the move by finding the original square where the piece in the move came from. To find the square where the piece came from (e.g. in the beginning, in the move Nf3, knight comes from g1 square), the algorithm checks pieces one by one (starting from 8th row and ending in 1st row which explains why it finds original square of the move Qh7 as g8) and when it finds a valid movement, it returns the square without further check in which your issue lies. In order to solve this issue, i should modify the original-square-finding algorithm to check for all pieces even it already found a match and when the program encounters with ambiguous move, it should try to invalidate all but one move (like Qgh7 is not possible as the queen is pinned). The ChessGame object does not implement all of the chess features, it only tries to get board state, and understands nothing else like king checks, checkmates, etc., so I may try to implement some of these features or use existing chess library for python. I will have look into this issue deeper and make necessary changes soon.

[ChaoticNeutralCzech]

Thank you for looking into the issue! Sadly, I won't be of much help anymore: I have read none of the repo's code and I'm helpless at OOP anyway, and as you may have guessed from the game, my (White's) strategic intelligence is at the level of a 9-year-old who just learnt how promotion works and what the word "harem" means. (Still better than Black though, who is just a dumb copycat.) So I didn't even realize that the queen was pinned - I only know chess rules, no strategy.

Due to lacking programming experience and English being my second language, I may have misused the word "parser" in place of "interpreter" or another more suitable denomination (perhaps the appropriate function has a name in the code but I haven't read it, sorry).

By the way, this project is awesome for its purpose, and though the quality could have been better than 480p, I won't complain. For this reason, I looked no further than here for the export of my games, though the option for the duration of individual frames seems like a missing basic feature (fortunately, GIF editors make up for it). What I find best about this SW is its good use of the redefinable 256-color palette (unlike Chess.com's GIF creator, which even dithers colors of the board!) (Well, this should not be a surprise when you compare someone's love project on Github to a corporation's proprietary code they wanted to get done ASAP.)

Cheers!