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

GobanCore: round square size to device pixels instead of arbitrary px units #153

Closed
wants to merge 2 commits into from

Conversation

pdg137
Copy link
Contributor

@pdg137 pdg137 commented Feb 18, 2024

This uses the device resolution to round "squareSize" to the nearest units in device pixels. For example, on my 14" laptop screen, with a devicePixelRatio of 1.25, 1 pixel is actually 0.8px. So I get square sizes of 60, 60.8, 61.6, etc.

I have not looked into what the size is used for, so I don't really know if it's okay for it not to be an integer. But here is an example screenshot on Pixel 7 showing that it at least works in some sense:

Screenshot_20240217-183543

Here's what it looks like before the change, maybe there are slightly fewer artifacts but that could be an accident:

image

@benjaminpjones
Copy link
Contributor

benjaminpjones commented Feb 18, 2024

I tried to see if there's a noticeable difference with non-integer square size, and I am noticing some more artifacting on non-integer sizes. Here are some key numbers/display info for reproducibility:

  • devicePixelRatio: 2
  • display dimensions (achieved with responsive design mode): width=940, height=800
  • MacBook Air 15"

On Beta

  • square size: 28
Screenshot 2024-02-17 at 8 21 01 PM

Pixel-perfect lines (2px, uniform color):
Screenshot 2024-02-17 at 8 21 47 PM

On This PR

  • square size: 28.5

Breaks at the line mid points:
Screenshot 2024-02-17 at 8 26 24 PM

Aliasing of the lines (lines are about 3 pixels wide with the outer edges blended with the board color)
Screenshot 2024-02-17 at 8 27 17 PM

@pdg137
Copy link
Contributor Author

pdg137 commented Feb 18, 2024

Maybe we can fix these issues by rounding appropriately before drawing. I tried a little on the latest commit and got it to look kind of nice in the Chrome inspector:

image

And on a Pixel 6 with pixel ratio 2.625:

image

We can probably do better by making the line thickness be an integer number of device pixels and also extending the lines a bit where they don't join up very well in the middle of a square.

@pdg137 pdg137 marked this pull request as draft February 18, 2024 07:37
@pdg137
Copy link
Contributor Author

pdg137 commented Feb 18, 2024

Related:

https://forums.online-go.com/t/request-customize-line-width/50810

@benjaminpjones
Copy link
Contributor

benjaminpjones commented Feb 19, 2024

Took a look at the recent commit, and the lines look a lot crisper!

Noticing a couple more things that seem to happen with non-integer square sizes:

Star points

Circles are off center by 1px. This happens consistently, but is more noticeable on smaller screens. For example, iPhone SE (small screen, but dpr=2):

Centered (beta):
Screenshot 2024-02-18 at 10 24 51 PM

Uncentered (this PR):
Screenshot 2024-02-18 at 10 27 12 PM

Corners

Corners not aligned. This also happens consistently and is more noticeable IMO

Aligned:
Screenshot 2024-02-18 at 11 01 40 PM

Misaligned (happens on non-integer square sizes):
Screenshot 2024-02-18 at 10 14 29 PM
Screenshot 2024-02-18 at 10 14 36 PM

@pdg137
Copy link
Contributor Author

pdg137 commented Feb 22, 2024

Right, I didn't try to do anything with the star points or ends, so far just messed with the line positions.

If we want this to be better, I think someone needs to take a careful look over the whole drawing code and figure out what to do with all the coordinates in a principled way. One question is whether the coordinates even have to be in px? It might be more straightforward if we could draw everything using actual device pixels - but taking px into consideration as well.

For example, currently the line with is 1px. Maybe instead it should be 1px rounded up to the nearest integer number of device pixels.

@anoek
Copy link
Member

anoek commented Feb 22, 2024

Something else to throw out there, I've had it in the back of my mind to explore creating a SVG rendering backend instead of using canvas rendering. I think at this point the canvas based rendering isn't so necessary these days and there are some notable downsides and problems we run into, if an SVG based renderer is performant enough it'd likely solve all you're trying to change here.

@benjaminpjones
Copy link
Contributor

benjaminpjones commented Feb 22, 2024

I think SVG could be performant for sure. However I don't think it is straightforward to do the pixel-perfect calculations* that goban currently does. Is this something we can forget about, given that screens are much higher res than they use to be?

I think it would make for much simpler code and layout issues would no longer be an issue, but I did want to check because Goban code goes to great lengths for pixel precision.


*namely, calculations that attempt to solve these constraints:

  • lines should align with (display) pixels
  • squares and lines should have uniform widths

@pdg137
Copy link
Contributor Author

pdg137 commented Feb 23, 2024

There are still a lot of people on 1px = 1 pixel screens (I'm using one as I type this). At higher resolutions it might be okay to be a little blurry (like it is already) but it would be better if it weren't! And some of the tests I did here made it a lot worse, so it doesn't sound great to have less control.

@anoek
Copy link
Member

anoek commented Jun 14, 2024

Feel free to open this back up if you want to pick it back up again. It might be moot with the new SVG renderer, although the current plan is to maintain both renderers at least while we discover what browser kinks and problems we run into when we deploy the SVG renderer to everyone as the default, so it might still be relevant if we decide to keep the canvas renderer as an option or the default.

@anoek anoek closed this Jun 14, 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

Successfully merging this pull request may close these issues.

3 participants