Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Oct 1, 2018
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d45406b
Support Read/Write Sgf with Variation, AW/AB & Comment
zsalch Sep 18, 2018
2b6358a
Use Spaces instead of Tab
zsalch Sep 19, 2018
8ebe43e
Remove unnecessary code
zsalch Sep 19, 2018
bf7587f
Add Test Case
Sep 22, 2018
339feef
Allow top branch
zsalch Sep 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/featurecat/lizzie/rules/Board.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,19 @@ public static boolean isValid(int x, int y) {
return x >= 0 && x < BOARD_SIZE && y >= 0 && y < BOARD_SIZE;
}

/**
* The comment. Thread safe
* @param comment the comment of stone
*/
public void comment(String comment) {
synchronized (this) {

if (history.getData() != null) {
history.getData().comment = comment;
}
}
}

/**
* The pass. Thread safe
*
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/featurecat/lizzie/rules/BoardData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ public class BoardData {
public int blackCaptures;
public int whiteCaptures;

// Comment in the Sgf move
public String comment;

public BoardData(Stone[] stones, int[] lastMove, Stone lastMoveColor, boolean blackToPlay, Zobrist zobrist, int moveNumber, int[] moveNumberList, int blackCaptures, int whiteCaptures, double winrate, int playouts) {
this.moveNumber = moveNumber;
this.lastMove = lastMove;
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/featurecat/lizzie/rules/BoardHistoryList.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ public BoardData next() {
return head.getData();
}

/**
* moves the pointer to the right, returns the node stored there
*
* @return the next node, null if there is no next node
*/
public BoardHistoryNode nextNode() {
if (head.next() == null)
return null;
else
head = head.next();

return head;
}

/**
* moves the pointer to the variation number idx, returns the data stored there
*
Expand Down
180 changes: 121 additions & 59 deletions src/main/java/featurecat/lizzie/rules/SGFParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package featurecat.lizzie.rules;

import java.util.HashMap;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand Down Expand Up @@ -66,6 +68,12 @@ private static boolean parse(String value) {
return false;
}
int subTreeDepth = 0;
// Save the variation step count
Map<Integer, Integer> subTreeStepMap = new HashMap<Integer, Integer>();
// Comment of the AW/AB (Add White/Add Black) stone
String awabComment = null;
// Previous Tag
String prevTag = null;
boolean inTag = false, isMultiGo = false, escaping = false;
String tag = null;
StringBuilder tagBuilder = new StringBuilder();
Expand All @@ -79,12 +87,9 @@ private static boolean parse(String value) {
String blackPlayer = "", whitePlayer = "";

PARSE_LOOP:
for (byte b : value.getBytes()) {
// Check unicode charactors (UTF-8)
char c = (char) b;
if (((int) b & 0x80) != 0) {
continue;
}
// Suppoert unicode charactors (UTF-8)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo Suppoert

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

for (int i = 0; i < value.length(); i++) {
char c = value.charAt(i);
if (escaping) {
// Any char following "\" is inserted verbatim
// (ref) "3.2. Text" in https://www.red-bean.com/sgf/sgf4.html
Expand All @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
if (i > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Allow the comment tag includes '('
tagContentBuilder.append(c);
}
}
break;
case ')':
if (!inTag) {
subTreeDepth -= 1;
if (isMultiGo) {
break PARSE_LOOP;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

// Restore to the variation node
for (int s = 0; s < subTreeStepMap.get(Integer.valueOf(subTreeDepth)).intValue(); s++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Lizzie.board.previousMove();
}
}
subTreeDepth -= 1;
} else {
// Allow the comment tag includes '('
tagContentBuilder.append(c);
}
break;
case '[':
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Lizzie.board.place(move[0], move[1], Stone.WHITE);
}
} else if (tag.equals("C")) {
// Support comment
if ("AW".equals(prevTag) || "AB".equals(prevTag)) {
awabComment = tagContent;
} else {
Lizzie.board.comment(tagContent);
}
} else if (tag.equals("AB")) {
int[] move = convertSgfPosToCoord(tagContent);
if (move == null) {
Expand Down Expand Up @@ -173,6 +202,7 @@ private static boolean parse(String value) {
e.printStackTrace();
}
}
prevTag = tag;
break;
case ';':
break;
Expand All @@ -199,6 +229,11 @@ private static boolean parse(String value) {
// Rewind to game start
while (Lizzie.board.previousMove()) ;

// Set AW/AB Comment
if (awabComment != null) {
Lizzie.board.comment(awabComment);
}

return true;
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zsalch zsalch Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Stone[] stones = history.getStones();
StringBuilder abStone = new StringBuilder();
StringBuilder awStone = new StringBuilder();
for (int i = 0; i < stones.length; i++) {
Stone stone = stones[i];
if (stone.isBlack() || stone.isWhite()) {
// i = x * Board.BOARD_SIZE + y;
int corY = i % Board.BOARD_SIZE;
int corX = (i - corY) / Board.BOARD_SIZE;

char x = (char) (corX + 'a');
char y = (char) (corY + 'a');

if (stone.isBlack()) {
abStone.append(String.format("[%c%c]", x, y));
} else {
awStone.append(String.format("[%c%c]", x, y));
}
}
}
if (abStone.length() > 0) {
builder.append("AB").append(abStone);
}
if (awStone.length() > 0) {
builder.append("AW").append(awStone);
}
}

// The AW/AB Comment
if (history.getData().comment != null) {
builder.append(String.format("C[%s]", history.getData().comment));
}

// replay moves, and convert them to tags.
// * format: ";B[xy]" or ";W[xy]"
// * with 'xy' = coordinates ; or 'tt' for pass.
BoardData data;

// TODO: this code comes from cngoodboy's plugin PR #65. It looks like it might be useful for handling
// AB/AW commands for sgfs in general -- we can extend it beyond just handicap. TODO integrate it
// data = history.getData();
//
// // For handicap
// ArrayList<int[]> abList = new ArrayList<int[]>();
// ArrayList<int[]> awList = new ArrayList<int[]>();
//
// for (int i = 0; i < Board.BOARD_SIZE; i++) {
// for (int j = 0; j < Board.BOARD_SIZE; j++) {
// switch (data.stones[Board.getIndex(i, j)]) {
// case BLACK:
// abList.add(new int[]{i, j});
// break;
// case WHITE:
// awList.add(new int[]{i, j});
// break;
// default:
// break;
// }
// }
// }
//
// if (!abList.isEmpty()) {
// builder.append(";AB");
// for (int i = 0; i < abList.size(); i++) {
// builder.append(String.format("[%s]", convertCoordToSgfPos(abList.get(i))));
// }
// }
//
// if (!awList.isEmpty()) {
// builder.append(";AW");
// for (int i = 0; i < awList.size(); i++) {
// builder.append(String.format("[%s]", convertCoordToSgfPos(awList.get(i))));
// }
// }

while ((data = history.next()) != null) {

String stone;
if (Stone.BLACK.equals(data.lastMoveColor)) stone = "B";
else if (Stone.WHITE.equals(data.lastMoveColor)) stone = "W";
else continue;

char x = data.lastMove == null ? 't' : (char) (data.lastMove[0] + 'a');
char y = data.lastMove == null ? 't' : (char) (data.lastMove[1] + 'a');

builder.append(String.format(";%s[%c%c]", stone, x, y));
}


// Write variation tree
builder.append(generateNode(board, writer, history.nextNode()));

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

// Generate node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

private static String generateNode(Board board, Writer writer, BoardHistoryNode node) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the writer argument here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garbage code, deleted.

StringBuilder builder = new StringBuilder("");

if (node != null) {

BoardData data = node.getData();
String stone = "";
if (Stone.BLACK.equals(data.lastMoveColor) || Stone.WHITE.equals(data.lastMoveColor)) {

if (Stone.BLACK.equals(data.lastMoveColor)) stone = "B";
else if (Stone.WHITE.equals(data.lastMoveColor)) stone = "W";

char x = data.lastMove == null ? 't' : (char) (data.lastMove[0] + 'a');
char y = data.lastMove == null ? 't' : (char) (data.lastMove[1] + 'a');

builder.append(String.format(";%s[%c%c]", stone, x, y));

// Write the comment
if (data.comment != null) {
builder.append(String.format("C[%s]", data.comment));
}

if (node.numberOfChildren() > 1) {
// Variation
for (BoardHistoryNode sub : node.getNexts()) {
builder.append("(");
builder.append(generateNode(board, writer, sub));
builder.append(")");
}
} else if (node.numberOfChildren() == 1) {
builder.append(generateNode(board, writer, node.next()));
} else {
return builder.toString();
}
}
}

return builder.toString();
}
}