Fix size/alignment of compare view selection rectangle
ClosedPublic

Authored by huoni on Apr 9 2018, 3:29 AM.

Details

Summary

There was inconsistency between the rects of:

  • Raster images in Zoom to Fit
  • Raster images in arbitrary zoom
  • SVG images
  • The selection rectangle drawn in DocumentView

This inconsistency was only apparent when drawing the selection rect
in View compare mode, resulting in it being drawn 1 pixel off at
some zoom levels/modes but not all.

The cause of this inconsistency was due to how the original QRectF of
the image was rounded to a QRect. Calling QRectF::toRect takes both
the point and the size into account, meaning the size will be rounded up
or down based on how the point was rounded.
The solution was to keep consistent rounding in RasterImageView,
SvgImageView, and DocumentView. using QRectF::toRect in all places
was impossible because RasterImageView resizes mCurrentBuffer
independently to drawing it at the image offset (point and size rounded
independently). Therefore I've changed everything to match.

In effect, this patch:

  • Makes the selection rect cleaner by drawing it at an integer rect
  • Removes the gap between image and selection rect
  • Fixes any inconsistency in the drawing of the selection rect

BEFORE

AFTER

Test Plan

Open an SVG and raster image in compare mode, and enable solid colour background.
Inspect the selection rect for both types of image, at lots of different zoom levels,
including Zoom to Fit mode, and manually zoomed all the way out.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
huoni requested review of this revision.Apr 9 2018, 3:29 AM
huoni created this revision.
huoni edited the summary of this revision. (Show Details)Apr 9 2018, 3:33 AM
huoni added a project: Gwenview.
huoni added a comment.Apr 9 2018, 3:36 AM

I'm not sure if my comments sufficiently explain the reasoning. Would welcome feedback, especially since it feels necessary to explain things in multiple places.

Works good and the frame itself is drawn perfect. :)
But I would prefer keeping the gap because the frame is nearly invisible if the image has similar colors.
I think the summary is detailed enough and I can follow the explanation. ;)

huoni added a comment.Apr 9 2018, 10:38 AM

But I would prefer keeping the gap because the frame is nearly invisible if the image has similar colors.

Fair point, though @rkflx may not agree - D11942#240855
Personally I don't mind either way, as long as it's consistently drawn properly :)

I think the summary is detailed enough and I can follow the explanation. ;)

Glad to hear it!

rkflx added a comment.Apr 9 2018, 11:30 AM

But I would prefer keeping the gap because the frame is nearly invisible if the image has similar colors.

Fair point, though @rkflx may not agree - D11942#240855
Personally I don't mind either way, as long as it's consistently drawn properly :)

Let's optimize for the common case: Every image has the gap, but only very few images have similar colours. Also, a proper fix for this would be to alter the frame, i.e. use a different coloured line or add a shadow. As you can see in my screenshot, this is already the case (not sure why this is missing in Huon's screenshot, probably because of the background colour?).

huoni added a comment.EditedApr 9 2018, 11:45 AM

Also, a proper fix for this would be to alter the frame, i.e. use a different coloured line or add a shadow. As you can see in my screenshot, this is already the case (not sure why this is missing in Huon's screenshot, probably because of the background colour?).

The rect has always been 2 pixels thick. In your screenshot, it has been drawn at non-integer coordinates (in both axes; my before screenshots show this in one axis). Since the patch converts everything to integer rects, it's now always drawn as a solid 2 pixel wide line.
Unless I misunderstand you?

I could test out drawing the rect with different colours? E.g. 1 pixel black (inner), plus 1 or 2 pixels with theme color.

huoni added a comment.Apr 9 2018, 11:52 AM

Or another idea - force it to be drawn 1.5 pixels away from the image, resulting in @rkflx's screenshot but without the gap. That could help with when the image is a similar colour.

rkflx added a comment.Apr 9 2018, 12:08 PM

The rect has always been 2 pixels thick. In your screenshot, it has been drawn at non-integer coordinates (in both axes; my before screenshots show this in one axis). Since the patch converts everything to integer rects, it's now always drawn as a solid 2 pixel wide line.
Unless I misunderstand you?

Ah, that explains it. Drawing snapped to the pixel grid obviously makes more sense, so my screenshot showed buggy behaviour.

I could test out drawing the rect with different colours? E.g. 1 pixel black (inner), plus 1 or 2 pixels with theme color.

Not sure if this will work. Basically this should be consistent with what is used in Browse mode, but I also found the approach used there a bit too "heavy".

I'd keep it simple and consistent (which possibly means changing Browse mode), I don't think the highlight colour is too commonly found in images to pose a real problem. Unless we have some non-fabricated example images where the current approach clearly fails, I'd say this isn't worth the added complexity.

Or another idea - force it to be drawn 1.5 pixels away from the image, resulting in @rkflx's screenshot but without the gap. That could help with when the image is a similar colour.

Nice trick, but this will bite us on HiDPI. IIRC those frames are also one of the problems the current HiDPI patches still have in Browse mode.

Seems it's a combination of my background color and the selection frame color. Using a white background it looks totally different - probably there's no perfect color.

rkflx added a comment.Apr 9 2018, 12:15 PM

Uh, that looks horrible. What colour scheme are you using?

Also, this might not be about "colour" after all, but more about how light or dark the image and/or the background is, i.e. this might be about contrast. Maybe the frame just needs to use two very differently lighted colours (in either a 2px or 3px width configuration).

What colour scheme are you using?

I'm using the Steel scheme.
Just tried Standard - the selection frame looks more 'glowing' here and I think that's ok for most cases. ;)

rkflx added a comment.EditedApr 9 2018, 12:41 PM

What colour scheme are you using?

I'm using the Steel scheme.

…which also did not look too good before the patch. I'd say we keep this patch focused on removing the gap (which is clearly a problem), and then (if there is interest) in a new patch or task we could figure out some new frame designs:

  • consistent for Light table and Browse
  • works good in HiDPI
  • compatible with dark and light images
  • looking good on any background

The current Browse mode frames already seem to contain considerations for these points, maybe cleaning them up and applying them everywhere will be sufficient.

rkflx accepted this revision.Apr 9 2018, 11:41 PM

Okay, now looked at your Diff in more detail. Really nice analysis, and very interesting that it turns out to be more than just a "gap"! Code LGTM. Tested successfully with a raster image containing a 1px border, as well as with all animation modes.

For SVGs depending on the zoom level I still get some displacement, however this is also visible when viewing an SVG in single mode, i.e. without a frame. Might be some rounding problem related to where the SVG renderer is placed, so unrelated to this patch but still kinda annoying:


That said, I was a bit concerned about how rounding to integers will affect HiDPI mode. For example, if you set 1.4 as a fractional scaling factor and point KMag at the hover frame of a toolbar button, you'll notice that it is drawn with antialiasing, while in normal mode it is snapped to the pixel grid.

In a sense snapping to integers in normal mode in your patch is fine, but I wondered whether antialiasing can still happen in HiDPI mode like it is supposed to. Thus, I rebased the HiDPI branch on your patch:

git checkout gsoc2017_hidpi_support
git rebase --onto arcpatch-D12059 master
# solve merge conflict

After a cursory look everything still seems to work as before (ignoring pre-existing issues). Nevertheless, I'd appreciate more eyes testing in that regard…

This revision is now accepted and ready to land.Apr 9 2018, 11:41 PM
huoni added a comment.Apr 9 2018, 11:44 PM

The current Browse mode frames already seem to contain considerations for these points, maybe cleaning them up and applying them everywhere will be sufficient.

To be honest, I have no issue with the current Browse frames. I would be happy to try and replicate that here.

works good in HiDPI

What does this involve exactly? Just work in full pixels, and use a design that scales to a higher resolution well?

rkflx added a comment.Apr 9 2018, 11:54 PM

The current Browse mode frames already seem to contain considerations for these points, maybe cleaning them up and applying them everywhere will be sufficient.

To be honest, I have no issue with the current Browse frames. I would be happy to try and replicate that here.

I don't see any grave issues either, but looking at them with KMag, they are quite complicated, which probably adds to their "smudgy" look. IIRC when working on D9078: HiDPI fixes for thumbnails in gwenview, there were some problems with those frames, i.e. with fractional scaling they would lose even more of their "sharpness" (besides some alignment/off-by-one issues…).

works good in HiDPI

What does this involve exactly? Just work in full pixels, and use a design that scales to a higher resolution well?

Well, with fractional scaling it should look somewhat reasonable ;) Not sure yet how this translates to pixels or even code… In general I'd assume that simpler frame designs will scale better.

For SVGs depending on the zoom level I still get some displacement, however this is also visible when viewing an SVG in single mode, i.e. without a frame. Might be some rounding problem related to where the SVG renderer is placed, so unrelated to this patch but still kinda annoying:

In your screenshot, you can see the border is correctly drawn next to the background. It's the SVG itself that seems to be unaligned. I will look into this, and probably submit another patch.

That said, I was a bit concerned about how rounding to integers will affect HiDPI mode. For example, if you set 1.4 as a fractional scaling factor and point KMag at the hover frame of a toolbar button, you'll notice that it is drawn with antialiasing, while in normal mode it is snapped to the pixel grid.

In a sense snapping to integers in normal mode in your patch is fine, but I wondered whether antialiasing can still happen in HiDPI mode like it is supposed to. Thus, I rebased the HiDPI branch on your patch:

git checkout gsoc2017_hidpi_support
git rebase --onto arcpatch-D12059 master
# solve merge conflict

After a cursory look everything still seems to work as before (ignoring pre-existing issues). Nevertheless, I'd appreciate more eyes testing in that regard…

Anti-aliasing still works (the selection rect is drawn with anti-aliasing, as shown by the corners), I'm just making sure everything is drawn snapped to the nearest pixel. This was already effectively the case for raster images (ignoring the zoom to fit scaling), since mCurrentBuffer is a QPixmap which must be constructed with an integer size. This makes sense, because we want the image to be drawn faithfully, and not have the edges have different rendering just because it's positioned at fractions of pixels.

The current 2 pixel rect doesn't seem too bad when scaled to 1.4x. It definitely loses its consistency, but I don't think anything will be pixel-level consistent at fractional scaling.

A larger and thicker rect would help offset the issues perhaps.

huoni updated this revision to Diff 31783.Apr 10 2018, 12:44 AM
  • Also make sure SVGs are drawn at integer coordinates

In your screenshot, you can see the border is correctly drawn next to the background. It's the SVG itself that seems to be unaligned. I will look into this, and probably submit another patch.

Actually, I just forgot to also make sure the SVG was using integer coordinates, so I fixed it in this patch.

rkflx added a comment.EditedApr 10 2018, 11:52 AM

Yay, works much better now. I still get some overflow on the right edge, though. Perhaps you also need to adapt the size?

huoni updated this revision to Diff 31803.Apr 10 2018, 12:33 PM
  • adjust SVG size so it never goes outside background

Yay, works much better now. I still get some overflow on the right edge, though. Perhaps you also need to adapt the size?

Well I managed to ensure the SVG never draws outside the background or overlap the selection rect, but this results in sometimes it leaving a small gap. Given the way I've had to calculate the scale, I'm not sure there's an easy way to fix this.
The problem is that the scale calculation I believe effectively rounds down, whereas toSize rounds to nearest int. I can make the background drawing match the SVG scale calculation, but the selection rect is still a problem. Maybe the only choice is to change rasters to always round down as well.

I'll look into this further tomorrow).

rkflx added a comment.Apr 10 2018, 5:18 PM

Well I managed to ensure the SVG never draws outside the background or overlap the selection rect, but this results in sometimes it leaving a small gap.

In my testing, it worked almost perfectly for an SVG where I aligned the content (a rectangle ;) to the pixel grid in Inkscape, i.e. I could not reproduce the gap you got. Given that, your patch is a huge improvement over the current state and thus is worth landing in any case.

Where I still got problems was when I let the rectangle extend generously over both the left and right side of the document (black rectangle on the top), which led to Gwenview drawing the rectangle 1px wider than the transparent background (yellow) (apart from that, clipping to the document size worked fine):

Might as well be a totally different topic, and certainly extending content beyond the document size is not that common.

huoni updated this revision to Diff 31845.Apr 10 2018, 11:16 PM
  • Fix some SVGs painting outside their bounds

Where I still got problems was when I let the rectangle extend generously over both the left and right side of the document (black rectangle on the top), which led to Gwenview drawing the rectangle 1px wider than the transparent background (yellow) (apart from that, clipping to the document size worked fine):

Might as well be a totally different topic, and certainly extending content beyond the document size is not that common.

Didn't even realise that was possible, but is easily fixed by clipping drawing. KColorPaint seems to do it this way (draws nothing outside the document shape), but I'm worried this is an unfaithful representation of the SVG.
I'm not sure what is typical in this situation.

Oh and yes, D11942 is still necessary after this latest change, I checked :)

So that leaves us with only one minor problem - some SVGs will have a gap between image and edge of background at some zoom levels. As much as I'm a perfectionist, I think it's is acceptable, unless you think it's worth changing everything to round down as described above?

Examples of this:

This is the SVG with the issue:

Didn't even realise that was possible, but is easily fixed by clipping drawing. KColorPaint seems to do it this way (draws nothing outside the document shape), but I'm worried this is an unfaithful representation of the SVG.
I'm not sure what is typical in this situation.

Awesome, can confirm the fix. Both Firefox and Chromium clip to the document size, so we should be fine. I suspect reading the SVG spec would lead to the same conclusion, because what else would the document size be specified for…

Oh and yes, D11942 is still necessary after this latest change, I checked :)

Good for you ;)


So that leaves us with only one minor problem - some SVGs will have a gap between image and edge of background at some zoom levels. As much as I'm a perfectionist, I think it's is acceptable, unless you think it's worth changing everything to round down as described above?

I now see it for my SVG too, with your patch the gap is even worse, I suppose because now it snaps differently to the grid… For reference, I used this file:

The problem is that the scale calculation I believe effectively rounds down, whereas toSize rounds to nearest int. I can make the background drawing match the SVG scale calculation, but the selection rect is still a problem. Maybe the only choice is to change rasters to always round down as well.

Could you change the scale calculation to effectively round to the nearest integer instead of always rounding down?

Another way which made it work good enough for me (i.e. the best among all iterations we tried so far) was reverting your introduction of SvgImageView::setScale (which is a bit brittle anyway), while keeping ItemClipsToShape. I never got a clear gap, only in very few situations there was some antialiasing over the background. Could you try that? If that worked for you, that would be my preferred option.

Otherwise, if you don't fix the problem now, you should at least open a new bug with the example file, steps to reproduce and a screenshot of the issue, so it's not forgotten.

huoni updated this revision to Diff 31942.Apr 12 2018, 12:12 AM
  • Remove setScale to rely on ItemClipsToShape
huoni added a comment.EditedApr 12 2018, 12:26 AM

Another way which made it work good enough for me (i.e. the best among all iterations we tried so far) was reverting your introduction of SvgImageView::setScale (which is a bit brittle anyway), while keeping ItemClipsToShape. I never got a clear gap, only in very few situations there was some antialiasing over the background. Could you try that? If that worked for you, that would be my preferred option.

This technically clips up to 1 pixel of the actual SVG, but I still agree it's the best solution.
I realised that QSizeF::toSize can round each axis in a different direction, effectively changing the aspect ratio slightly so that no matter what scale we set the SVG to, we get a gap on either the right or the bottom.
Again, we could probably fix this by changing everything to always round up or always round down. But since we need ItemClipsToShape anyway, and it will at most clip 1 pixel, it's probably not worth the "brittleness" of all that manual rounding.

huoni updated this revision to Diff 31948.Apr 12 2018, 1:04 AM
  • Remove leftover include

Looks excellent - aside from my color scheme... ;)
Tested with different raster and svg files, including these special ones posted above and found no issues. :)
Also tested the HiDPI patch but I'm not sure if I should see any difference without the corresponding hardware and/or system config?

rkflx accepted this revision.Apr 12 2018, 1:14 PM

Okay, great. Let's commit this before we find even more problems…


This technically clips up to 1 pixel of the actual SVG, but I still agree it's the best solution.

Hm, hard to reproduce for me. For my test SVG I could not get the red border to be clipped, even when trying with other sizes (i.e. different to 180px width).

I realised that QSizeF::toSize can round each axis in a different direction, effectively changing the aspect ratio slightly so that no matter what scale we set the SVG to, we get a gap on either the right or the bottom.

Yeah, better not to fiddle with the aspect ratio.

Also tested the HiDPI patch but I'm not sure if I should see any difference without the corresponding hardware and/or system config?

It's a common misconception that you need HiDPI hardware to test this. In fact, HiDPI displays make it harder to spot issues, because everything will be so tiny ;) You can simply use QT_SCALE_FACTOR=1.4 gwenview to test with 1.4x scaling (but with other scaling factors other issues might come up, so when doing HiDPI work, often more extensive testing is required).

Anyway, for this patch we already determined that rounding to integers won't negatively affect the ability to scale up, so it will be just fine.

This revision was automatically updated to reflect the committed changes.
huoni added a comment.EditedApr 12 2018, 11:27 PM

This technically clips up to 1 pixel of the actual SVG, but I still agree it's the best solution.

Hm, hard to reproduce for me. For my test SVG I could not get the red border to be clipped, even when trying with other sizes (i.e. different to 180px width).

That's good! What I meant was, some scale factors cause up to 1px of the SVG to be drawn outside the bounds. That is getting clipped now. So e.g. if the border happens to be 10px wide, and we scale so it's trying to render 10.4px wide, that 0.4px might be getting clipped. That's if I'm understanding it correctly. All in all, not a major issue, since using my example, the original 10px is still being shown.