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

Add score lead line to winrate graph #823

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zogwarg
Copy link

@zogwarg zogwarg commented Sep 7, 2021

Early PR at this stage, would welcome some suggestions to make cleaner. and for how to do the i18n correctly.

Content of this PR:

  • When analyzing with katago, black's score lead is added to a node property SBKS <-- Welcoming a better suggestion for the node property name
  • Adds an toggleable option to display the score lead graph Show Score Lead Graph
  • Adds a blue line representing score lead as an overlay to winrate graph
  • Adds score lead in tooltip

Example image:
Screen Shot 2021-09-08 at 1 06 16

@zogwarg
Copy link
Author

zogwarg commented Sep 7, 2021

Apparently corresponds quite well to #755

@apetresc apetresc self-assigned this Sep 9, 2021
@apetresc apetresc added this to the v0.53.0 milestone Sep 9, 2021
@apetresc
Copy link
Member

apetresc commented Sep 14, 2021

Thank you for beginning work on this :)

Is this ready for review? I can't seem to get it to work - when I toggle the "Show Score Lead Graph", it just toggles the game tree. I haven't had time yet to try to diagnose anything, just my first attempt.

Nothing suspicious-looking in the Console.

Sorry, ignore the above. I think I just had a bad webpack cache or something

@zogwarg
Copy link
Author

zogwarg commented Sep 21, 2021

Hi,

It's a pleasure to contribute!
Are there big flagrant issues, or should there just be small corrections and esthetic changes ?

@kevinsung
Copy link

I've tested this and found some issues:

  • When the Show Winrate Graph option is unchecked, the entire graph disappears. I think the correct behavior should be to show the graph if either Show Winrate Graph or Show Score Lead Graph is checked. When only one of them is checked, only the corresponding data should be shown (the current behavior is correct when only winrate is checked but incorrect when only score lead is checked).
  • The current shade of blue is a bit difficult to distinguish from the background. I think the color should be made lighter. I suggest using #36f and #36d.

@kevinsung
Copy link

It's possible this is outside the scope of this PR, but the other features related to the win rate should be replicated for the score lead, namely, the displays in the graph showing the numerical value, as well as the difference bars in the graph. For the difference bars it might make sense to have an option to choose whether the bars correspond to win rate or score lead, and even to hide them altogether.

@kevinsung
Copy link

Actually, having all information simultaneously displayed might be too cluttered. Perhaps the graph should have information corresponding to only one of win rate or score lead at any given time, with an option to toggle between them. Then the same colors could be used for the lines and bars in either scenario.

@kevinsung
Copy link

This feature is a high priority for me so I went ahead and fixed the issues myself in #828 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants