Not using set_free_handicap ?

[lemonsqueeze]

One thing i'm confused about: human plays black with handicap, why does gtp2ogs use play commands to send handicap stones to the engine ? Shouldn't it be using set_free_handicap instead ?

I see it's there in the code, but only on loadstate(), so it's practically never used.

I had to put some workarounds in Pachi because of this, otherwise handicap wouldn't get detected correctly. Not sure how other engines cope with this...

[roy7]

I think it had to do with Chinese vs Japanese when I was first working on gtp2ogs and bots weren't Chinese-only back then. In Chinese, handicaps are freely placed but in Japanese I believe they are supposed to go to specific locations. Had something to do with that, but it's been a really long time.

I'm also unsure if set_free_handicap can be used to place stones one at a time. It takes a list of vertices to put stones on.

[TokumotoK]

Some bots are Chinese-ruleset-only (like AlphaGo despite its Korean Pro status) and some are Japanese-ruleset-only (like AQ, though it can accept Chinese), yet many experimental bots are Tromp-Taylor-ruleset-only (https://www.cs.cmu.edu/~wjh/go/tmp/rules/TrompTaylor.html) because that is the ruleset used by the Computer Go Server.

To me, using play command is about the only viable solution. I once invited Pasky (Pachi dev) to run Pachi on OGS years ago, but that issue might have dissuaded him.

[lemonsqueeze]

Interesting, until now the sequences i was familiar with used either fixed_handicap or set_free_handicap for japanese rules:

kgs-rules japanese
boardsize 19
clear_board
komi 0.5
fixed_handicap 4
genmove w

and set_free_handicap for chinese rules (no choice here):

kgs-rules chinese
boardsize 19
clear_board
komi 0.5
set_free_handicap d16 q16 d4 q4
genmove w

Yes, set_free_handicap must send all stones at once afaik.

No big deal for sure, and looks like other engines are happy with play commands too. Initially i put workarounds in pachi, but i think i'll try fixing gtp2ogs instead: there's a potential for issues with this. For example an sgf file with initial b&w setup stones converted to gtp also results in multiple play b commands, so it's going to think it's handicap game ...

[TokumotoK]

Ahh, now I clearly see what you meant.

Thanks to Nick Wedd, a large part of computer Go has been KGS-driven for a long time.
With the resurrection of CGOS (https://senseis.xmp.net/?CGOS) by Hiroshi Yamashita, and roy7's efforts on gtp2ogs, I hope this picture is changing.

[roy7]

This probably is being done incorrectly then. It's also maybe why the adaptive resign code in LZ doesn't do anything for LZ bots on OGS. I'm not sure of a simple solution though.

[lemonsqueeze]

Trying this in my set_free_handicap branch. This seems to work ok (starting and resuming live games at least) but there are some code paths i'm unclear about:

• initial_state in loadState(): looks like it's always empty in the games i tried. Maybe the server uses it for some special games ? Having initial_state + handicap would be problematic ...

• The setup code after:

  // If we're in free placement handicap phase of the game, make extra moves or wait it out, as appro$
  //                                                                                                  
  // If handicap == 1, no extra stones are played.                                                    
  // If we are black, we played after initial gamedata and so handicap is not < length.               
  // If we are white, this.state.moves.length will be 1 and handicap is not < length.                 
  //                                                                                                  
  // If handicap >= 1, we don't check for opponent_evenodd to move on our turns until handicaps are f$

  It seems the if (this.bot) ... statements never trigger, for me bot is always undefined until first genmove, so looks this logic isn't used. Are there special cases where it triggers maybe ?

[lemonsqueeze]

btw what's a forked game ? Can bots end up in one ?

[roy7]

Yes, you load up a game and move to the move you want to start a new game from, hit Fork, and it'll make a brand new game starting from that position. Bots will accept it like any other match.

[lemonsqueeze]

Nice =)
Ok, i see initial_state is set in forked games.
Just tried one with experimental code. Right now it won't use set_free_handicap in this case, but i'm happy to live with that ...

[roy7]

What should probably be happening is chinese handicap moves are stored in gtp2ogs and then when the last one is made and we need to ask the bot for a move, we issue the set_free_handicap before asking for the genmove. That's basically what happens when a game is resumed if the bot is restarted (or if --persist isn't being used I assume).

You're the first person to notice this behavior isn't strictly right but it also means Leela Zero isn't picking up the proper handicap info either since it has a dynamic resign threshold for handicap games which is currently being ignored. I never noticed it myself these past 1-2 years since I mostly only cared about my bots ranked non-handicap matches. :) Oops!

[lemonsqueeze]

Yes, checkout my branch, this is pretty much what it does. I was thinking along the same line: storing moves until ready, but it turned out even simpler: bot doesn't exist until genmove, at which point loadState() is called and sends all the play commands. So gtp2ogs already does the buffering for us, just need to shape the play commands into set_free_handicap there:

// Use set_free_handicap for handicap stones, play otherwise.
if (doing_handicap && handicap_moves.length < state.handicap) {
   handicap_moves.push(move);
   if (handicap_moves.length == state.handicap)
       this.sendHandicapMoves(handicap_moves, state.width);
   else continue;  // don't switch color.
} else
   this.command("play " + c + ' ' + move2gtpvertex(move, state.width))

[roy7]

Will do! :)

[roy7]

Why remove the support for set_free_handicap in state.initial_state?

[roy7]

I won't experiment tonight but here's what I'm thinking. As you point out, the bot isn't created until the game needs a move from the bot. So the initial loadState() is where the handicap magic needs to happen.

When state comes from server, if there are already handicap stones placed they are in state.initial_state.black. (If someone is doing something odd with white getting the stones we can ignore that and just play the stones.) Other moves (including handicap stones?) are in state.moves. This is already supported and working with set_free_handicap.

If we connect when handicap is partially done, I'm unsure if state.initial_state.black has the partial list or not. I'm going to assume it does. As more handicap placements arrive, they are added to state.moves. I assume a partial list gets sent with set_free_handicap. We'd need to set up a few test handicap games and dump gamedata packets to see exactly what the server sends in this case.

If we connect at game start then state.initial_state.black is empty, and all moves that arrive go into state.moves within gtp2ogs.

So it seems to me, on the gtp2ogs end, if we are in a handicap game and not all handicap moves have been played yet, do we want to add the play to state.initial_state.black instead of state.moves? (Or do they go on both? Without dumping a gamedata I'm unsure if state.initial_state.black moves are duplicated in state.moves or not.)

The result here is that whether a game was connected to mid-handicap or at start of match, one the bot does start, state.initial_state.black correctly has all handicap stones inside it. And thus set_free_handicap will be used properly.

Because

gtp2ogs/gtp2ogs.js

Line 572 in f6aadc9

for (let i=0; i < moves.length; ++i) {

uses all of the state.moves to send moves after handicap is handled, I'm assuming the moves in state.initial_state.black and state.initial_state.white are not duplicated into state.moves.

As such... would we change

gtp2ogs/gtp2ogs.js

Line 795 in f6aadc9

this.state.moves.push(move.move);

to push to the right initial_state instead of moves if it's a handicap placement?

All of the tests like (this.state.handicap) > this.state.moves.length would break though. We'd need to compare the handicap perhaps to this.state.moves.length + this.state_initial.black.length?

Anything using state.moves.length() would have to be glanced up to see if we'd break it by not putting the handicap stones in state.moves any more, and instead putting them into state.initial_state.black.

There might be better alternatives as well. For example, we leave all of the move processing logic the same but in loadState we could see if state.initial_state.black.length is less than state.handicap, and if so we know some handicap moves are inside state.moves and thus add those to our set_free_handicap command and skip the move commands at

gtp2ogs/gtp2ogs.js

Line 572 in f6aadc9

for (let i=0; i < moves.length; ++i) {

by setting the counter to the # of moves to skip (used earlier from state.moves in set_free_handicap command) instead of 0.

[roy7]

The thing is we don't want to break forked games. ;) We should try and solve this in a way that works for every scenario. (Also remember someone might give handicap to the bot, which will be calling genmove on each handicap placement needed. Should be tested with any changes.)

Off the cuff we could do something like

for (let i=0; i < black.length; ++i) {
s += " " + move2gtpvertex(black[i], state.width);
}
for (let i=0; i < state.handicap - black.length; i++) {
s += " " + move2gtpvertex(state.moves[i], state.width);
}

Because we know when we loadState opponent has no more handicap stones to place. And then we do this at line 572 if black had handicap:

for (let i=state.handicap - black.length; i < moves.length; ++i) {

(black.length doesn't exist in this scope but I'm just giving example what I'm thinking, could calculate those arrays in the outer scope to access here).

I think the merit of this idea is nothing outside of loadState changes, we simply add more black moves to set_free_handicap if needed?

[lemonsqueeze]

Why remove the support for set_free_handicap in state.initial_state?

Had to rewrite it because there are issues with it:
First I tested all the situations i could think of, initial_state is always empty, even in handicap games. The only case i found so far where it's not is forked games, in which case all the stones on the board are in there.
Current code:

        if (state.initial_state) {
            let black = decodeMoves(state.initial_state.black, state.width);
            let white = decodeMoves(state.initial_state.white, state.width);

            if (black.length) {
                let s = "";
                for (let i=0; i < black.length; ++i) {
                    s += " " + move2gtpvertex(black[i], state.width);
                }
                this.command("set_free_handicap " + s);
            }
            ...

So only in forked games we have black.length or white.length != 0.
We can't do that for those though: if there are 70 black stones on the board it's going to set_free_handicap 70 stones !

So for now i just send play commands which is more sane. You're right, in the end we'll need a proper set_free_handicap there as well, but i wanted to get forked games out of the way to get a fix for regular games first (put another way, it's been like this a long time, it can wait a little =)

[lemonsqueeze]

I think the merit of this idea is nothing outside of loadState changes, we simply add more black moves to set_free_handicap if needed?

Yes, definitely.
The normal case is simple actually: bot doesn't exist until genmove, so there won't be a loadState() midway through the handicap setup.

Try to break my branch, i've been running pachi without workarounds with it for a day or two trying different things and couldn't find flaws so far. Handicap looks good except in forked games, but that's easy to fix now.

[lemonsqueeze]

The other way (handicap with bot as black) is more tricky, maybe we can have another issue / PR for it ?
Just realized recently i'd not tested it with pachi on ogs. Turned out it was crashing ! Complaining about non-alternating gameplay after receiving two genmoves in a row...

[lemonsqueeze]

One thing i haven't tested is 'edited' moves:

if (move.edited) {
    ...

They could get in the way if there are some in/before the handicap stones.
But first i need to find out what these are ...

[roy7]

Oh that's a mess. @anoek what were you thinking? ;)

I assumed from reverse engineering that state.initial_state was only the handicap stone placements. But I guess that's not true at all. Oops.

[anoek]

¯\_(ツ)_/¯

[roy7]

I don't even know what edited moves are lol. This code predates my involvement.

So initial state is the state when game was created, like forked. Easy enough.

[roy7]

I will give your code a try on my bots tonight. So if there is an initial state we count off some handicap stones from there and the moves list and then do normal moves for the rest of the state. If state is shorter than handicap we append some moves to it. I agree existing code probably just gets scrapped. I'm glad you discovered this.

Handicap stones played by white isn't a thing right?

[lemonsqueeze]

Sounds good to me.

Handicap stones played by white isn't a thing right?

Hell, no ! That would be sooo wrong...
Could be interesting april fools material for a feature announcement though.

[roy7]

I tried it out some with RoyalLeela and it seemed to work fine. So right now on a fork of a handicap game, we'll play all stones with normal move commands and still not use set_free_handicap. But non-forked handicap matches will use the command.

Do you want to try and get forked games working correctly too before doing a pull request?

[roy7]

@anoek I don't know what an "edited" move in moves.edited is. Can you clue me in? :)

[lemonsqueeze]

Do you want to try and get forked games working correctly too before doing a pull request?

Ok, I updated my branch to handle forked games but it doesn't work:
Looks like state.handicap isn't set in forked games (?) so right now it still falls back to play commands ...

Edit: Actually, forked game itself is setup without handicap, see #12434541's game info for example.

[anoek]

An "edited move" is used in reviews to place stones on the board in non accordance to the normal flow of the game... ie, if you want to just create a wall of black, or make an impossible situation for demonstration purposes.

I don't think you should ever see those in a game for bots, unless you are seeing them for forked games? (sorry it's been awhile since I've been in that code)

[roy7]

I imagine it's safe to ignore. @lemonsqueeze Maybe make a weird board in the editor, then fork it, and see if the gamedata has moves.edited or not? If not then we never need to deal with edited moves. And atm since forked games lose handicap data, we can probably ignore that too. Unless @anoek wants to consider that a server bug and I could go post an issue for it.

[roy7]

I'm pretty much okay with "if you are going to fork a game or edit a game then the bot might not be aware of handicap properly", considering handicap has been wrong all of this time and bots still work well enough. ;)

[anoek]

I do consider the forked games losing handicap data a server side bug, kinda forgotten there was still some issues with that

[lemonsqueeze]

Just tried a few forked edited games: no edited moves so far, all goes into initial_state.
So i guess it's safe to remove.
Updated set_free_handicap branch: initial code without forked game / edited moves support.
And set_free_handicap2 has forked games code should we need it again sometime.

I used to think the other case was the tricky one, maybe it's this one actually =)

[lemonsqueeze]

btw, i'm running gtp2ogs with extra logging, sometimes i see notification.handicap = -1 when receiving a challenge. It happens only there, later on state.handicap is set correctly during the game (can be anything, even game, handicap ...)
Just seems weird ...
Could be bypassing maxhandicap check i guess.

[roy7]

That means they used "auto" handicap. I tried at one point to figure out the correct handicap to process it with max/min handicap checking but decided to just give up and set RoyalLeela to do no ranked games with handicap at all. ;)

If we need to check and not worry about set_free_handicap in the -1 case I can live with that. The API to my knowledge doesn't tell the client the # of handicap stones assigned is the hassle.

[roy7]

I'd prefer the server internally figure out the auto handicap, then tell client the real handicap being offered/assigned.

[lemonsqueeze]

Ok, it's a feature then =)
I can live with that too, was asking just in case...

[roy7]

@lemonsqueeze Since #71 was merged is this issue ready to close out?

[lemonsqueeze]

Hi roy, yes this one can be closed, thanks.