openscopeproject / TrguiNG

Remote GUI for Transmission torrent daemon
GNU Affero General Public License v3.0
334 stars 39 forks source link

Fix piece canvas scaling on HiDPI displays #119

Closed jpovixwm closed 10 months ago

jpovixwm commented 10 months ago

This fixes an issue with the piece visualization canvas where it wouldn't span the entire available space. Before (my window.devicePixelRatio is 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:

const getPieceLayout = (canvasWidth: number, canvasHeight: number, pieceCount: number) => {
    const ratio = canvasWidth / canvasHeight;

    let cols = Math.ceil(Math.sqrt(pieceCount * ratio));
    let rows = Math.ceil(pieceCount / cols);

    while (cols < rows * ratio) {
        cols++;
        rows = Math.ceil(pieceCount / cols);
    }

    return [canvasWidth / cols, rows, cols];
};

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.

jpovixwm commented 10 months ago

Not sure I follow this part, current algorithm also always uses entire width

I meant cases like this (2755x615 device pixels, 4229 pieces): image As you can see, there's a gap on the right, because a piece layout was chosen that spans the entire available height rather than width.

With the implementation in OP, it looks like this: image

There's no longer a gap, because the algorithm only targets the width of the canvas, and not the height (even if targeting the height would be more optimal in terms of the ratio of filled area vs empty area within the rectangle, with these specific inputs)

right and mid should be scaled to device pixels too

I assume you mean I should only add this in the initialization of right and mid, and I should leave the while loop body as it is? It didn't make much of a difference when I tried it, but maybe it's more obvious with some specific piece count and canvas size. Anyway, I can make this adjustment if that's indeed what you meant. (Edit: I can see this affects the maximum size of a single piece and it's apparent with smaller piece counts and bigger canvases)

qu1ck commented 10 months ago

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.

I assume you mean I should only add this in the initialization of right and mid, and I should leave the while loop body as it is?

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.

jpovixwm commented 10 months ago

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.