From bd6d5b62e3febedaf4639c4fcee66bc9cbac8e6e Mon Sep 17 00:00:00 2001
From: Savanni D'Gerinel <savanni@luminescent-dreams.com>
Date: Fri, 20 Oct 2023 00:36:03 -0400
Subject: [PATCH] Reduce the recursion amount of parser Node to GameNode

---
 sgf/src/game.rs | 59 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/sgf/src/game.rs b/sgf/src/game.rs
index 003c0a7..ad90d78 100644
--- a/sgf/src/game.rs
+++ b/sgf/src/game.rs
@@ -43,6 +43,14 @@ pub struct Player {
     pub team: Option<String>,
 }
 
+/// This represents the more semantic version of the game parser. Where the `parser` crate pulls
+/// out a raw set of nodes, this structure is guaranteed to be a well-formed game. Getting to this
+/// level, the interpreter will reject any games that have setup properties and move properties
+/// mixed in a single node. If there are other semantic problems, the interpreter will reject
+/// those, as well. Where the function of the parser is to understand and correct fundamental
+/// syntax issues, the result of the Game is to have a fully-understood game. However, this doesn't
+/// (yet?) go quite to the level of apply the game type (i.e., this is Go, Chess, Yinsh, or
+/// whatever).
 #[derive(Clone, Debug, PartialEq)]
 pub struct Game {
     game_type: GameType,
@@ -267,17 +275,45 @@ impl Node for GameNode {
 
 impl TryFrom<&parser::Node> for GameNode {
     type Error = GameNodeError;
+
     fn try_from(n: &parser::Node) -> Result<Self, Self::Error> {
+        // I originally wrote this recursively. However, on an ordinary game of a couple hundred
+        // moves, that meant that I was recursing 500 functions, and that exceeded the stack limit.
+        // So, instead, I need to unroll everything to non-recursive form.
+        //
+        // So, I can treat each branch of the tree as a single line. Iterate over that line. I can
+        // only use the MoveNode::try_from and SetupNode::try_from if those functions don't
+        // recurse. Instead, I'm going to process just that node, then return to here and process
+        // the children.
         let move_node = MoveNode::try_from(n);
         let setup_node = SetupNode::try_from(n);
 
-        match (move_node, setup_node) {
-            (Ok(node), _) => Ok(Self::MoveNode(node)),
-            (Err(_), Ok(node)) => Ok(Self::SetupNode(node)),
+        // I'm much too tired when writing this. I'm still recursing, but I did cut the number of
+        // recursions in half. This helps, but it still doesn't guarantee that I'm going to be able
+        // to parse all possible games. So, still, treat each branch of the game as a single line.
+        // Iterate over that line, don't recurse. Create bookmarks at each branch point, and then
+        // come back to each one.
+        let children = n
+            .next
+            .iter()
+            .map(|n| GameNode::try_from(n))
+            .collect::<Result<Vec<Self>, Self::Error>>()?;
+
+        let node = match (move_node, setup_node) {
+            (Ok(mut node), _) => {
+                node.children = children;
+                Ok(Self::MoveNode(node))
+            }
+            (Err(_), Ok(mut node)) => {
+                node.children = children;
+                Ok(Self::SetupNode(node))
+            }
             (Err(move_err), Err(setup_err)) => {
                 Err(Self::Error::UnsupportedGameNode(move_err, setup_err))
             }
-        }
+        }?;
+
+        Ok(node)
     }
 }
 
@@ -333,7 +369,7 @@ impl TryFrom<&parser::Node> for MoveNode {
     type Error = MoveNodeError;
 
     fn try_from(n: &parser::Node) -> Result<Self, Self::Error> {
-        let mut s = match n.mv() {
+        let s = match n.mv() {
             Some((color, mv)) => {
                 let mut s = Self::new(color, mv);
 
@@ -383,14 +419,6 @@ impl TryFrom<&parser::Node> for MoveNode {
             None => Err(Self::Error::NotAMoveNode),
         }?;
 
-        s.children = n
-            .next
-            .iter()
-            .map(|node| {
-                GameNode::try_from(node).map_err(|err| Self::Error::ChildError(Box::new(err)))
-            })
-            .collect::<Result<Vec<GameNode>, MoveNodeError>>()?;
-
         Ok(s)
     }
 }
@@ -677,9 +705,10 @@ mod file_test {
         with_text(&text, f);
     }
 
-    #[ignore]
+    /// This test checks against an ordinary game from SGF. It is unannotated and should contain
+    /// only move nodes with no setup nodes. The original source is from a game I played on KGS.
     #[test]
-    fn it_can_load_basic_game_records() {
+    fn it_can_load_an_ordinary_unannotated_game() {
         with_file(
             std::path::Path::new("test_data/2020 USGO DDK, Round 1.sgf"),
             |games| {