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

GraphicsView overhaul #699

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

GraphicsView overhaul #699

wants to merge 16 commits into from

Conversation

jdpurcell
Copy link
Contributor

@jdpurcell jdpurcell commented Jul 6, 2024

Slightly less ambitious version of the graphicsview-rewrite branch. I actually started with that branch, made some minor fixes, squash merged everything down, removed anything related to constrained positioning, removed anything related to navigation/resize resets zoom, and then broke it back down into more manageable commits. In terms of operation/feel, this should function the same as currently, aside from Original Size which now simply sets the zoom level to 100% instead of functioning like a quasi-toggle / different mode (EDIT: And I can make it function like a toggle again if desired, I just wasn't sure if that was even desired per a past Discord conversation).

jdpurcell added 7 commits July 6, 2024 15:41
Ability to make the zoom level relative to physical screen pixels
Eliminates the need for overscan and ensures that no unnecessary scaling takes place when window matches image size.
Calculated aspect ratio was slightly off in some cases. Also enforce minimum window size in both dimensions.
* Original size can toggle between 100% and zoom-to-fit.
* Minimum window size just requires one dimension to be over the minimum.
@jdpurcell
Copy link
Contributor Author

My latest commit restores part of the "original size" toggle functionality, so that a user can press o repeatedly to switch between original size and fit modes. On my fork which not even many people use, I already got several requests to put the toggle behavior back, so I'll just leave that functionality and you can remove it if you don't want it. The only thing gone now is the "sticky" behavior of original size, i.e. how you could enable original size and then resize the window without it auto-fitting, and even after zooming in/out it's still stuck in that mode.

I also backed out one of my changes to the window sizing; it was trying to ensure that both dimensions are at least the minimum size, which is arguably correct, but ugly because it could leave borders in one dimension.

Both changes are controlled by hard-coded options in case you want to play around, otherwise feel to hard-code whatever logic you want and remove the constants.

Comment on lines -587 to -598
// Windows reports the wrong minimum width, so we constrain the image size relative to the dpi to stop weirdness with tiny images
#ifdef Q_OS_WIN
auto minimumImageSize = QSize(qRound(logicalDpiX()*1.5), logicalDpiY()/2);
if (imageSize.boundedTo(minimumImageSize) == imageSize)
imageSize = minimumImageSize;
#endif
Copy link
Contributor Author

@jdpurcell jdpurcell Dec 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was ancient code from before qView had configurable min/max window sizes; I think the minimum window size setting makes this obsolete in a lot of cases, plus Qt/Windows seems to enforce an absolute minimum anyway (I tested on the 5.15.2 build too) so this may have been working around a Qt bug that's since resolved.

@jdpurcell jdpurcell mentioned this pull request Dec 21, 2024
@jdpurcell
Copy link
Contributor Author

Most recent commit addresses a bug I found with expensive scaling. Details are in the comment in the updated code, but it was partially an existing problem that got worse without the extra safety margin. Basically if you were zoomed out, which I'll show in this extreme example to demonstrate, then when you reset zoom, the tiny image in loadedPixmapItem (before the scaling timer redoes it at the new size) is used for some of the calculations along the way which can introduce a massive rounding error.

Screenshot 2024-12-22 at 9 50 47 PM Screenshot 2024-12-22 at 9 50 53 PM

That was zoomed way out, then zoom reset, with 6.1 official release, leaving a large border on the side. I think I managed to fix it but I'll mark this as draft for a bit until I complete further testing.

@jdpurcell jdpurcell marked this pull request as draft December 23, 2024 03:17
Having it inverted is just another potential source of rounding errors.
@jdpurcell jdpurcell marked this pull request as ready for review December 26, 2024 05:04
Qt makes this difficult by not providing access to the native geometry.
Extend logical pixel "rounding" to handle case where menu bar is visible at top of window which influences the calculation.
@jdpurcell
Copy link
Contributor Author

jdpurcell commented Dec 30, 2024

The last few commits were dealing with a 1 pixel "border" (the image not touching the bottom/right edge, showing a tiny bit of the background) that could appear when display scaling is in use. This is really difficult to because Qt sizes everything in logical pixels; for example when setting the window geometry, it's always in integer logical pixels and thus impossible to set an exact desired physical size (without platform-specific code), and likewise when fetching the geometry, it's always in terms of rounded logical pixels. A few examples:

  1. In the window sizing code, say we want to resize the window to match an image that will be displayed at original size (100% zoom) with the "Zoom level is relative to screen pixels" option enabled. The image width is 52 physical pixels (ignore minimum window sizing for the sake of this example) and the display is configured for 125% scaling. 52/1.25 = 41.6 logical pixels. Qt only lets us use an integer size, i.e. 41 or 42. Maybe we can round up and go with 42? 42*1.25 = 52.5, which gets rendered as 53 physical pixels. But our image is only 52 (original size; stretching is not an option) which would leave a 1px border. Or if we go with 41 logical pixels, 41*1.25 = 51.25, rendered as 51 physical pixels, 1px gets cut off on the right. That seems like an okay compromise and better than blindly chopping off pixels from all sides whether we need to or not. In this example it ended up not being safe to round up, but that's not always the case; by intelligently choosing when to round (instead of floor in all cases), we can avoid some unnecessary cropping. This is the logic implemented by LogicalPixelFitter::snap.
  2. In the zoom to fit code, say the window size is smaller than the image and we need to pick an appropriate zoom level. Qt tells us the window is 43 logical pixels wide and 175% display scaling is in use. So take 43, divide it by the image width in logical pixels, and that should work right? Well it depends. If the window size was set by Qt to 43 logical pixels, 43*1.75 = 75.25, so it's 75 physical pixels. If we chose the zoom level based on 43 logical pixels, when painting to the scale set in the transform, it would try to hit 75.25 which gets rounded down to 75 and that indeed works. But what if the user resized the window and it happens to be 76px wide? 76/1.75 = 43.43, Qt rounds and still tells us it's 43 logical pixels. But we're only painting 75, and now there's a 1 pixel border. We'd actually need to know that 43 logical pixels is ambiguous and resolve it to the largest physical pixel value, which in this case is that 43.43 number we just calculated. Then when we calculate the zoom level based on that, when painting, 43.43*1.75 is the exact 76 physical pixels we need to avoid borders in all cases. This is the logic implemented by LogicalPixelFitter::unsnap.

I did a lot of testing and find it to work well on Windows, macOS, and KDE. I tested at 125%, 150%, 175%, and 200% (w/ macOS only the last once since it doesn't do fractional scaling). I believe it works exactly as intended when viewing images normally, and good enough when viewing images rotated (there's a tiny chance like < 1% in my testing that it will end up w/ a 1px border on an edge, probably only with fractional scaling). I tried hard to solve the rotation thing but no luck so far. EDIT: Works in all cases now!

@jdpurcell
Copy link
Contributor Author

The last two commits fixed the remaining 1px border issues and I re-tested everything thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant