-
Notifications
You must be signed in to change notification settings - Fork 266
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.
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
IPVGO: Add optional board state argument to the go analysis functions #1716
base: dev
Are you sure you want to change the base?
IPVGO: Add optional board state argument to the go analysis functions #1716
Conversation
Seems reminiscent to how we manage |
@@ -322,6 +332,49 @@ export function getStats() { | |||
return statDetails; | |||
} | |||
|
|||
export const boardValidity = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you're using this export anywhere? In which case, you can also change this to a tighter
const boardValidity = {
...
} const;
Declaration (making it a true constant for TypeScript)
error(boardValidity.badCharacters); | ||
} | ||
try { | ||
const board = boardFromSimpleBoard(boardState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the places where you use this end up wanting the full board. Why not return the full board, and take that as the optional argument (on the internal, not external side only)?
column.split("").map((char, y) => { | ||
if (char === "#") return null; | ||
if (char === "X") return blankPointState(GoColor.black, x, y); | ||
if (char === "O") return blankPointState(GoColor.white, x, y); | ||
return blankPointState(GoColor.empty, x, y); | ||
}), | ||
); | ||
updateChains(board); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this added? The comment explicitly says "no analytics."
@@ -59,6 +59,13 @@ export function getNewBoardState( | |||
return newBoardState; | |||
} | |||
|
|||
export function getNewBoardStateFromSimpleBoard( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only use this once, I think it makes more sense to just do this where you use it instead of exporting a new function.
export function getValidMoves() { | ||
const boardState = Go.currentGame; | ||
export function getValidMoves(_boardState?: SimpleBoard) { | ||
const boardState = _boardState ? getNewBoardStateFromSimpleBoard(_boardState) : Go.currentGame; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that you have to transform this into a BoardState, unlike the others, hints at the underlying issue with giving this an optional parameter of a SimpleBoard: Determining valid moves requires knowing if certain moves are illegal due to ko, and this simply cannot be represented from a SimpleBoard representation. Given the subtlety of this particular foot-gun, I think this one would be better to omit (unless you can come up with a way to provide a full BoardState as input.)
On one hand, I kind of agree. On the other hand, I hate having to pay for formulas, and the design of these fits in well extending the existing analysis functions, so I'm fine just doing it this way. :D |
Adds support to the go analysis functions to provide an optional boardstate to analyze, instead of only supporting analysis of the current board state. This makes it easier to build lookahead or other move search tools.
This feature was requested here:
https://discord.com/channels/415207508303544321/1221920484111814828/1297387013036576818