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.

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

Feature/abutting kata overlay #571

Merged
merged 12 commits into from
Aug 3, 2019

Conversation

gjm11
Copy link
Contributor

@gjm11 gjm11 commented Jul 12, 2019

This PR adds a new config option called show-katago-estimate-large. By default it's true (this could be changed for greater conservatism). When it's true and KataGo ownership display is turned on, instead of showing KataGo's ownership estimates as small squares on top of the stones they are shown as squares exactly large enough to abut one another without gaps, underneath the stones, with a bit more transparency to make up for the fact that they're larger.

I find this both easier to make sense of and less distracting when I just want to focus on the board. (Others may feel differently, which is why it's optional.)

While I was visiting that bit of the code, I also streamlined it a little -- there was some avoidable repetition.

You can see a (reduced-resolution) comparison of old and new at https://i.imgur.com/8E9huSK.png.

This change also modifies what happens when "branch" stones are displayed and small squares are used for the ownership overlay. Before, "branch" stones would (unlike stones actually played on the board) appear over rather than under the ownership squares -- making these purely hypothetical stones more prominent on the board than the ones actually played, and also hiding the ownership predictions. I've changed that so that small ownership squares appear on top of branch stones as well as actually-played stones.

gareth.mccaughan added 2 commits July 12, 2019 22:22
… rectangles large enough that they abut, with more transparency, and _behind_ the stones, which some users -- including me -- may prefer.
@roy7
Copy link

roy7 commented Jul 12, 2019

@featurecat There's been a lot of recent interest in Katago on Windows, which the author has been working on supporting as well as adding support for non-nVidia GPUs. Lizzie seems to be the GUI of choice for Katago users. If you have a chance to merge some of these recent PRs in, maybe it'd be a good time for a fresh official release so the latest 'official' client will work well out of box with Katago?

Thanks for your big contribution to the scene!

@yzyray
Copy link
Contributor

yzyray commented Jul 13, 2019

It is good when the game is on start or middle stage ,but seems not very clear when the game comes to ends especially the deadstones.
对比2
对比1

@isty2e
Copy link

isty2e commented Jul 13, 2019

Then what about the mixture of two? Filling the square fully like in the first figure, but additionally drawing smaller squares onto the dead stones like in the second figure seems decent.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 13, 2019

@yzyray Personally I find "my" version clearer even when there are lots of stones on the board. I take your point, though: it is harder to see the occupancy info behind stones than on empty points.

It would be possible to draw the overlay "on top" of the stones on the board instead of underneath, which would largely resolve that problem -- but at the cost of making the stones themselves harder to see, which I think would be worse rather than better overall.

Maybe the "." key should cycle through { no overlay, full-square overlay behind stones, small-square overlay on top of stones } and then if you find different ones clearer at different stages in the game you can change?

Or large-square mode could also draw small squares but only when there's a stone present on the board? That might be awkward to implement in the presence of "branch" stones, though.

I don't see any option I'm 100% happy with. Anyone got any cleverer ideas?

@zsalch
Copy link
Contributor

zsalch commented Jul 15, 2019

I think, only the dead stone need to draw a square.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 15, 2019

How to define "dead stone" for that purpose, though? Draw squares on top of black stones whose ownership estimate is more-white-than-black, and on top of white stones whose ownership estimate is more-black-than-white? That might work but seems a little complicated.

(I had another worry but I realise it was wrong; I'll say what it was in case anyone else has the same feeling. I was concerned that doing that would introduce a sudden "jump", from not drawing the extra square to drawing it. But if we draw the extra square exactly when the ownership estimate "has the wrong sign", then there isn't a jump because at the point where it changes we're drawing the square with alpha=0, i.e., completely transparent.)

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 16, 2019

OK, I have a version with the following behaviour.

  • Instead of the boolean option mentioned above, there is a string option called katago-estimate-mode.
  • This can take the values none, small, large, both, or smart.
  • In modes large, both, and smart we draw large squares under the stones.
  • In modes small, both, and smart we draw small squares on top of the stones; in smart mode, though, we don't do this on empty spaces or on stones whose colour matches the more-likely predicted ownership.
  • The . key, instead of toggling estimate display on or off, cycles through these options.

I haven't yet pushed this version because it might be appropriate to make a few other changes at the same time and I thought I should get others' opinions before doing so.

  • Given the functionality above, there's no real need for the boolean show-katago-estimate option to exist at all. Removing it would be a backward-incompatible change, but I think it would probably be an improvement.
  • There is a mode of operation, selected by the option show-katago-estimate-bysize, in which Lizzie displays small squares on top of the stones but varies not their opacity but their size; with the change above, this should probably be one of the modes. Or else it could control how the small squares are drawn when in modes that draw small squares.
  • As well as the KataGo estimate-drawing stuff, there is something similar for Zen. The code in BoardRenderer is almost identical, except that apparently Zen and KataGo have different conventions for whether their estimates are from black's perspective or that of whichever player is to move. It seems to me that these should be unified; this would simplify the code and also give Zen users the same range of options for estimate-drawing as KataGo users have.
    • At present there are completely separate options governing KG and Zen estimate-drawing. I think those should be unified too -- but note that this would be a backward-incompatible change because the option names would change. (I suppose it would be possible to read the "old" options as well as the "new" ones, but that seems like unnecessary complexity.)

Thoughts?

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 16, 2019

(Oh, the current version of this -- which, again, so far exists only on my laptop -- also includes the fix for #569.)

@zsalch
Copy link
Contributor

zsalch commented Jul 16, 2019

How to define "dead stone" for that purpose, though? Draw squares on top of black stones whose ownership estimate is more-white-than-black, and on top of white stones whose ownership estimate is more-black-than-white? That might work but seems a little complicated.

I think, it is a dead stone when the color is different from the square color.

PS: @yzyray proposes to draw a square on the alive stone, which is good for viewing the vitality of the stone. I think it makes sense.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 17, 2019

I've made a version (still only on my own machine) that, in addition to the changes above,

  • makes "no estimates shown" and "by size" just more modes in the list you can cycle through with "." (so there is no more show-katago-estimate-bysize; the cycling goes none -> small -> large -> both -> smart -> size -> none)
    • to repeat, "smart" means that we always show the large "abutting" squares but that we show the small squares on top of the stones only when the stone's colour differs from the estimate's
    • I'm not sure that "both" is actually ever what anyone wants; perhaps it should be removed
  • unifies the KataGo and Zen estimate-showing code
    • one reason why this may not be a good thing: if I'm understanding right, Zen's estimates are "ternary" (black, white, neither) rather than varying smoothly from -1 to +1, which means "size" mode isn't really any different from "small" mode. (And maybe the lightness/darkness is a little too extreme in "large"/"both"/"smart" modes?)
    • perhaps it would be better to do the cycling-through-states differently depending on whether the engine is Zen or not?

I can push this and make it the proposed change for this PR, but I don't want to do that without giving others a chance to object since some of the changes are backward-incompatible.

I have tested this with KataGo but not with Zen. (Zen is a commercial product and I don't have a copy.)

@zsalch
Copy link
Contributor

zsalch commented Jul 18, 2019

If can upload some screenshots, it might help to understand.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 18, 2019

OK, here are lots of screenshots. First of all, for context, here is a position with no ownership information shown:
image
and with the small-squares-on-top display already implemented in Lizzie:
image
Here are the large squares, completely covering the board, behind the stones:
image
and here is what happens when you show both large and small:
image
Here is what I've called "smart", implementing the suggestion made above to draw small squares only on "opposite-coloured" stones; in this case it's just the ones in the upper corners.
image
And here's the size-based visualization (which was already in Lizzie):
image
The behaviour I currently have in the local version of Lizzie on my laptop is that pressing . cycles between these six modes of display; there are no longer separate config options for turning ownership display on and off, or for showing ownership by square-size rather than by opacity. The same six modes are used when running with Zen rather than KataGo, which may or may not be a good idea.

Here is a modified version of the "smart" display, showing small squares on top of all stones rather than just "opposite-coloured" ones, as per @zsalch's more recent suggestion.
image

The position here is a pretty stupid one, intended only to exhibit a variety of degrees of ownership. Here's a position from a game between some very strong players :-), first of all with no ownership information shown:
image
and now with small squares on top of the stones:
image
which personally I find very distracting, and with large squares under the stones:
image
Here's the modified "smart" display (large squares behind + small squares on all stones but not on empty intersections):
image
and the original "smart" display (large squares behind + small squares on "opposite-colour" stones only):
image
I like the older "smart" display ("dead" stones only) better than the newer one (all stones) here.

My current thinking:

  • The modes most likely to be useful are "none", "small", "large" and (first version) "smart".
  • Some people may prefer "size" so it should probably also be there.
  • I doubt that anyone prefers "both" (but I could be wrong! let me know if so).
  • I think the first version of "smart" is better than the second.
  • I think cycling through modes, with "none" and "size" being two of them rather than being controlled separately, is better and simpler than having lots of separate options controlling these things.
  • I don't know anything about Zen and am not at all sure whether treating its ownership information basically the same way as KataGo's is actually a good idea. (And can't test whether my code is buggy when used with Zen.)

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 18, 2019

(I'm not very sure about the two versions of "smart". E.g., in those last two images the version that shows squares over all stones makes it easier to see that KataGo thinks the black stone on E16 is less securely white than the white stones surrounding it. Maybe that sort of comparison is worth the extra clutter.)

@zsalch
Copy link
Contributor

zsalch commented Jul 19, 2019

It provides a good choice, but I think in most cases, it may not need to cycling, but turn it on/off.

Maybe there are three options:

  1. Keep cycling, but provide a quick-close shortcut, such as Alt+.
  2. Just turn on/off, but the user can choose the style they like.
  3. Allow the user to chooses to turn on/off or cycling.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 19, 2019

Option 1: Alt-. is already used to mean "even though KataGo is being used, treat its ownership info as if it were Zen". I assume it's for a situation where you're running KataGo and Zen at the same time and want to be able to change which engine's ownership estimates are being displayed.

Option 2: that's certainly possible; it requires that the user remember the name of the style they prefer (whereas with the "cycling" approach they can just hit . until they see something they like) but maybe that's OK.

Option 3: to me this feels needlessly complicated. An option to decide how you select between other options?

It's probably true that most users will have a single style they prefer, and that they're more likely to want to turn ownership display on and off than to want to change the style. So I'm coming round to the following:

  • One config option to determine whether ownership is on or off by default. Another to determine how it's shown.
  • . on its own turns on and off.
  • Some other key/combination cycles between modes when on, does nothing when off. Not Alt-. because that already has a purpose. Maybe Ctrl-.? Or ;?

gareth.mccaughan added 3 commits July 19, 2019 17:21
@gjm11
Copy link
Contributor Author

gjm11 commented Jul 19, 2019

If I haven't messed up my merging :-) then the current state of this PR should behave as follows.

  • There is still a separate show-katago-estimate config option. Pressing . while Lizzie is running toggles this.
  • There is a new katago-estimate-mode config option. Pressing Ctrl-. while Lizzie is running and the estimate is being displayed cycles through its possible values.
  • The possible values are: small (old default mode: small squares, on top, everywhere), large (large squares, underneath, everywhere), large+small (both of those), large+dead (the first version of "smart": large squares + small ones where there are "opposite-coloured" stones), large+stones (the second version of "smart": large squares + small ones where there are any stones), and size.
  • There is no longer a show-katago-estimate-bysize config option; if you want that you set katago-estimate-mode to size or use Ctrl-..
  • KataGo and Zen estimates use largely the same code, and in particular Zen estimates are subject to the same set of modes as KataGo ones. If you're running both KataGo and Zen then you get the same presentation for both estimates; . still prefers KataGo in this situation and Alt-. forces Zen estimates.
  • I found another concurrency issue: if estimates are being displayed and you press . at the wrong time then removeEstimateRect can be called while drawEstimateRect is executing, and then the old estimates remain on the display until the next time you hit .. I've fixed that.
  • There was an option whose name contained subbord where it should have had subboard; I've fixed that. (Along with mainbord instead of mainboard, but that was only in variable names and not in anything user-facing. There are also a lot of variables and methods whose names have esitmate instead of estimate but I haven't changed those here -- it feels like the extra noise in the diffs would be a nuisance. They can be changed separately if anyone cares.)

Once again, I don't have Zen and can't test against it, so it's possible that I might have broken things for Zen users.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 19, 2019

The text the user sees on holding down the x key says (at least in the default English version) that . means "score game". I'm not sure that's an accurate description in the KataGo case (maybe it is for Zen), and of course it doesn't mention the new ctrl-. functionality I've added. I haven't tried to change this, not least because I am not competent to update all the other languages that have their own strings.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 19, 2019

@yzyray I think you're the Zen expert; at any rate, the Zen estimate code was added by you. Would it be possible for you to take a look at the changes in this PR and see whether they break things for Zen users? (I expect that the behaviour of Ctrl-. is suboptimal for people who are using Zen and not using KataGo as well, but my feeling is that it's acceptable.) Thanks!

@yzyray
Copy link
Contributor

yzyray commented Jul 24, 2019

I tried Zen estimate,now its like
zen
zen will not output a number on a live stone. so I think when zen use drawEstimateRect , it can always be "drawSmall".

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 24, 2019

The image in yzyray's comment above doesn't look particularly bad to me; I can imagine preferring it to the "small" style. But if Zen never gives ownership for points with stones on then the large+small, large+dead and large+stones options seem pretty pointless...

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 27, 2019

Uh-oh, looks like @zsalch's latest merge from master broke the build. I'm mostly away-from-keyboard right now, but in a few days' time I can take a look and see what happened (unless @zsalch wants to figure it out first...).

@TFiFiE
Copy link
Contributor

TFiFiE commented Jul 28, 2019

When ownership colors the entire board, you might consider (if only as another option) having it fully determine the color instead of it being blended with the normal wood color, so as to minimize distortion. Intersections and star points that would become too obscured by this could then possibly be rendered in white instead (which would hopefully make things not too ugly).

You might also want to make the ownership color spectrum customizable, so that it won't necessarily have to be on a gray scale.

@zsalch
Copy link
Contributor

zsalch commented Jul 29, 2019

Uh-oh, looks like @zsalch's latest merge from master broke the build. I'm mostly away-from-keyboard right now, but in a few days' time I can take a look and see what happened (unless @zsalch wants to figure it out first...).

Because the variable name in the config.java has been changed, references of the Menu.java must now be changed.

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 30, 2019

Wow, I had no idea that even existed.

... Ah, that's because it didn't exist until just now.

OK, so what I think I'll do -- rather than making the "Estimate info" menu super-long -- is to split it up. Something like this:

  • Estimate info
    • Display
      • None
      • On main board
      • On subboard
      • On both
    • Mode
      • Small
      • Large
      • Large + small
      • Large + small over "dead" stones
      • Large + small over all stones
      • Varying size

Any objections?

@gjm11
Copy link
Contributor Author

gjm11 commented Jul 30, 2019

OK, I've done that. As before, I have made no attempt to do anything with non-English string resources.

@zsalch
Copy link
Contributor

zsalch commented Aug 1, 2019

LGTM. but I think default should be tune off.

@gjm11
Copy link
Contributor Author

gjm11 commented Aug 1, 2019

You're right, the default should be off. I've changed that, and also fixed a stupid mistake. (I forgot that == on Java strings is testing object identity instead of actual content, with the result that the display mode wasn't shown correctly in the menu.)

Do we need all the variant styles? (I have no strong opinion on this; the least likely one for anyone to want, I think, is the one that shows large and small squares everywhere.) Should I make ctrl-. cycle through a smaller set of variants when using Zen rather than KG? (My preference is not to do this, but users who use Zen all the time and KG never may well feel differently.)

@zsalch
Copy link
Contributor

zsalch commented Aug 2, 2019

It seems to have no effect on Zen now.
Can't turn on for the Zen.

@gjm11
Copy link
Contributor Author

gjm11 commented Aug 3, 2019

That sounds bad. I don't have Zen to test with, so could you be a bit more specific:

  • If you try to turn it on when running Zen, do checkmarks appear where they should in the menus? Or does the View > KataGo > Estimate info > Display menu have "Not shown" selected even when you try to turn on ownership display?
  • If you repeatedly cycle using ctrl-. (with ownership display turned on), what happens to the checkmarks in the menu?
  • If you repeatedly cycle using ctrl-. (with ownership display turned on), do no ownership markers at all appear on any setting?

(My guess: everything appears right in the menus but nothing ever shows up because there's an error in my handling of the Zen ownership output. I'll investigate.)

@zsalch
Copy link
Contributor

zsalch commented Aug 3, 2019

I have no the Zen too.
@yzyray Could you help to fix the Zen issue?
Or I will merge the PR firstly and you submit a new PR to fix.

@zsalch zsalch merged commit 4100586 into featurecat:master Aug 3, 2019
@gjm11 gjm11 deleted the feature/abutting-kata-overlay branch August 3, 2019 17:59
zsalch added a commit to zsalch/lizzie that referenced this pull request Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants