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

Use different color to indicate move blunder in variation window #389

Closed
wants to merge 11 commits into from

Conversation

phiming
Copy link

@phiming phiming commented Oct 6, 2018

#383
Use different color to indicate move blunder in variation window.
Add concentric circles display for nodes with comments.
21

@zsalch
Copy link
Contributor

zsalch commented Oct 7, 2018

@phiming

@OlivierBlanvillain has added a auto formatter plug-in into the pom.xml in latest branch 'next'.
It is recommended to commit the formatted code again.

@OlivierBlanvillain
Copy link
Contributor

@phiming It's a nice idea, but I think it makes more sense to display this information alongside the winrate. Indeed, at any point in time we can only see a small subset of the nodes in the variation window, whereas the winrate graph shows an overall view of the game. So my suggestion would be to close this in favor of #384.

@featurecat
Copy link
Owner

no I like this idea. why not display it?

@featurecat
Copy link
Owner

can I have an explanation of the colors meanings? I don't get why there is black

@phiming
Copy link
Author

phiming commented Oct 8, 2018

@zsalch
It seems this function will not be merged very soon, so if you need it right now, you may get it from my homepage Please let me know if any question.

@phiming
Copy link
Author

phiming commented Oct 8, 2018

@olivier
Sorry for I use phone to type, cant type too much.
some people like complicated thing and want to master everthing, some like simple and easy thing. though we cant see all blunders in variation window, it should be enough for some one. so, why not to keep it. frankly, i think its a little crowd and complicated to dipslay all blunders in winrate window.

@phiming
Copy link
Author

phiming commented Oct 8, 2018

@featurecat
I opened an issue to get some suggestion.
currently, black means deadly blunder. its not final decision, any suggestion will be considered.

@featurecat
Copy link
Owner

what about red orange yellow? you could even make it a smoother gradient.

@zsalch
Copy link
Contributor

zsalch commented Oct 8, 2018

@phiming
I think this feature is still good, There are a few things to improve:

  1. Now the 'next' branch has changed a lot, so the commits are too different now. It is recommended to rebase the current commits and only submit the difference from the latest next;
  2. as the comment of , the node display is not smooth enough, of course, the current version may also have this problem, will better if it can be improved;
  3. Thresholds for different colors can be configured by the user, such as:
         "blunder-thresholds": [2, 6, 10, 30]
         If this PR is approved, then can consider adding color settings to the theme in the future.
  4. The node with comments feels that the color is more conspicuous than the concentric circle.

@OlivierBlanvillain
Copy link
Contributor

I'll take care 1.; merging next into this branch.

@OlivierBlanvillain
Copy link
Contributor

By the way, why is this PR based on the "Use mouse to jump to any node(position) in variation window"? If it doesn't have to it would be better to make independent commits, because as it stands we will have to wait for:

Before even starting to review the code...

@phiming
Copy link
Author

phiming commented Oct 8, 2018

@zsalch, for
1, I will rebase it. Though everytime I was painful with so many conflicts. It ruined funs of programming.

2, 4, I am not good at UI designing, in fact, this feature needs only a little coding, but the ideas is nice, at least for me. You are more experienced at UI designing, and also have good program skill. You may change the code as you like, I would be very glad for your help.

@OlivierBlanvillain
Copy link
Contributor

@phiming You can also merge if rebasing is too painful (and it probably is here).

@zsalch
Copy link
Contributor

zsalch commented Oct 11, 2018

@phiming
I'll try to make it better.

@zsalch
Copy link
Contributor

zsalch commented Oct 19, 2018

@OlivierBlanvillain @phiming
I have tried to do:
image

Config:
"blunder-winrate-thresholds": [-30,-20,-10,-5,5,10],
"blunder-node-colors":[[255,0,0],[0,255,0],[0,0,255],[255,255,0],[0,255,255],[255,0,255]],

What do you think, what suggest? I always feel that the current node is a little bit strange.

@zsalch
Copy link
Contributor

zsalch commented Oct 20, 2018

@phiming @OlivierBlanvillain @featurecat

With comment node:
image

This effect does not look good.
Perhaps using a color marker blunder is not a particularly good choice because the current node and the node with comments have to use other schemes.
Anybody have any suggestions?

@featurecat
Copy link
Owner

I think this effect looks good to indicate comments. for blunders did you try experimenting with less saturated colors? something like that?

@zsalch
Copy link
Contributor

zsalch commented Oct 20, 2018

Allow users to specify the blunder color, like [255,0,0] or [255,0,0,200].

@zsalch
Copy link
Contributor

zsalch commented Oct 31, 2018

Reopen #413, closed this.

@zsalch zsalch closed this Oct 31, 2018
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.

4 participants