ArneVogel / listudy

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

User generated arrows and circles #138

Closed olleeriksson closed 1 year ago

olleeriksson commented 1 year ago

Here is a draft release on this other feature I've been working on for showing arrows and circles from the PGN file itself. It's a draft because the code is not ready yet, it contains a bunch of commented out stuff and I haven't cleaned it up, and I don't even know if you want it. So don't look at the code yet! It's also probably lists all the changes from the other PR on max depth, since it's not in master yet. But I put it up here mostly so that we could talk about it separately from the other PR, and in case you wanted to play around with the feature yourself. If not, I also recorded a video where I describe the feature and sort of demo it. The video got slightly too large for GitHub so I had to put it on Youtube instead. It's 5 minutes long.

https://www.youtube.com/watch?v=UhIydhwAlBk

So, opinons?

image

ArneVogel commented 1 year ago

This one I see no fundamental problem in merging it. Though as you said, I think the interface could use some improvements.

I would probably want the automatic + pgn option to be default when it is merged. So I think it should be obvious what is happening.

  1. If I click on a piece the annotations they are removed, which I think is good. But when I click outside of the board without making a move the annotations do not reappear. This is also the current behavior, but since you are already looking into it could this be easily get added that the annotations reappear?
  2. The length of the highlighted arrow is currently shorter than regular arrows. They should probably be the same length excluding any highlighting which could extend the arrow
  3. As for the highlighting, I feel like the gray makes the color look "dirty". You said to maybe make it darker which I could see. Maybe also try a light/white/"shiny" highlight?
  4. Also could you try that the highlighting is equal width on all sides of the arrow head?

I also thought about maybe making the custom arrows one color to differentiate them but I think there is value in preserving the color, e.g., red = dont actually play this. Lowering the transparency on all of them would also probably be a bad idea so highlighting the actual hints is probably the best idea.

olleeriksson commented 1 year ago
  1. I'll see what I can do about that one. Of course there's always the "Show hints for this move" that can be clicked to get them to re-appear. But yeah, I'll look into it.

2 and 4: Unfortunately I don't think this is possible. I've tried and tried, but I'll try a little more. It seems to be a Chessground thing that arrows get shorter in length when multiple arrows are pointing to the same square. I don't remember if I said in the video, but it's actually not a pure outline, but just two arrows over one another. Same thing with the width of the outline. This just seems like the way arrows behave when you put one over the other. But I'll give it another go and see if I can figure something out. I can also give you some other examples, for example if you put the automatic (the old fashioned) arrows on top of the pgn (custom) arrows and make them really dark or bright or just a different color. But the problem then is that they look completely different from what you were used to seeing arrows like in LiStudy, or if you were to select Hint source: automatic. Another option would be to change the color of the old fashioned auto arrows to some completely different color like orange in combination with putting them on top of the new pgn arrows. Let me put together some examples for you..

  1. Agree. This was one of the hardest parts. Keeping the distinction between high transparency (played alot) and low transparency (played less) while also showing arrows of all colors. I will experiment some more. :)
olleeriksson commented 1 year ago

I've updated the PR to address your concerns and make some improvements.

I set out to explain the changes and all the edge cases with different colors and transparencies in a video, and it ended up 20 minutes long. I'm really sorry about that. But it really goes through everything you need to know. The last video is about what I had to do in terms of css to make it work, might be interesting to watch that one as well.

It's divided into 4 videos and this playlist contains them all: https://www.youtube.com/playlist?list=PLZGUk-_LFY97UyTTUKnc0gKufxtdoFE6F

Feel free to download the code and play around with it yourself to get a sense of the colors and transparencies and come with suggestions. But don't look at the code yet, it's not ready.

ArneVogel commented 1 year ago

Are the changes from the video pushed already? I am still getting the old arrows and "Hint Source" instead of "Hint Type".

Otherwise I seems way better now based on the videos.


One thought I had: text hints on the board to explain the style:

image

Unfortunately chessground does not have an api to create text annotations itself so we would have to add it. I can take the lead on that.

The image was created by adding <span>Playable<br>(outlined)</span> into the div with id chessground and the following css:

element {
    background: white;
    border-radius: 50%;
    position: absolute;
    padding: 10px;
    text-align: center;
    top: 286px;
    left: 325px;
    border: 1px solid black;
}

Since the chessground has a fixed width this should work fine.

Only problem right now: the span somehow prevents moves from being made but I will look into that as well.

olleeriksson commented 1 year ago

Ah stupid me! I had pushed to another branch. The changes are now available here.

The idea with text hints might work, as long as they only show up at the very beginning for the first x number of moves or something, just like your other text hints.

Another idea could be to make the background arrows even more transparent.

Let me know how it progresses and if want me to make any further changes on the visuals. Internally I have some refactoring to do before the code is ready even for review.

ArneVogel commented 1 year ago

I added a prototype TextOverlay. The basic usage is shown in study.js in the main. Its a prototype so if you need anything else from the API or anything else please let me know.

The idea with text hints might work, as long as they only show up at the very beginning for the first x number of moves or something, just like your other text hints.

The idea would be to show this the first time an arrow type is shown.

olleeriksson commented 1 year ago

I tried your overlay implementation.. I like the idea, but I am a little worried about opening this can of worm and getting every little detail to work. For example, there are some issues with how the coordinates are calculated, and also it doesn't scale or move when the board resizes. Of course everything can be fixed, and maybe it's worth it.

But just wanted to let you know that I found another way, to add a custom svg to a square using chessground's shape api. This works flawlessly out of the box, scaled etc, appears and disappears together with the arrows. Very easy to add, just like any arrow. The problem is you can't add text. Just wanted to hear your opinion before I move forward with one of the solutions.

For example it could look like this: image

This is a free svg file from the public domain (https://www.svgrepo.com/svg/198654/hand-gesture-hands-and-gestures). This would have to be complemented with a text description I suppose. Maybe it could even be combined with the svg as the icon, like so, but with a text that explains the arrows:

hand

Thoughts?

Which solution do you think you prefer?

olleeriksson commented 1 year ago

Here is a video:

https://user-images.githubusercontent.com/41130048/206920242-ea71bb21-41bf-42ec-b33d-063467864d97.mp4

olleeriksson commented 1 year ago

Here are a bunch of pictures of different options. Not all of these icons are licensed in the public domain, so we might have to check the license first but I'm sure it's possible to find something similar elsewhere if needed.

Same as above but with the icon better centered: image

image

image

image

image

I think I kind of like the last one, two different icons. One for moves played often, and one for moves played less often. I know the arrows don't match in the screenshot above (their outline are equally thick) but it's just because it's a test so I didn't implement it properly for the screenshot. But putting the icons on the destination square instead of the starting square as well. And then we could put both these two maybe in the info box to explain exactly what each arrow does. Keep it on for I don't know, 5 moves?

ArneVogel commented 1 year ago

Keep it on for I don't know, 5 moves?

I thought just having it shown only one move would be enough.

I like the idea of the svgs, but I would want the interaction to be beyond obvious. And I am not sure thats possible without the text overlay.

olleeriksson commented 1 year ago

And can I ask what it is you want to communicate the most.

1) That a move is playable at all? 2) The distinction between a more frequently played playable move (thinner outline) and a less frequently played playable move? 3) Both?

Is one more important than the other? It's just good to know so that I know where I should focus more.

olleeriksson commented 1 year ago

I've made some progress on the arrow first time hints. Everything is checked in so you can download and play around with it if you want. I've also prepared a video where I demo the hints. I would appreciate some feedback on whether you think I am on the right track so I know if I should pursuit it more. And maybe if you want to answer the questions above?

VIDEO: https://www.youtube.com/watch?v=ni0MH_gtaV8

So, I kind of like the speech bubbles. I agree they are more clear. There is some work left on making sure they are positioned correctly maybe. Sometimes they cover arrows. It's going to be hard to cover all the situations but there are some improvements to be made. Also, when you click on the options and clear the line etc it doesn't reset the speech bubbles. I will look into that more, but this is just so I could demo it for you.

What I am most interested in hearing is if you think I am providing the right types of hints. It's pretty tricky because the things we want to give hints to come appear in all kinds of different orders. You might have 10 arrows on a page, in which case you would get 10 speech bubbles for example. Or the first PGN arrow appear exactly on the same move as the first thinner outlined arrow appear. Right now I am only showing one type of speech bubble and associated info box hint at the same time because I feel like it's clearer this way. Not too much to read at the same time.

Note! If you check out the code, all you have to do is reload the page and it resets the arrow hints.

So, am I on the right track? Anything jumps out at you that we should do differently?

ArneVogel commented 1 year ago

And can I ask what it is you want to communicate the most.

1. That a move is playable at all?

2. The distinction between a more frequently played playable move (thinner outline) and a less frequently played playable move?

3. Both?

Is one more important than the other? It's just good to know so that I know where I should focus more.

Point 1. is more important to me. You could use the interface just fine not knowing 2. at all but trying to play move indicated by arrows from the pgn would feel irritating I feel like. This just gave me another idea: How about additionally, when we detect that a pgn arrow way attempted to be played, we show some error message.

Playing around with the interfaces with key move on and off I clearly prefer the textual label on the board.

image

This hint is probably not needed. The hints should probably only be shown when the arrow could be mistaken for a move, i.e. the arrow starts at one of your own pieces.

So from the video:

image

  1. Should be shown
  2. Not needed
  3. I wonder if there should also be a hint for the playable moves
  4. I like the additional hint

With those thoughts: image

Moves 1. or 2. either one, but only one, show the PGN hint Move 3. shows a playable hint

I said that "The distinction between a more frequently played playable move (thinner outline) and a less frequently played playable move" was not that important to me but I still like having it when there is place for it. image


So with those thought how about these rules:

(1) IF a pgn arrow does not start at at a playable piece THEN ignore it for the other rules (2) IF there are only playable arrows THEN no hints (3) IF there are (multiple) playable moves and (multiple) pgn arrows starting at playable moves THEN show one hint for the playable moves and one hint for pgn arrows (4) IF (multiple) special arrow for a already played move is shown THEN give one hint on one of the special arrow (5) IF hints for rule (3) are shown THEN (4) is not applied on that move but only on the next time (4) applies [with rule (6), (4) will not get "blocked" again] (6) IF (3) or (4) already showed a hint in the current session THEN they dont apply again

So each situation gives only once a hint, and we limit the number of textual hints to two. Playable move hints get precedence over try other variation hints.

Obviously these rules are not set in stone so if you have any thoughts or idea I would be happy.

olleeriksson commented 1 year ago

Working on it.. :)

olleeriksson commented 1 year ago

I've pushed my recent changes so you can download and play around. I've got nothing on my to do list right now, so apart from fixing some probable bugs, the behavior is ready for testing. When the behavior is ok I will do some code refactoring, but I'm waiting for your approval before making those final changes.

Two videos to explain the changes: https://www.youtube.com/playlist?list=PLZGUk-_LFY96KxlJgY8wGYD92iC2qGey2

Note! Pay attention to my slight modifications to point no 3. See description below and in the video.

Other things:

olleeriksson commented 1 year ago

I'd say I have refactored the code now and checked it and would say it's ready for "almost final" review. Of course you might have opinions on the arrow hints still, but unless they are large changes, I would say you can look at the code and build the code and see if I've missed some bugs or anything.

~NOTE! There is one more thing to change though, and that's making the arrow hint suggestions appear only one time and not every time you reload the page. But that's it as far as I know.~

Some details:

olleeriksson commented 1 year ago

_(Or if you want to turn it on yourself you can look inside the function setup_arrow_overlays(). It has three calls to another function seensuggestion() which takes two parameter. By changing the second parameters in those three locations (line 286, 292, 312) from false to true you turn on making arrow hints a one type thing.

Ready for review and testing!

ArneVogel commented 1 year ago

Looking good. I think this is ready to merge. So just one final commit needed, the switching of the production flag :+1:

olleeriksson commented 1 year ago

Nice!

Actually there is no more production flag that has to be re-enabled. I talked about it before because I had made the overlays once=false for the overlays before. But now that the logic controlling when the overlays is shown is broken out and placed in TextOverlayManager I saw no reason to even have a once-flag there since all overlays are always once=true, so the only thing TextOverlayManager.seen_overlay(overlay_id) looks at is localstorage, which means there is no flag to change.

I tried explaining it in the Note! part in https://github.com/ArneVogel/listudy/pull/138#discussion_r1055976758 but I realize I was a bit unclear.

On Thu, Dec 29, 2022 at 10:33 AM ArneVogel @.***> wrote:

Looking good. I think this is ready to merge. So just one final commit needed, the switching of the production flag 👍

— Reply to this email directly, view it on GitHub https://github.com/ArneVogel/listudy/pull/138#issuecomment-1367188218, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJZZQQGKKBYEOPHM64L3PJTWPVLGFANCNFSM6AAAAAASTAEEQU . You are receiving this because you authored the thread.Message ID: @.***>

ArneVogel commented 1 year ago

Turns out I don't have my ssh keys with me right now. I will release the change when I get back home around the 9th of january.

olleeriksson commented 1 year ago

Turns out I don't have my ssh keys with me right now. I will release the change when I get back home around the 9th of january.

Alrighty. :) No worries.

ArneVogel commented 1 year ago

Published now :)

olleeriksson commented 1 year ago

Nice! :)

On Thu, Jan 12, 2023 at 7:38 PM ArneVogel @.***> wrote:

Published now :)

— Reply to this email directly, view it on GitHub https://github.com/ArneVogel/listudy/pull/138#issuecomment-1380850176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJZZQQEGU2OEWFKJ3R2F2WLWSBFRDANCNFSM6AAAAAASTAEEQU . You are receiving this because you authored the thread.Message ID: @.***>