altarchess / ristinolla

RistiNolla

A m,n,k engine in C++.

Releases

RistiNolla 1.0

Links to documentation

Design document

Implementation document

Testing document

Self play testing

User guide

Hour reporting

Weekly reports

• Weekly report #1
• Weekly report #2
• Weekly report #3
• Weekly report #4
• Weekly report #5
• Weekly report #6

ristinolla's Issues

Code Review 1

Hello @altarchess!
Now a disclaimer right up front: I've maybe read 5 lines of C++ before this review, so I can't go into immense detail on your implementation, but anyways, here's my shot at a code review for your Tic-Tac-Toe -engine:

Environment

With small changes to
GUI/gui/config.cfg: change player_0 path to ../Engine/RistiNolla
GUI/gui/config.py: change "\config.cfg" to "/config.cfg"
I got your project running on Linux!

• Linux, Zorin OS (Ubuntu clone)
• Date and time of download: 5.10. 14:13
• Python 3.10.7

I tested 2 versions of your program, the latest commit (4caf64c) and the last commit for the 4th week (093cd0f).

Usability

What did I do?

• Clone project
• Build the Engine executable
• Install python dependencies for the GUI
• Fix compatibility errors
• Play a bit against the AI with the default settings
• Change the Board config to size 4x4 and max_depth to 6
  • Here the AI got stuck on their second move
• Play around with different configs and comparing the performance with different max_depth values
• Start reviewing code

Experience

The engine seems to be working well with this config:

• size_x: 12
• size_y: 12
• max_depth: 6

After a bit of playing though, the AI freezes up: I get tons of "Failure to print bestmove!" errors on the console.
It also seems possible to play for the AI if you click fast enough on the GUI.
With small boards the AI doesn't really know what to do on their first/second move.

Code

• The project structure looks really good to me. I especially love how the GUI is completely separated from the Engine itself.
• Looking at the actual search -algorithm:
  • It's interesting how it's implemented as a negaMax with makeMove & undoMove
    • Not sure if it's more optimal to just reserve more memory with instances of the board as opposed to "undoing" the move after calling negaMax. (This is a tiny detail though)
  • I think the search could be optimized by moving the board evaluation out from the makeMove function (evalChange), because right now it evaluates the board after every move at every depth, but it only needs to be evaluated at the leaves of the game tree.
• More documentation comments would help
  • Especially in the comms.cpp file: clarify the IO structure and more docs on the individual blocks of code
• The board class looks really good and optimized.
• I recommend implementing unit tests and performance tests asap: unit tests for making sure your code works as inteded and performance tests for monitoring engine performance during optimization.
• The GUI code also looks clean and well structured.

Summary

Your project needs testing, more documentation and there is still some room optimization (it seems you're already working on transposition tables). Also, the AI freezes I experienced could just be because the project is under construction or because of my environment.

Overall, the project looks really nice. Good job!

Code review 2

Hey! Cool project. Didn't expect to see someone casually using prefetch(), but you clearly have a bit of a history with optimizing AI opponents...

The project was downloaded at 7:35 PM @ October 15th and tested on fuksilaptop 2021 (running Ubuntu 20.04).

I started by reading the design document, after which I read the user guide and built the engine & GUI and started the game. Aside from having to install a newer Python (not your problem), build process was smooth.

Either I found a difficult combo and it hung, or a board this big just takes a long while, but it didn't respond even after at least a minute of waiting with these inputs. Tested 5x5 grid as well (because previous reviewer had issues with small boards) and that definitely works, plenty fast too, and I was not able to beat the AI. Also, it starts spamming the console once there are no moves to make, but I assume this is known.

To the code: I have some C++ background, so I was hoping to be able to give feedback here, but aside from pedantic suggestions I wasn't actually able to find much to nag about, ...

... so here's some pedantry...

... and I'm sure there are counterarguments to many of them so take them as you will!

• Generally, constexpr variables are preferred over #defines, unless you necessarily want to have them configurable at compile-time without a fancier system...
• std::from_chars is preferred over stoi, because stoi is technically locale-dependent and possibly slower.
• Prefixing C functions (such as stoi) with std:: is encouraged since they are technically not guaranteed to exist without the std:: prefix, even if in practice they do on the major compilers (GCC, Clang, MSVC).
• Some allocations could be avoided with the use of std::string_view over std::string - for example the split() function could operate entirely on string views. Of course, in this case performance literally does not matter, so the effort is probably not worth it.
• std::vector::at() does bounds checking, which isn't necessary for example here in comms.cpp.
• Functions that aren't meant to be called from outside a specific .cpp file shouldn't be declared in a header file; for example processCommand seems like it should be internal to comms.cpp. Prefer forward-declaring it there if necessary, and make it static to ensure internal linkage
• board.h has #defines before header guards
• You have a fair bit of global state. Since this is single-threaded, it's not much of an issue, but generally not having global state is preferred where possible

The code is fairly C-like at a times, but in this case I see zero issues with that. Memory is not being manually managed and C++ is used to make life easier as appropriate, and everything does look to be (mostly) valid C++, unlike a lot of C-like-C++ codebases I've seen 😅

Code quality wise my main concern would be primarily the lack of testing, but also the naming: I'd appreciate either unshortened names or comments explaining the abbreviations somewhere (what's "comms" short for, or especially "tt"?). Variables like Board *b, SearchData *sd could use longer names too, although the meaning of these is easier to infer.

This is all still very surface-level unfortunately, but I couldn't find any deeper issues. ...so, looking good! Aside from the freeze I got, this seems presentable already. Nice work!

Edit: Forked the project on accident, please don't mind that!