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

Scoring features to mark dead stones and count territory #6

Merged
merged 43 commits into from
Apr 16, 2012
Merged

Scoring features to mark dead stones and count territory #6

merged 43 commits into from
Apr 16, 2012

Conversation

dexonsmith
Copy link
Contributor

I've tested this with my own SDK, but you should test it too before putting it live. A few things to note:

  • The code is probably a little rough. I tried not to do anything too ugly, but we may differ on what that means. If you want me to clean things up before you pull, just let me know (but you'll have to tell me what you don't like).
  • I left behind a README.deadstones that I used for planning (and updated as my plans changed). Feel free to delete it, but for now you may find it helpful to figure out the control flow.
  • The new images are placeholders. I'll be disappointed if they go live :). My thoughts: "territory-black" should be a small solid black circle in the centre of a transparent image and "dead-white" should be a small solid black circle in the centre of a white ghost stone. In both cases, the grid background underneath should be (partially) exposed. But it doesn't matter, as long as they're clear and they aren't as ugly as the placeholders.
  • Old unfinished games should end up going through scoring, just like new games, when the second player passes.
  • Old finished games will remain unmodified, and no winner is declared (even if one of the players resigned).

awong-chromium and others added 30 commits January 17, 2012 11:15
Adding support in the game board for marking owners.

 - Using already existing images to mark territory.  These are not
   really appropriate, but will allow me to play around.

 - The image names *are* good... but they are just copies from other
   images.
Added a document planning out sections of the dead stone removal.
Cleaned up the wording, especially around the territory algorithm.  Also
updated the state_string handler in go.js to match the document.
Now there is support in the python code for the new states.
Started making real changes to the code to handle scoring.
This is just a first draft, and is probably full of syntax (and other)
errors.
The information per player may not be specific to a game in the future.
Further, Player is accessed via memcache.  It seems much cleaner to
store this information directly in the game.
First draft of algorithm to figure out territory that changes hands.
This algorithm is probably good enough now until I find bugs in it.

 - Renamed it to compute_changed_coordinates, since it includes
   coordinates that do not have stones.

 - Using a new class BoardArray as a helper to perform the DFS.  This
   just wraps a python array object to provide a 2D interface.

 - Wrote GameBoard.is_japanese_corner_case to handle the special case in
   Japanese rules.
They were in the middle of old related functions before.
Fixed a number of minor errors.

 - Forgot to use self._has_owners instead of _has_owners.

 - Added getters and setters (and the variables themselves) for
   GameState.white_territory and GameState.black_territory.

 - Added GameState.compute_territory(), and functions in GameBoard to
   make it happen.
Fixed a javascript error
Now clicking on the board actually makes things happen.  Lots of bugs
still, though.
Now dead stones are *not* necessary to recognize territory.

 - On the second pass, runs a new GameBoard.mark_territory method that finds
   the initial territorial boundaries.

 - Documented the meaning of variables called "color", since it was getting a
   little confusing.

 - Simplified the functionality of GameBoard.is_japanese_corner_case().

   - Now it only takes the colour of the potential territory.

   - The caller is now responsible for only calling it when relevant.

 - NOTE:  I did not test this change at all; the python probably doesn't even
   run.
I wasn't able to test 3935b9b before;
now the syntax (and other) errors it introduced should be fixed.
Almost every case goes as expected now.

 - Now GameBoard.compute_changed_stones just looks for stones made dead
   or alive.

 - GameBoard.mark_territory is called every time now.  Its algorithm is
   more consistent.

 - Removed the Japanese corner case code; I'm not sure it was quite
   right.  In particular, it's important that false eyes on the edge
   count as territory, and I'm not sure they did.
Cleaned up the algorithms and made their behaviour consistent.

 - Not going to reimplement special Japanese dame cases.  If taking away
   an opponent's liberties will force him/her to fill in, then the
   player should do that.  Special handling of dame ala KGS can be
   implemented later.

 - If every stone on the board is marked dead, then the board resets.

 - More careful about letting a stone get marked as owned by its own
   colour.
Things are not quite working here, but they may be getting closer.

 - start_scoring does not seem to be working in the javascript.

 - _done_ does not seem to be working either.
Wrote some notes to help with UI state changes, and fixed a couple of JS
bugs.
Notes on client-side scoring may be easier to use now.
Added a little more hierarchy to make it easier to read.
Updated much of the interface to deal properly with starting to score.
Added notes about updating _scoring_number_.
There are only three known bugs remaining:

 - The new images look like junk.  Someone with any experience making
   images can correct that quickly.

 - The territory text does not appear automatically when an open page
   transitions to scoring.

 - There's something wrong with the transition from "you_are_done" to
   "game_finished"... i.e., the page never seems to get the memo from
   /has-opponent-scored/.
I have fixed all the bugs I know about, except for the ugly images.
However:

 - This could use some testing from others.

 - I wasn't too careful about coding style.
@davepeck
Copy link
Owner

Still haven't had a chance to review this. Sorry @duncanexonsmith for the crazy slowness on my end.

@davepeck
Copy link
Owner

Took my first real pass on this this afternoon. Lots to look through in more detail, but first impressions: this is amazingly awesome work!

@davepeck
Copy link
Owner

@duncanexonsmith I've pushed the new version (6-dead-stones) live, but have not made it "production" yet -- I've got a way to go before I'm ready to do that. But, that said, you can play with it against the live database here:

http://6-dead-stones.davepeck-go.appspot.com/

I've also added you as a Developer to the app engine app (davepeck-go) itself, so if you want to play with things you can look at logs, etc. for the service. Obviously please don't go deleting user data. ;-)

Cheers,
Dave

@dexonsmith
Copy link
Contributor Author

@davepeck Sounds great. I'm glad there's a way to push it live without making it production. I'm very busy with work right now, but I hope to get a chance some time this week to have a look.

@dexonsmith
Copy link
Contributor Author

@davepeck I was playing through a 19x19 game on 6-dead-stones, and I hit a surprising error around move 257 in "/service/make-this-move/":

RequestTooLargeError: The request to API call datastore_v3.Put() was too large.

Have you ever seen this before? If you want to have a look, run SELECT * WHERE ANCESTOR IS 'agtkYXZlcGVjay1nb3INCxIER2FtZRjhl4wBDA'.

current_state is under 4K. My best guess is that the addition of the owners array in the state makes Game.history too large, but that seems strange. Any other ideas?

EDIT: Yes, that's it, actually. There's apparently a 1MB limit per put. 4K*256 == 1MB, so along with the other data, my <4K hits the limit. See http://stackoverflow.com/a/3068371

EDIT2: This shouldn't be too hard to fix. GameState.owners is only needed for the final state. It can probably be left uninitialized for all the states in Game.history.

GameState.owners almost doubled the storage cost of GameState.  Since
a single Game contains all the states in Game.history, this means that
long-running games cost too much to store (i.e., surpass appengine's 1MB
limit for Game.put()).

This change delays the creation of GameState.owners to the final state,
where it's actually used.
@dexonsmith
Copy link
Contributor Author

@davepeck I did a quick check of 84e6703 (local 10 move game) and didn't seem to break things. Let me know when you've had a chance to push it live (maybe 7-dead-stones?) and I'll start a new test game.

Added some code to check the `current_move_number`, guaranteeing that
stale games do not submit moves.

 - Added `GameController.current_move_number`, and did what was
   necessary to keep it up-to-date.

 - Changed frontend code to submit `this.current_move_number` to the
   handlers.

 - Changed the handlers to check `current_move_number` against
   `Game.get_current_move_number()`.
@dexonsmith
Copy link
Contributor Author

Just a note about a1b3ffd and eaf3467: these are fixes for issue #7.

@davepeck
Copy link
Owner

davepeck commented Apr 1, 2012

...and then my entire life went insane. I'm still here, @duncanexonsmith -- just got snowed under by my day job, a tiny little app I maintain called Cloak -- but slowly digging out from underneath.

@dexonsmith
Copy link
Contributor Author

Work happens... I figured you'd come back eventually.

@davepeck davepeck merged commit eaf3467 into davepeck:master Apr 16, 2012
@davepeck
Copy link
Owner

What! @duncanexonsmith -- bet you thought I'd disappeared. But I just merged this and pushed the entire thing live. It's amazing work and DESERVED TO BE USED rather than sit around languishing in my push inbox.

Thanks so much for this major new advancement to the site. In addition to life insanity, this was a big enough set of changes that I felt I had to really pour over the commits. But they look great; I really like how you kept the same interaction style throughout.

I know you mentioned replacing the temporary graphics -- well, I haven't had a chance, but I have a pretty good feel of what I'd like 'em to look like -- so, stay tuned for that. In the mean time... thanks again.

Cheers,
Dave

@dexonsmith
Copy link
Contributor Author

Thanks, @davepeck.

I've been travelling, but finally had a chance to try it out live just now... and I've already found a bug (forgot to add komi!). Have a look at #8 when you get a chance... it's just a one-liner!

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.

3 participants