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

Stones are slightly smaller than grid, not touching - esthetics #110

Closed
swooboo opened this issue Feb 15, 2023 · 13 comments
Closed

Stones are slightly smaller than grid, not touching - esthetics #110

swooboo opened this issue Feb 15, 2023 · 13 comments

Comments

@swooboo
Copy link

swooboo commented Feb 15, 2023

The stones are not touching, which is not esthetic IMO, and many players I talked to agree with me:

image
image

I believe these lines are related, with the radius of the arc being less than what's supplied at line 48:

goban/src/themes/disc.ts

Lines 45 to 52 in b1313e4

ctx.arc(
cx,
cy,
Math.max(0.1, radius - lineWidth / 2),
0.001,
2 * Math.PI,
false,
); /* 0.001 to workaround fucked up chrome bug */

Not sure whether the lineLength is 0 here (might be set to 0 if too small, line 37), but maybe nothing should be subtracted, which should make the lines overlap. This happens in Firefox and Chrome, in both "flat" mode (no shading / drawings in stones) and also in the regular mode. I also briefly checked the mobile view in Firefox and it's the same there.

Now that I think of it, the code I referenced is probably only used in the 'flat' mode.

This might be a nitpick, and I still think it justifies the fix.

@anoek
Copy link
Member

anoek commented Feb 15, 2023

If someone takes this on just make sure there's not artifacts left over as the board state changes (most easily observable by moving the hover stone around the board or going forward/backward through the game). If I recall correctly this was dialed in to what it is because there were artifact problems, though a lot has happened since then. It might be a quick fix, and it might be surprisingly in complicated.

@swooboo
Copy link
Author

swooboo commented Feb 21, 2023

I have some findings, only in the "Plain" theme, after some playing around. Changing to lineWidth/4 in the mentioned code aligns the stones correctly. So from this:

image

It became this:

image

But starting with lineWidth/5 and below, there are artifacts after removing the stones, as suspected by @anoek (bottom left):

image

My guess is that there was a miscalculation of how much to subtract from the radius to account for the width of the outline, or maybe the deletion of the pixels isn't big enough when removing the stones. Either way on small enough boards there are still one-pixel gaps often (you'll have to zoom in):

image

@swooboo
Copy link
Author

swooboo commented Feb 23, 2023

After some more playing around I found out that even lineWidth/4 causes the artifacts, although the board needs to be pretty big for it to be noticeable. I guess the GobanCanvas::__drawSquare() method doesn't clear a big enough area when removing a stone.

To me it looks like the whole board is drawn on a single canvas, while juggling with deletions / restorations. Is there a reason that a canvas per layer was not used? It's pretty easy to stack several canvas elements together and use them as layers. Several layers I could think of:

  1. Lowest layer - Goban wood, grid, hoshi marks; shouldn't change much / at all
  2. Stones layer - only black and white stones. Maybe even 2 layers, 'checkered' pattern division, then the clearing of the stone can be done with a bigger square without touching any nearby stones because they're in the other 'checkerboard' layer
  3. Markings / annotation / numbers layer - topmost layer for text and markings

This does look like a pretty big overhaul though, I do believe that in case of this specific issue it can be solved in an easier way.

@anoek
Copy link
Member

anoek commented Feb 23, 2023

Re multiple layers, it has been many years now but once upon a time I experimented with that and chose a single canvas implementation for lines and stones for performance and memory usage reasons. Mainly the issue was when viewing lots of games on the overview screen or observe games screen.

That said, a lot of optimizations and device improvements have been made since then, so I'm not sure if that's still valid or not.

@swooboo
Copy link
Author

swooboo commented Feb 23, 2023

I didn't think about performance... Can you point me where I can read more, or maybe explain here? Is 1 canvas really that "heavy"? Also something I didn't think about is how hard it would be to make sure they always are on top of each other exactly, in every view configuration...

@anoek
Copy link
Member

anoek commented Feb 23, 2023

Is 1 canvas really that "heavy"?

No it's not, and on the game page we do use different layers (in specific a layer for the shadows and for the pens). However the problem we ran into (8-10 years ago now) was when folks were viewing 100+ game thumbnails, so adding another layer doubles the amount of ram and composition work needed. On those screens, it's 100+ extra canvases not just one. Of course mobile devices are a lot more powerful now and the long tail of ancient PC's we support are also more capable now, and browsers have made improvements all around, so it may not be an issue anymore, I'm not sure. That and we have the table view for when those game thumbnails are too much for devices.

All that said, I don't actually think the layers thing would solve the issue you are seeing. Right now we have the wood layer (implemented as a background image iirc), then the lines and stones are drawn on one layer, then shadows on another if applicable, and pen marks if applicable. But the artifacting seen above is all on the stone/line layer, in specific part of a stone isn't getting cleared when the draw radius exceeds the clear bounding box.


Idea for a solution:

I think the way to solve this is to make a clipped region larger than the stone we're redrawing, clear that whole region, then draw the up-to four surrounding stones and the center stone. The clipping will make it so drawing the surrounding stones won't redraw outside the clipped/cleared box so only the portion of the stone that was cleared will be redrawed, and the center stone can be drawn partially over the other stones so they can touch.

Furthermore, we could, if we wanted to, then draw all of the shadows on the same layer, eliminating the need for the shadow layer entirely. Currently we have shadows on a separate layer because shadows tend to go outside of the stone areas, so we offset what we clear / draw on that layer. However if we switch to using clipping we can eliminate that layer and just make sure our clipping region extends far enough to include the shadow. That'd also enable us to have some richer shadows, there have been some complaints about our shadow quality in the past but we've been limited because of how we handled clearing / redrawing.

@swooboo
Copy link
Author

swooboo commented Feb 26, 2023

I like your idea. I've been giving this thing some shower thoughts, and I had a similar idea, but it was just straight up redrawing every square surrounding the removed stone, which would be wasteful since we only need to redraw a little and not the whole square. This might have performance and wastage issues when removing large number of stones - removing a 10x10 group of stones will redraw 4*10*10=400 'residues', with many being common, while we only need to redraw the 40 'residues' on the capturing boundary. To be fair though, the situation is rare enough to ignore probably.

Regarding layers fixing this issue, I partially disagree. Suppose we split the stones layer into two, using checkerboard pattern (the rule is square at (i,j) belongs to layer (i+j)%2). Now, when we want to erase a stone, we can use a larger square without touching any of the surrounding stones: the orthogonal adjacent ones can only be in the other layer, while the diagonal adjacent stones are not large enough to fill the whole square so even a slightly larger square won't reach the circle that's diagonal from the removed stone.

That being said, I don't know which solution is better, while leaning towards your one.

EDIT: I misunderstood your redraw idea. My original idea is pretty much the same, redrawing the adjacent places. What I thought it was instead, was that we could somehow redraw only the portions of the stones that needed to be redrawn, and not the whole stone.

@anoek
Copy link
Member

anoek commented Feb 26, 2023

10x10 group of stones will redraw 41010=400 'residues', with many being common, while we only need to redraw the 40 'residues' on the capturing boundary. To be fair though, the situation is rare enough to ignore probably.

It might be fine to not get too fancy with that, but redrawing large numbers of stones actually happens a lot when you're skipping around in the move tree, so not just dealing with captures but skipping from say the start to the end of a game, you'll commonly redraw 200+ stones, so it might be worth some consideration.


The checkerboard idea is an interesting one. It seems simpler than dealing with clipping and drawing surrounding stones.

I do worry a bit about memory, particularly on iOS devices as they tend to be stingy with memory and have been problematic in the past when trying to allocate what I would consider to be a reasonable amount of canvases. But I 'd say so long as they can see a low amount (say 8-9?) at a time I think it's fine, problematic devices can use the table view beyond that.

@swooboo
Copy link
Author

swooboo commented Feb 27, 2023

On the other hand, we only clean the bigger box if we remove a stone and not when placing a stone, and only redraw the adjacent locations that actually have a stone. So if I understood the mechanism correctly, there should almost no duplicate redraws, and in case of the 10x10 example it would indeed redraw only 40, because the inner 100 locations are all empty now and should not be redrawn.


Regarding memory issues - do we have a way to measure this? Like statistics of device resources?


What's the best way to progress in your opinion? I like both approaches and maybe implementing redrawing first, since it seems simpler to me, to then see whether the layering will work well.

@anoek
Copy link
Member

anoek commented Feb 27, 2023

Regarding memory issues - do we have a way to measure this? Like statistics of device resources?

There's not an exposed JS api to inspect those details but in the chrome and firefox (and probably safari) developer tools you can inspect memory usage.

image

What's the best way to progress in your opinion? I like both approaches and maybe implementing redrawing first, since it seems simpler to me, to then see whether the layering will work well.

I think I lean towards redrawing as well, so if you like that better too then 👍 . That said, if you get into it and it's problematic then laying should be good too I'd think.

@swooboo
Copy link
Author

swooboo commented Feb 27, 2023

Played around with the code a little bit, and with the redraw idea I got this:

image

You can check out the branch and the diff here - main...swooboo:goban:fix-stones-too-small

Of course it probably needs more work so it's just a start for now. Also while this works with the plain theme, the rendered theme always puts at least 1 pixel between the stones, and I can't figure out why...

@anoek
Copy link
Member

anoek commented Feb 27, 2023

the rendered theme always puts at least 1 pixel between the stones, and I can't figure out why...

Hmm, so it's been a long time since I wrote that, but as I recall basically I create a canvas, draw a plain circle, then do my own coloring of all the pixels that were colored in. So if there's always a pixel between, I might suspect the base circle is drawn a pixel too small, but that's just a suspicion, it might be some other reason entirely.

@anoek
Copy link
Member

anoek commented May 21, 2024

Fixed in #159

@anoek anoek closed this as completed May 21, 2024
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

No branches or pull requests

2 participants