GNOME Bugzilla – Bug 17904
Off-by-one in selection boundary preview
Last modified: 2009-08-15 18:40:50 UTC
Package: gimp Version: 1.1.24 Sometimes a selection or layer boundary gets mis-calculated: the actual layer is one pixel wider or taller (or both) than the displayed border. This leads to pixels being left behind when you move the layer. This is a display artifact only. I've only ever seen pixels left to the right edge or on the bottom (or both). This happens either when zoomed out, or (as in my original investigations) when a layer has been scaled down (my tests were at 50% reduction). My guess is that the layer display and mask calculation code is rounding image pixels to the nearest integer display pixel, but the boundary calculation is truncating away the fractional display pixel portions, so in some cases display code rounds up while the boundary code rounds down, leaving a 1 display pixel gap between the two. Since the boundary is used to generate redraw events, they are not redrawing quite enough. However, the rounding up/down/to nearest int stuff that happens in gdisplay.c:gdisplay_{un,}transform_coords() is reasonably subtle. Sometimes you really do want to round to the nearest int, sometimes you just want to truncate. Careful though is needed before changing these functions, since it is used in a very large number of places. Someone needs to go through every single use of these translate functions, to make sure they're being used sensibly :( Austin ------- Bug moved to this database by debbugs-export@bugzilla.gnome.org 2001-01-28 10:55 ------- This bug was previously known as bug 17904 at http://bugs.gnome.org/ http://bugs.gnome.org/show_bug.cgi?id=17904 Originally filed under the gimp product and general component. The original reporter (austin@gimp.org) of this bug does not have an account here. Reassigning to the exporter, debbugs-export@bugzilla.gnome.org. Reassigning to the default owner of the component, egger@suse.de.
Re-assigning all Gimp bugs to default component owner (Gimp bugs list)
Apparently related: Select 1:1 view, klick in the image and move the mouse one pixel to the right and one pixel down. The resulting selection is 1x1 pixels although the area covered by the mouse hotspod is 2x2 pixels. This also goes for wider rectangles (in 1:1 view): The selection ends a the pixel to the top left of the final mouse-position. This is especially odd, because the preview-frame of the selection always goes exactly to the mousepointer. So if you have a rectangle on the screen and want to select it exactly you have to drag your mouse from the top left pixel to the pixel to the bottom right of the rectangle although the (xor'ed) frame tells you otherwise. One slight problem: If we change this it is not possible to select 1x1 pixels (in the 1:1 view), since there is no way to distinct it from the "erase selection" click in an image. However, i think it is more important to fix the behaviour above, if one wants to select one pixel he could zoom in or select a fixed size of 1x1 pixels.
*** Bug 51541 has been marked as a duplicate of this bug. ***
*** Bug 56910 has been marked as a duplicate of this bug. ***
Created attachment 5797 [details] [review] Here's a patch against the gimp-1-2 branch as of today. Would be niceif someone could give it some testing (Simon?!).
Thanks for attempting a fix, Sven. However, this looks to me like a "shallow" fix: it fixes the symptoms, but not the root cause (ie that the co-ords are being incorrectly rounded). Having said that, I haven't tried running the fix. More info when I have done.
I don't think that the cause are rounding errors at all. IMO the old code was simply wrong. Drawing the rectangles using draw_rectangle (x1, y1, x2 - x1, y2, - y1) will lead to a rectangle that is one pixel too wide and one pixel too tall because the one pixel wide outline will be drawn at the right, bottom side of the given rectangle. If we want the outline of the rectangle to match the outline of the selection, the fix is probably correct. The proposed fix does however not fix all the symptoms. Especially layer moves still look wrong sometimes. That's whay I asked for some feedback.
This bug has been idle for over a year - has this patch been applied to either HEAD or the 1.2 branch? If so, can we close the bug? Dave.
As far as I can remember it hasn't been applied since it wasn't completely correct and might cause other problems. Would be interesting to see if the problem is still in 1.3 since we changed a lot of the related code.
This bug has been fixed in the current stable and unstable versions, see bug #110115.
I'm reopening this bug because the problem still persists. I was wrong when identifying it as a duplicate of bug #110115; my confusion was due to the last paragraph of Sven's comment dated 2001-10-10 which IS related to said bug and not to this. Sven's patch properly fixes most of the symptoms of this bug report. Should be considered also for the HEAD version which still suffers the same problem as well. For ellipse_select and rect_select, a check for x2 > x1 && y2 > y1 is necessary to avoid passing negative widths to the gdk_draw_* functions when one of the sizes is zero. Unluckily, but in a different matter, a 1x1 ellipse does not get drawn as a single pixel, at least in my Linux system. AFAICT the only remaining issue after applying Sven's patch is the selection outline segments placement while moving the selection around. That's more difficult to patch because the 1-pixel displacement should always occur towards the innermost part of the selection. This last issue may be a WONTFIX, at least for 1.2.
Sorry again. The original report mixes two different problems, one that was a duplicate of bug #22375 and has been reported several times, and the off-by-one in the GDK rectangle that is the one reported by Simon and fixed in Sven's patch. As bug #22375 has been already fixed, I'm leaving this report open for the other part and changing the summary accordingly. I'm also submitting the modified version of Sven's patch that ensures that no negative sizes are used. For robustness the patch makes also the check in the selection moving code even if it may be unnecessary.
Created attachment 16255 [details] [review] Modified version of Sven's patch
Please commit, if possible to 1.3 as well.
Fixed in both branches: STABLE... 2003-05-05 Pedro Gimeno * app/edit_selection.c (edit_selection_draw) * app/ellipse_select.c (ellipse_select_draw) * app/rect_select.c (rect_select_draw): Applied a (modified) patch by Sven Neumann that fixes bug #17904. ... and HEAD: 2003-05-05 Pedro Gimeno * app/tools/gimpdrawtool.c (gimp_draw_tool_draw_rectangle, gimp_draw_tool_draw_arc): Ported the fix for bug #17904 from the STABLE branch (off-by-one when drawing the rectangle/ellipse previews).
The following patch against HEAD fixes the last remaining bit of this problem, noted in my comment dated 2003-05-04 11:06. Everything was already there except for the actual pixel displacing; the code has been shamelessly copied-and-pasted from app/display/gimpdisplayshell-selection.c (selection_transform_segs) which is used for the marching ants and works fine. After this change both functions look the same so it could even be considered to make one of them public instead of duplicating the code. Looks safe to backport since the function that sets the .open member of each segment (gimp_image_mask_boundary) is shared between the marching ants and this code, and the marching ants are definitely well tested :)
Created attachment 16830 [details] [review] Fixes off-by-one in selection boundary drawing
Of course we'd like to see both functions merged. But please commit your patch first.
Committed to both branches: HEAD... 2003-05-26 Pedro Gimeno * app/tools/gimpeditselectiontool.c (selection_transform_segs): Fix off-by-one when dragging the selection. Fixes the last pending issue of bug #17904. Use temporary variables for clamp values. * app/display/gimpdisplayshell-selection.c (selection_transform_segs): Perform the clamping that fixes bug #110014 here instead of in the callers. Solves a rare case that was not properly handled before. (selection_render_points, selection_generate_segs): Remove the clamping code from here. * app/tools/gimpdrawtool.c (gimp_draw_tool_draw_rectangle): More clampings to avoid overflow of 16-bit coordinates. ... and STABLE: 2003-05-26 Pedro Gimeno * app/selection.c (selection_transform_segs) * app/edit_selection.c (selection_transform_segs, edit_selection_draw): Merged last bit of the fix for bug #17904 from HEAD. Also merged fix for bug #110014. * app/rect_select.c (rect_select_draw): Added more clampings to avoid 16-bit overflows like those in bug #110014. Hopefully it won't need to be reopened again after this change.
The final fix is part of the stable release 1.2.5 (with the intermediate fix in 1.2.4). Closing this bug.