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

Set 'handicap' from GoEngine.computeScore to the handicap bonus #146

Conversation

dexonsmith
Copy link
Contributor

This commit changes GoEngine.computeScore() to return 0 for handicap when it does not contribute to the score. Indirectly, this changes the scoring summary in the web-client to not show the handicap in this case.

This fixes an inconsistency in the API/return, which forwarded to the web-client and caused user confusion.

Previously it was neither consistently equal to the handicap (see AGA) nor the handicap scoring bonus (see Japanese/Korean/New Zealand).

  • For Chinese/Ing, it was both the handicap scoring bonus and the handicap, which happen to be equal.
  • For AGA, it was the handicap scoring bonus, which is handicap-1.
  • For Japanese/Korean/New Zealand, it was the handicap (scoring bonus is 0).

Now, for all rulesets, it's the handicap scoring bonus (if any). For a client computing the score, this is what's relevant. This is similar to how stones is returned for rulesets where it's relevant to the score.

It might be nice for the web-client to show the number of handicap stones somewhere in the main view, but (a) it wasn't doing that previously and (b) computeScore() isn't the right way to get that info.

Fixes online-go/online-go.com#1469

@dexonsmith
Copy link
Contributor Author

In the web-client, we now get the following for a 3-stone game after handicap stones have been placed and 1 move by white:

Chinese (no change):
chinese

AGA (no change):
aga

Japanese (was 3, now skipped because 0):
japanese

New Zealand (was 3, now skipped because 0):
nz

This commit changes `GoEngine.computeScore()` to return `0` for `handicap` when
it does not contribute to the score.  Indirectly, this changes the scoring
summary in the web-client to not show the handicap in this case.

This fixes an inconsistency in the API/return, which forwarded to the
web-client and caused user confusion.

Previously it was neither consistently equal to the handicap (see AGA) nor
the handicap scoring bonus (see Japanese/Korean/New Zealand).

- For Chinese/Ing, it was both the handicap scoring bonus and the handicap,
  which happen to be equal.
- For AGA, it was the handicap scoring bonus, which is `handicap-1`.
- For Japanese/Korean/New Zealand, it was the handicap (scoring bonus is 0).

Now, for all rulesets, it's the handicap scoring bonus (if any).  For a client
computing the score, this is what's relevant.  This is similar to how `stones`
is returned for rulesets where it's relevant to the score.

It might be nice for the web-client to show the number of handicap stones
somewhere in the main view, but (a) it wasn't doing that previously and (b)
`computeScore()` isn't the right way to get that info.

Fixes online-go/online-go.com#1469
@dexonsmith dexonsmith force-pushed the computescore-handicap-should-be-handicap-bonus branch from 8524a18 to 7019ead Compare January 19, 2024 01:24
@dexonsmith
Copy link
Contributor Author

Oops; forgot to commit --amend after fixing a bug; force pushed to 7019ead, which should get the linter passing.

@pdg137
Copy link
Contributor

pdg137 commented Jan 19, 2024

This sounds great! Maybe the English on that line should say "Handicap bonus" to reduce confusion.

@dexonsmith
Copy link
Contributor Author

This sounds great! Maybe the English on that line should say "Handicap bonus" to reduce confusion.

Maybe... seems like it'd be better to consider separately, since the only user-visible changes in this commit are to hide the line when it doesn't apply. (In any case, that'd be a change in the web-client.)

@anoek
Copy link
Member

anoek commented Jan 19, 2024

Hmm, if I'm recalling correctly we had that in there for rules that it didn't matter because some vocal users wanted to quickly see the handicap stones given in the game.

@dexonsmith
Copy link
Contributor Author

Hmm, if I'm recalling correctly we had that in there for rules that it didn't matter because some vocal users wanted to quickly see the handicap stones given in the game.

Well, it's the wrong number for AGA, so it's not serving that purpose well. Maybe we can add the number of handicap stones somewhere?

dexonsmith added a commit to dexonsmith/online-go.com that referenced this pull request Jan 19, 2024
Adapt to Goban change in online-go/goban#146 and show
the actual handicap stones for black separately from the handicap bonus.

To make it clear that this handicap field is not part of the score:

- Use a `<hr/>` to separate it
- Add `+` in front of scores that are supposed to sum together.

Also move white's handicap bonus to the end, after komi.
- This means that there's always a `+` in front of it, even at the beginning of
  the game, so it's implicitly clear that there's a bonus.
- I considered changing the text to `Handicap Bonus` but that wrapped.

Relates to online-go#1469
@dexonsmith
Copy link
Contributor Author

Hmm, if I'm recalling correctly we had that in there for rules that it didn't matter because some vocal users wanted to quickly see the handicap stones given in the game.

Well, it's the wrong number for AGA, so it's not serving that purpose well. Maybe we can add the number of handicap stones somewhere?

See online-go/online-go.com#2507.

@anoek
Copy link
Member

anoek commented Jan 20, 2024

Thanks, good work @dexonsmith

@anoek anoek merged commit 42b864c into online-go:main Jan 20, 2024
1 check passed
@dexonsmith dexonsmith deleted the computescore-handicap-should-be-handicap-bonus branch January 20, 2024 17:57
benjaminpjones added a commit to benjaminpjones/goban that referenced this pull request May 2, 2024
Behavior changed in online-go#146 (Set 'handicap' from GoEngine.computeScore
to the handicap bonus)
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.

"score breakdown" incorrectly includes handicap points under Japanese rules
3 participants