ArneVogel / listudy

Listudy - chess training server
https://listudy.org
GNU Affero General Public License v3.0
292 stars 45 forks source link

Option to set max-depth #137

Closed olleeriksson closed 1 year ago

olleeriksson commented 1 year ago

image

Here is another option, to control the max depth.

I added both a range input control and a number and a + and a - next to it. I felt it made it much clearer what the range was for. The max depth chosen is saved in local storage per study id and chapter index. And if any of the options are lost it reverts to defaults. Same if the repertoire should happen to be smaller than the saved value, which I am not sure could happen.

What do you think? You didn't say anything about the screenshot in the last PR so I figured you'd be ok with this one.


Also, next, maybe the final thing :) I am working on is reading user created arrows and circles from the study and displaying them. As you might be aware a user can create these themselves and choose among these four colors, at least on Lichess. And I think ChessTempo and ChessBase follows the same format for storing these in the PGN. Of course, there would have to be some way to decide whether to show these user created arrows vs the auto-generated once based on upcoming possible moves. Maybe an option, haven't tried that yet. But, just to give you a heads up, and give you the opportunity to come with suggestions if you have any, or object if you don't like it.

image

On the picture above the e2e4 arrow is an autogenerated one because that move is in the repertoire. I experimented with changing the color of these two a grayscale but I don't know. Also, whenever there is a user generated arrow between the exact same squares that there is a move on, I think you would want the user generated arrow to take precedence, if you were even combining both in the same view. The option could be, 1) user arrows, 2) auto arrows, 3) user + auto arrows. Maybe even rename the existing arrows option into "hints" instead. Just some ideas that have gone through my head.

ArneVogel commented 1 year ago

I don't really get the use case. I feel like users shouldn't care about that aspect.

Some css issue:

image

olleeriksson commented 1 year ago

I think it's a great feature that some people may really want. :) But that's just my personal opinion. Yours is the one that matters.

My use case is this. When I add a new opening to my repertoire, I study a new opening, watch videos and read up on it, and then I start populating my repertoire with moves. Usually create a pretty deep line for the main line, deeper than I know that I will learn at the initial stage. Other sub lines I make shallower. With this option, I don't have to think so much about limiting the number of moves when I am in the process of studying the opening because I know that I can just set the limit later when training.

Also, you could start by training through your repertoire at a low depth level, and then increase it over time as you learn your repertoire better and better. Also it would allow you to decide per session to for example train just the very first opening moves, or train deep. Playing around with this option I found it very useful. But of course if you think it's unnecessary or clutters the interface you don't have to accept it. But, if you can accept the option showing, then you can just leave it at the default which is to set max depth to the full depth of the current tree. Of course, one could also hide away more advanced options in a section that collapses. But I don't know if we're there yet. And I think it kind of shows off some of the capabilities of the training tool.

I think I fixed the css issue by the way.

olleeriksson commented 1 year ago

Fixed a bug I found in the code.

ArneVogel commented 1 year ago

The css issue is fixed for me.

Ok I thought some more what my problem is with this option and I think its just that I don't understand it.

Even with your explanation I have to think quite a lot to get a grip on it. So the UI, would need to change. Could you come up with a very short and distinct description, e.g., "lower = less repetition" for the feature? Maybe this could then get shown somewhere when the values are changed to make it clear what is happening.

olleeriksson commented 1 year ago

How about this hover title for the option: "Lets you choose how deep in the repertoire to train. The highest value you can choose is the depth of the repertoire (in moves). Set to a lower value to train against a more shallow subset of the repertoire, and to the highest value to train against the full repertoire."

The name of the option could also be changed to "Training depth:" or maybe just "Depth:". I like "Training depth" but it probably requires us the widen the space for the options slightly, which could be a good thing anyway.

Excluding the widening of the option space, it would look something like this:

image

ArneVogel commented 1 year ago

I think having the units would be good. So "... depth: 3 replies" and "... depth: complete study" for the max setting.

olleeriksson commented 1 year ago

I thought of another scenario where I think it would be a useful option. Imagine someone who downloads a repertoire or purchases one through a website. Maybe the repertoire is way to deep for the person. With this option he/she could easily decide to train only a part of it. Also, the opening trainer at chesstempo.com has an almost identical option, and they call it Maximum learning depth. Screenshot at the bottom.

Here are some samples according to your thoughts. I changed replies to moves because if you're white the first move is not a reply, and I think the term "move" is very well known in the chess world and it's something you come across quite quickly. Also, because there's just not going to be enough space to fit in everything I moved the slider to a second row. Not sure what you think about that. The other option is to let it extend the option and have the next one move over.

image

image

I am not sure if "complete study" is a good word because it's not a measure of depth. Perhaps "max" would be a better word? I had that there before when I was experimenting with this, but I changed it to the number itself because I felt I was curious in knowing the actual number of moves the AI would ask me to play.

Also, I don't know if you noticed but in the current implementation the number changes when you change chapter. So if you've got the slider all the way to the right, the number will always tell you the number of moves of the longest lines. I agree it's not instantly obvious, but I think you will realize that pretty soon. Also, since the slider is at the maximum I think people will understand that the number you see is the maximum.

Another drawback of sometimes having a number and sometimes a label ("max", "complete study") is that you will get a lot of white space between the number and the slider if you make the width absolute, and if not the slider will jump back and forth. So that's another reason why I went with only a number and not a unit.

image

Third option is to put the number to the right of the slider, but I tried that too and I think the association between the label "Training depth" and the number itself is a lot less clear:

image

Here is a screenshot from Chesstempo:

image

So, I really think that the way it is now is the best compromise. Maybe changing the name to "Training depth: ", with no unit, and maybe with this addition in the hover title it should be clear to everyone what this is. But just changing the name to "Training depth" will mean that the first column of options (and thus all columns) will need to be made a little wider.

"Lets you choose how many moves deep in the repertoire to train. The highest value you can choose is the depth of the repertoire (in moves) and is updated when you change chapter. Set to a lower value to train against a more shallow subset of the repertoire, and to the highest value to train against the full repertoire."

What do you think?

ArneVogel commented 1 year ago

I don't agree that having the hover will help people understand. First I think very few people actually know that hovering will show the help text and second on mobile there is no way to show it at all.

I reason for suggesting the "complete study" (or some equivalent) was the thought about having a study with a 25 move line. I would assume it the slider goes in interval of ones that when you go from N-1 to max that it would still limit the study to N where N<25, thus you would think the lines are still cut off.

I still think without a unit it would be confusing. But im also not sure how to solve the size constraint. I would have to think about it some more.

olleeriksson commented 1 year ago

Here is a little video just to show how it works.

And again, just to clarify, the default is always the full depth, so unless you change the slider you always play full depth. Is that what you are worried about? That people might accidentally touch the slider, and not realize they did it, and wonder why the lines are cut off?

https://user-images.githubusercontent.com/41130048/205738578-d4c2a72b-f519-4ec8-9295-f3a7700b7857.mp4

olleeriksson commented 1 year ago

If that is your concern, one thing that could be done is to not remember the setting, and just always set it to the max depth when you change chapter or open the study.

olleeriksson commented 1 year ago

Or you could whenever there is anything other than the max depth selected highlight the control with some color so that it's obvious that it's engaged.

I think if I understood better exactly what it is you're worried about I might be able to come up with something. But in the end, if you don't like it, we can also just skip it altogether. I just saw that you had already prepared for setting the max depth in the code and felt it was a pretty natural to make it available for everyone. But, don't be afraid to say we should skip it.

ArneVogel commented 1 year ago

I like the idea of highlighting the setting if it is engaged. Maybe just underlining the setting would be enough?

Otherwise I would feel comfortable merging.

olleeriksson commented 1 year ago

Here are some changes. Mostly some code improvements (refactored), a bug or two.

And...

I've made so that the option turns red when it engaged (meaning it's set to anything but the full depth). I tried changing the background but that was tricky with both dark and light mode. Underline didn't look very good. And bold meant to text jumped back and forth. What do you think?

I also:

ArneVogel commented 1 year ago

Looking good now :+1:

Just requested one final change and then I can merge.

olleeriksson commented 1 year ago

Alright... there!

ArneVogel commented 1 year ago

Thanks a lot!