Improve ergonomics around editing SGF files

[obtext]

I'd like to use this library to merge two SGF files (add a new game to a tree), but it seems impossible currently without a way to add an SgfNode to the children of an existing one.

(I tried, but I'm still new to Rust so may have missed a way?)

[julianandrews]

The serialize function lets you take a collection of GameTrees (it could be a Vec or anything else that implements IntoIterator), and generate the SGF file. You can see an example of how to do it in the docs there.

It turns out that with the way the SGF spec is defined, you don't actually have an SgfNode or GameTree at the root - just some container object, and I didn't see any reason to require a specific container - any IntoIterator object works.

You can see the flip side of this with the parse function which returns a vector of GameTrees.

I should see if I can make this more obvious in the documentation.

[julianandrews]

As a concrete suggestion, if you're working with a collection of game trees you probably want to either use a Vec<GameTree> or a Vec<SgfNode> (where in the second case you'll convert into GameTrees right before serialization). It should be possible to convert between a GameTree and an SgfNode without copying.

[obtext]

Ah, thank you, I will try with serialize directly. I should have looked at its documentation and realized that it would work with a collection of game trees!

[obtext]

This may not be quite what I'm after unfortunately. I want to merge game B into game tree A by creating a variation in A at the point of the first differing move, throwing away other metadata such as player names etc in B.

[julianandrews]

So I mostly developed this library for the parse functionality, and haven't made heavy use of serialize or editing. I made a point of making sure the semantics of editing nodes are correct and that you could do anything necessary, but the ergonomics are a little gross.

Here's an example of code that should do what you want:

use sgf_parse::go::{parse, Prop};

fn main() {
    // Setup a couple nodes for our example
    let mut game_a = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hd];W[gd])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();
    let game_b = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hf];W[gf])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();

    let mut parent_a = &mut game_a;
    let mut parent_b = &game_b;
    // Find the first node that's different
    loop {
        let node_a = parent_a.children().next().unwrap();
        let node_b = parent_b.children().next().unwrap();
        let move_a = node_a.get_property("B").or(node_a.get_property("W"));
        let move_b = node_b.get_property("B").or(node_b.get_property("W"));
        let moves_match = match move_a {
            None => move_b.is_none(),
            Some(Prop::B(m)) => matches!(move_b, Some(Prop::B(n)) if n == m),
            Some(Prop::W(m)) => matches!(move_b, Some(Prop::W(n)) if n == m),
            _ => unreachable!(),
        };
        if !moves_match {
            // add `node_b` as a neighbor to `node_a` in the tree
            parent_a.children.push(node_b.clone());
            break;
        }
        parent_a = &mut parent_a.children[0];
        parent_b = node_b;
    }
    println!("{}", game_a.serialize());
}

This is just a proof of concept and will fail on some important edge cases - to do this properly you'd want to replace the unwrap calls with proper error handling.

Anyway, I hope this helps you move forward with your project. In the meanwhile I'm going to stare at this a while and see if I can come up with a way to make this interface a little more ergonomic.

[obtext]

Thank you very much! This solves it.

[julianandrews]

It's a small change, but I've added an Eq implementation for SgfProp and released a new version (4.1.0). The ergonomics still aren't ideal, but it helps clear a lot of the noise. The code above should look something like this now:

use sgf_parse::go::parse;

fn main() {
    let mut game_a = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hd];W[gd])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();
    let game_b = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hf];W[gf])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();

    let mut parent_a = &mut game_a;
    let mut parent_b = &game_b;
    loop {
        let node_a = parent_a.children().next().unwrap();
        let node_b = parent_b.children().next().unwrap();
        let move_a = node_a.get_property("B").or(node_a.get_property("W"));
        let move_b = node_b.get_property("B").or(node_b.get_property("W"));
        if move_a != move_b {
            parent_a.children.push(node_b.clone());
            break;
        }
        parent_a = &mut parent_a.children[0];
        parent_b = node_b;
    }
    println!("{}", game_a.serialize());
}

[julianandrews]

I've added a few more updates in 4.2.0 to make code like this a little cleaner. There's now a main_variation method that makes it easy to iterate over the main variation of a node, and a get_move method that gets the move for a given node.

With those, I was able to rewrite this logic more concisely, and in a way that handles most of the edge cases correctly:

use sgf_parse::{
    go::{parse, Prop},
    SgfNode,
};

fn main() {
    let game_a = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hd];W[gd])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();
    let game_b = parse("(;GM[1]SZ[9];B[ee];W[ce];B[ge];W[he];B[hf];W[gf])")
        .unwrap()
        .into_iter()
        .next()
        .unwrap();

    println!("{}", merge_variation(&game_a, &game_b));
}

/// Merge `variation` into `node` at the first point where child moves differ.
///
/// Note that this function doesn't check the root nodes (which should probably have no moves).
fn merge_variation(node: &SgfNode<Prop>, variation: &SgfNode<Prop>) -> SgfNode<Prop> {
    let mut node = node.clone();

    let mut parent = &mut node;
    for variation in variation.main_variation().skip(1) {
        let child = parent.children().next();
        match child {
            Some(child) if child.get_move() == variation.get_move() => {}
            _ => {
                parent.children.push(variation.clone());
                break;
            }
        }
        parent = &mut parent.children[0];
    }

    node
}

[julianandrews]

I think I'm going to close this issue now. I might re-evaluate improving the documentation at some point, but for now, I don't see an obvious way to make it better, and I think the better ergonomics here will help make problems like this easier to approach.

[obtext]

Happy for this to be closed, thank you! I was able to adapt your code to do what I need, which is a little bit more complex because I need to check all children of parent in the main tree for matches, not just the first child (in other words, I'm not concerned only with the main variation). I added a function

fn move_at(node: &SgfNode<Prop>) -> Option<&Prop> {
    node.get_property("B").or(node.get_property("W"))
}

fn moves_match(node_a: &SgfNode<Prop>, node_b: &SgfNode<Prop>) -> bool {
    move_at(node_a) == move_at(node_b)
}

fn match_index<'a>(
    children: impl Iterator<Item = &'a SgfNode<Prop>>,
    target: &SgfNode<Prop>,
) -> Option<usize> {
    let mut i: usize = 0;
    for child in children {
        if moves_match(child, target) {
            return Some(i);
        }
        i += 1;
    }
    return None;
}

to return the index of the matching child node if any, and then used that to select the new parent.