next: Support Read/Write Sgf with Variation, AW/AB & Comment #364

zsalch · 2018-09-18T05:47:55Z

Closes #350

Support following features when Read/Write the SGF file:

Variations tree
AW/AB (Add White)/(Add Black) stone
Comment (C tag)

OlivierBlanvillain · 2018-09-18T21:34:21Z

src/main/java/featurecat/lizzie/rules/SGFParser.java


        // close file
        builder.append(')');
        writer.append(builder.toString());
    }
+
+    // Generate node


Useless comment

OlivierBlanvillain · 2018-09-18T21:35:08Z

src/main/java/featurecat/lizzie/rules/SGFParser.java


        // close file
        builder.append(')');
        writer.append(builder.toString());
    }
+
+    // Generate node
+    private static String generateNode(Board board, Writer writer, BoardHistoryNode node) throws IOException {


Why do you need the writer argument here?

Garbage code, deleted.

OlivierBlanvillain · 2018-09-18T21:37:53Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

@@ -250,65 +285,92 @@ private static void saveToStream(Board board, Writer writer) throws IOException
                    builder.append(String.format("[%c%c]", x, y));
                }
            }
+        } else {
+        	// Process the AW/AB stone


I don't understand that else. Can you explain why this should be executed only when handicap == 0?

When has handicap, there is Black stones only, the original code already is processed.
The first time I modified the code for lizzie, I didn't want to destroy the original structure too much.
So just add a else.

OlivierBlanvillain · 2018-09-19T08:16:42Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

                        if (isMultiGo) {
-                            break PARSE_LOOP;


You can also remove the PARSE_LOOP label (outside the parsing loop)

OlivierBlanvillain · 2018-09-19T08:18:37Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

-            if (((int) b & 0x80) != 0) {
-                continue;
-            }
+        // Suppoert unicode charactors (UTF-8)


Typo Suppoert

But I think you should remove that comment. After your changes the code supports unicode because it uses charAt instead of looking that bytes, so it will be hard to understand the comment out of context.

OlivierBlanvillain · 2018-09-19T08:23:05Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

+                        // Initialize the step count
+                        subTreeStepMap.put(Integer.valueOf(subTreeDepth), Integer.valueOf(0));
+                    } else {
+                    	if (i > 0) {


Can you explain the i > 0 here? It seems that if we are inside a tag we always want to append that '('

For skip first '(' that at the top of the Sgf file.

OlivierBlanvillain · 2018-09-19T08:24:14Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

                        if (isMultiGo) {
-                            break PARSE_LOOP;
+                            // Restore to the variation node
+                            for (int s = 0; s < subTreeStepMap.get(Integer.valueOf(subTreeDepth)).intValue(); s++) {


It's a bit suspicious to recompute subTreeStepMap.get(Integer.valueOf(subTreeDepth)).intValue() at every step of the loop. Any reason for doing that instead of computing that just outside of the for?

OlivierBlanvillain · 2018-09-19T08:26:23Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

@@ -134,15 +152,26 @@ private static boolean parse(String value) {
                        if (move == null) {
                            Lizzie.board.pass(Stone.BLACK);
                        } else {
+                        	// Save the step count
+                        	subTreeStepMap.put(Integer.valueOf(subTreeDepth), Integer.valueOf(subTreeStepMap.get(Integer.valueOf(subTreeDepth)).intValue() + 1));


Indentation is off here and a bit all over the PR... Could you configure your text editor to output spaces instead of tabs? The output should look nicer on GitHub and it would also be more consistent with the rest of the lizzie code base.

Sorry, I have ignored this problem because with multiple editors.
Fixed it.

OlivierBlanvillain · 2018-09-19T08:27:43Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

                            Lizzie.board.place(move[0], move[1], Stone.BLACK);
                        }
                    } else if (tag.equals("W")) {
                        int[] move = convertSgfPosToCoord(tagContent);
                        if (move == null) {
                            Lizzie.board.pass(Stone.WHITE);
                        } else {
+                        	// Save the step count
+                        	subTreeStepMap.put(Integer.valueOf(subTreeDepth), Integer.valueOf(subTreeStepMap.get(Integer.valueOf(subTreeDepth)).intValue() + 1));


This line seams to be copy pasted. Is a way to avoid the duplication?

OlivierBlanvillain · 2018-09-19T08:34:04Z

src/main/java/featurecat/lizzie/rules/SGFParser.java

@@ -96,14 +101,27 @@ private static boolean parse(String value) {
                case '(':
                    if (!inTag) {
                        subTreeDepth += 1;
+                        // Initialize the step count
+                        subTreeStepMap.put(Integer.valueOf(subTreeDepth), Integer.valueOf(0));


I think in Java it's not required to explicitly convert between int and Integer, the java compiler will automatically box and unbox int and Integers to make things work. Concretely this line could be simplified to subTreeStepMap.put(subTreeDepth, 0);

This comment applies for the entirety of this PR, it would be nice if you could go over the diff and remove all the Integer.valueOf() and .intValue() calls that would be automatically inserted, I think the result would be simpler to read!

You are right.
The previous IDE compiler reported error, now there is no such problem.
Fixed.

OlivierBlanvillain · 2018-09-19T08:45:07Z

@zsalch These are some really good additions! The only thing that worries be is the absence of tests. The code seams to be rather low level and fragile, and it very hard just by looking at the diff to make sure that you didn't break anything. Also, how is this code going to evolve in the future? In the absence of tests, it would very easy for someone to accidentally break the support for comments of variations without noticing.

My suggestion to move forward would be to add integration tests to this PR. I would like this parsing code to be accompanied with a set of sample games that demonstrate all the features handled by the parser, and a few tests to show that the parser doesn't crash on these games and produces the expected output.

You could maybe start from this commit efc8fe3 where I setup JUnit for the project, and write a simple test for the parser. If you need help with that I might also be able to contribute a test sample to kick start the effort.

Also I think I will wait for this PR to be ready to merge before looking at #365.

zsalch · 2018-09-19T14:57:11Z

@OlivierBlanvillain

Thank you for your review, so the code is much clearer.

Introducing JUnit is a good idea and I will add some JUnit use cases.

For the test variations and comments, I think sgf e-book will be a good choice, of course, we have just added the AB / AW and C tag processing, for other tags have yet to be added.

I will upload some sgf for testing, but most of them are in Chinese. Maybe you can provide some English sgf files.

Regarding the test points, I think the first thing is the following:

1. Whether AB/AW and C tag and variations are correctly stored in the NodeList;
1. Is the AB/AW placed in the correct position;
1. Whether the variations are displayed correctly;
1. The comment for each step is displayed correctly;
1. When saving, AB/AW and C tag and branches are saved correctly.

Since the structure of the Node and the NodeList is not modified, the general operation should be normal when variations are correctly displayed. But this also requires testing to verify.

Before do JUnit testing, you may be want to refer #356 . Here are some simple test cases.

zsalch · 2018-09-22T16:00:38Z

@OlivierBlanvillain
I have added two simple test cases. Please check.

OlivierBlanvillain

Looks good to me!

Support Read/Write Sgf with Variation, AW/AB & Comment

d45406b

zsalch mentioned this pull request Sep 18, 2018

next: Support display the comment of the SGF file #365

Merged

zsalch changed the title ~~Support Read/Write Sgf with Variation, AW/AB & Comment~~ next: Support Read/Write Sgf with Variation, AW/AB & Comment Sep 18, 2018

zsalch mentioned this pull request Sep 18, 2018

next: About display comment of sgf and read/write sgf with variation & Winrate. #352

Closed

OlivierBlanvillain reviewed Sep 19, 2018

View reviewed changes

zsalch added 2 commits September 19, 2018 17:30

Use Spaces instead of Tab

2b6358a

Remove unnecessary code

8ebe43e

Add Test Case

bf7587f

OlivierBlanvillain approved these changes Sep 24, 2018

View reviewed changes

Allow top branch

339feef

featurecat merged commit f804902 into featurecat:next Oct 1, 2018

zsalch mentioned this pull request Oct 2, 2018

Use mouse to jump to any node(position) in variation window #374

Closed

This was referenced Oct 2, 2018

Store winrate and playouts in sgf #149

Closed

Saving SGF / Ctrl+C does not include variations #189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

next: Support Read/Write Sgf with Variation, AW/AB & Comment #364

next: Support Read/Write Sgf with Variation, AW/AB & Comment #364

zsalch commented Sep 18, 2018

OlivierBlanvillain Sep 18, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 18, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 18, 2018

zsalch Sep 19, 2018 •

edited

Loading

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

OlivierBlanvillain Sep 19, 2018

zsalch Sep 19, 2018

OlivierBlanvillain commented Sep 19, 2018

zsalch commented Sep 19, 2018

zsalch commented Sep 22, 2018

OlivierBlanvillain left a comment •

edited

Loading

next: Support Read/Write Sgf with Variation, AW/AB & Comment #364

next: Support Read/Write Sgf with Variation, AW/AB & Comment #364

Conversation

zsalch commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zsalch Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierBlanvillain commented Sep 19, 2018

zsalch commented Sep 19, 2018

zsalch commented Sep 22, 2018

OlivierBlanvillain left a comment • edited Loading

Choose a reason for hiding this comment

zsalch Sep 19, 2018 •

edited

Loading

OlivierBlanvillain left a comment •

edited

Loading