Clean up ScoreEstimator.ts

[benjaminpjones]

I've wanted to play with ScoreEstimator for a while, but there is a lot going on in there, and it's not easy to test due to external dependencies on OGSScoreEstimator (c++ WASM lib) and the remote AI scorer. I figured the best way to approach this is to clean it up as much as I can in one pass and manually test all changes together. That said, let me know if there are commits that aren't desired, it shouldn't be too hard for me to remove them.

Motivations

• Make score estimator more approachable so we can experiment:
  • interest in a new scoring tool expressed here
  • I'd like to try a less aggressive metagroup algorithm (Score estimator connects groups that should not be connected online-go.com#1670)
• As a follow-up, I'd like to make the local estimator replaceable
  • This will make testing easier
  • Will allow experimentation with JS-only implementations

Observations

• There is duplicate code for calculating groups (see GoMath.GoStoneGroups)
• Score estimator is mostly used indirectly through methods in GoEngine/GobanCore
  • The methods construct the estimator, then call a single method
  • exception: the estimator is emitted, and the UI Game class accesses the winner and amount
• Many properties appear to be unused

Assumptions

Please let me know if these are invalid. If you have code snippets on external usages, I can create additional unit tests to help capture the behavior (similar to the "amount and winner" test).

• Server is not using WASM anymore
• Server uses GoEngine functions to access the ScoreEstimator, not accessing the estimator directly

Changes

See individual commit messages - I tried to make these as atomic and self-explanatory as possible.

Testing

• Manual client testing
  • [remote] Click estimate score on the current user's finished game
  • [local] Click estimate score on the current user's ongoing game
  • [remote] Click estimate score on an ongoing game (as an observer)
  • [remote] Finish a game, mis-score, auto-score
• Unit tests
  • created ScoreEstimator.test.ts
  • 25% increase in line coverage for ScoreEstimator.ts

[anoek]

Awesome, this is some pretty dusty code, and you'll make a lot of people happy (myself included) if you can make it less dumb (but not too smart either!).

Regarding the not too smart part - when I introduced the KataGo backend the community basically said it was too smart, which is why we still have that WASM backend for live game estimates, so that's something to keep in mind when pondering your JS backend and for revising things to be less dumb.

Less dumb is good, too good at the game is bad, but if we can work towards some sweet spot in-between that's great :)

Anyways, I always love your refactors, thanks for taking this one on!

[anoek]

These are still used on the backend, ogs-node repository

[anoek]

At this point it might make more sense to have a separate type for the score estimate requests sent off to the server that include these, I'd be OK with that too depending on how much you want to change. It's convenient to have it somewhere in the goban repository that way both the UI and the server side can use it, although on the flip side maybe it still makes a bit of sense too to have a common ScoreEstimateRequest structure depending on which backend we're using, the WASM module, the server KataGo system, or the newfangled JS backend you have in mind.

[benjaminpjones]

Thanks, and added back in! I don't really have a preference regarding separate types or a unified type, but I'll keep it as-is in this PR to minimize downstream churn.

[benjaminpjones]

Thanks for reviewing! Definitely makes sense to want a "dumb" in-game estimator. The algorithms I'd like to experiment with are geometric and have no knowledge of Go (variations on the Voronoi algorithm from this thread), so I don't think they should be perceived as too smart at all.