-
Notifications
You must be signed in to change notification settings - Fork 87
Fix piece canvas scaling on HiDPI displays #119
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
Fix piece canvas scaling on HiDPI displays #119
Conversation
qu1ck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scaling fix is good, just need to apply it to the binary search starting point too.
The simpler algorithm you showed is fine but there needs to be a limit on the square size, huge ones just look silly past some point. Although I would argue binary search is also as simple as it gets :)
I removed the part where it tries to fill the whole height, because I think it looks better in the UI if the squares always fill the entire available width.
Not sure I follow this part, current algorithm also always uses entire width (unless there is only 1 row)
src/components/piecescanvas.tsx
Outdated
| if (rows * size < deviceHeight) return [rows, cols]; | ||
| else return [-1, -1]; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right and mid should be scaled to device pixels too
|
Ah, yes the gap is because current algo always chooses largest possible square size (or 20 css pixel max) that would fit all pieces in the rectangle, even if it's not an even divisor of width.
Yes, only in init, this limits upper size of squares. With your scaling I imagine 20px would be quite small. I don't oppose to your algo, it does look a bit neater if there is no gap on the right, but it should have max size cap like described above. |
|
I think the max size can be capped like this: const getPieceLayout = (canvasWidth: number, canvasHeight: number, pieceCount: number) => {
const ratio = canvasWidth / canvasHeight;
let cols = Math.ceil(
Math.max(
Math.sqrt(pieceCount * ratio),
canvasWidth / toDevicePixels(20),
),
);
let rows = Math.ceil(pieceCount / cols);
while (cols < rows * ratio) {
cols++;
rows = Math.ceil(pieceCount / cols);
}
return [canvasWidth / cols, rows, cols];
};Appears to work fine with the limited testing I did. Can you give it a go too? In case I missed something. |
fdadcdc to
6c3d33d
Compare


This fixes an issue with the piece visualization canvas where it wouldn't span the entire available space.

Before (my
window.devicePixelRatiois 2.5):After:

Tested on Windows 10 as a native app and in Brave and Firefox in Windows/Android when used as a web interface.
Also, it seems that the piece layout can be calculated with a simpler algorithm found here: https://math.stackexchange.com/a/2570649
The code can be adapted into something like this:
I removed the part where it tries to fill the whole height, because I think it looks better in the UI if the squares always fill the entire available width. Only thing missing is an upper limit on the returned square size, which I think is present in the original implementation.
Let me know what you think.