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

Game history rengo merge #1691

Merged
merged 9 commits into from Feb 8, 2022
Merged

Game history rengo merge #1691

merged 9 commits into from Feb 8, 2022

Conversation

benjaminpjones
Copy link
Contributor

@benjaminpjones benjaminpjones commented Feb 5, 2022

Implements https://forums.online-go.com/t/rengo-status/40415/483

Proposed Changes

  • Refactor game_history_groomer: this function was really long (120 lines!), so I broke out some logical functions. It's still pretty long but now it's half the length and fits on one "page".
  • Make the normal columns handle rengo games (sort of)
  • Remove the rengo-specific table
  • Add some types to the Game History table

Notes

I could use some API support:

  • player lists or at least number of players for rengo matches
  • Fix the game result fields (black_lost, white_lost) i hear a fix is in flight :)
  • include the player whose page we are on in the game.players and game.historical_ratings (currently it just picks a player at random from each team?)

Screen Shot 2022-02-05 at 12 58 20 AM

(screenshot from Sophiam's page)

@benjaminpjones benjaminpjones changed the title Game history merge Game history rengo merge Feb 5, 2022
@GreenAsJade
Copy link
Contributor

I could use some API support:

  • player lists or at least number of players for rengo matches

I submitted a fix for this.

include the player whose page we are on in the game.players and game.historical_ratings (currently it just picks a player at random from each team?)

Pseudo random of course. I think it is the first players on each team :)

This looks to need some thought from anoek, due to gotchas (we only have those two player's full details at the time this is being serialised)

We could easily make it send nothing instead?

@benjaminpjones
Copy link
Contributor Author

  • player lists or at least number of players for rengo matches
    I submitted a fix for this.

Awesome, thanks!

We could easily make it send nothing instead?

If there are significant technical barriers, I suppose nothing is fine, but if we want to populate the rank column consistently, I think the player-of-interest needs to show up in the historical_rankings somehow.

Screen Shot 2022-02-05 at 4 12 15 PM

@GreenAsJade
Copy link
Contributor

GreenAsJade commented Feb 6, 2022

I have a path forward to deliver the historical rating for the person who's page we are on.

The weird thing is that this api also returns the ratings for the player of the "other" colour. In that case, I think I continue to return the "random" player in the "opposite" field - what else could it be?

Edit: there's a backend PR for this change now.

@benjaminpjones
Copy link
Contributor Author

I have a path forward to deliver the historical rating for the person who's page we are on.

Woo awesome! Thanks for the help with this, I didn't realize there was backend support needed when I started, but I think the changes you're adding will really make this sparkle :)

The weird thing is that this API also returns the ratings for the player of the "other" colour. In that case, I think I continue to return the "random" player in the "opposite" field - what else could it be?

For normal games, the ratings are shown in the opponent column so I think that makes sense. For rengo though, yeah I don't think I can logically show an "opponent" so random player (or no player) in that field is fine as far as the game history goes.

@anoek
Copy link
Member

anoek commented Feb 7, 2022

@benjaminpjones I just pushed up @GreenAsJade 's API support for you to beta so you can validate things are behaving as you expect and whatnot. I'm gonna mark this as draft, when you've banged on it against the beta API and things are working as expected mark as ready for merge

@anoek anoek marked this pull request as draft February 7, 2022 17:54
@benjaminpjones
Copy link
Contributor Author

benjaminpjones commented Feb 7, 2022

@anoek Sounds good! Just so I understand the backend changes, past games haven't been corrected right? I'll need to examine new rengo matches to see the changes?

Also, possibly related: when I go to beta.online-go.com I get a 500 error. The site still functions but I see the pop up when I visit the homepage.

EDIT: seems like 500 error for when I try to create/start a rengo match as well 😬

@anoek
Copy link
Member

anoek commented Feb 7, 2022

Sorry, beta should be fixed now, I forgot to run a migration.

I think the way @GreenAsJade wrote it, it should work for past games too.. but don't hold me to that :)

@benjaminpjones
Copy link
Contributor Author

Awesome, thanks! API looks good, I'll need to make some minor changes to get the new information to show up.

@benjaminpjones
Copy link
Contributor Author

Here's the table with those updates!

Screen Shot 2022-02-07 at 11 24 37 AM

I think the way @GreenAsJade wrote it, it should work for past games too.. but don't hold me to that :)

Regarding what was backfilled, it looks like rengo_(black|white)_team and the historical_rankings changes show up in old games, but (black|white)_lost still hold the old values (which is probably fine as long as new games are correctly populated).

@benjaminpjones benjaminpjones marked this pull request as ready for review February 7, 2022 20:37
@GreenAsJade
Copy link
Contributor

GreenAsJade commented Feb 7, 2022

... it's definitely the case that _lost is computed at game completion, so won't be fixed by the code change, but ought to be correct now for new games.

(Edit: actually, that's what I thought when I was writing it, but I just had a "wait a minute" kind of thought, so either I'll go back and look or we'll find out, whichever happens first :D )

Nice table :) I kinda liked "fun was had", in sending the right message, but I guess ... my fun was had :D

@GreenAsJade
Copy link
Contributor

^^ confirmed, it is "end_game_task", which runs once, that calculates and saves _lost.

@anoek
Copy link
Member

anoek commented Feb 8, 2022

Great work again @benjaminpjones :)

@anoek anoek merged commit 11eb14c into online-go:devel Feb 8, 2022
@benjaminpjones benjaminpjones deleted the game-history-merge branch May 3, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants