I can reproduce this.

I have changed the summary of the bug report because it happens with both positive and negative angles.  Try the following:

- Create or load a square image (any size will do)
- Select the rotate tool
- Set "Clipping" to "Crop to result" or "Crop with aspect"
- Rotate by +60 degrees or -30 degrees

=> The whole layer is gone.

Note that even when there are some bits of the layer remaining, the area that remains is much smaller than it should be.  Compare the results of a rotation by +44 and +45 degrees and you will see that something is obviously wrong (or +44.99 just for fun).

Would somebody please be so kind and explain how "Crop to result"
is supposed to work? To me its purpose looks identical to "Adjust",
which is the default "don't do anything to the result" mode.

Anyway it's obvioiusly 100% b0rk and needs to be fixed or disabled for
the release. Raising priority.

Ehm, I see what it's supposed to do now. I wondered about the usefulness
back when it was introduced and even more now. How is this ever supposed
to produce a useful rectangle that contains anything that is not just
random? (even though it might be topologically correct if the algorighm
worked correctly).

Crop to Result and Crop to Aspect are very useful features (provided that they are implemented correctly). It removes an extra step that you need to do when correcting photos or scans that are slightly rotated. Instead of rotating the image and cropping the result manually, the Rotate tool now does this work for you.

Unfortunately it seems that the implementation is still broken.

The problems seems to be quite local. With a soon-to-be-attached patch applied, completely reasonable points are sent to

  find_maximum_aspect_rectangle (&r, points, i);

for Crop with aspect and

  find_three_point_rectangle          (&r, points, i);
  find_three_point_rectangle_corner   (&r, points, i);
  find_two_point_rectangle            (&r, points, i);
  find_three_point_rectangle_triangle (&r, points, i);

for crop to result at ./app/core/gimp-transform-resize.c:~368, and these are static function only used there. Would be cool if the original author could look at this, so I am adding Geert Jordaens to CC.

Created attachment 96052 [details] [review]
001_gimp_transform_resize_crop-debug.patch

Just to clarify; the points sent to those functions are completely reasonable, but the returned rectangle is unreasonable (as outputed by the debug patch right after the results of these functions has been extracted). I.e. the errors seems to lie locally in these functions.

Could some body give me a correct definition(functionality) of the 
normal and backwards options?

These options are irrelevant here, as far as I can see. They only affect the transformation matrix before it is being passed to the transform-resize code.

The use is explained in http://docs.gimp.org/en/gimp-tools-transform.html, see "Transform Direction". The primary use of the backward mode is to correct tilted photographs using the Rotate tool and to correct for perspective distortions (for example falling lines) using the Perspective Transform tool.

Created attachment 96304 [details] [review]
Fix  for clip to result

Fixes the invalid cropping

Created attachment 96305 [details] [review]
same patch with coding-style cleanups

With this patch applied, Crop to result seems to work better.

Crop with aspect however still sometimes causes the layer to be cropped away completely. Simply try to rotate a photograph about 70 degrees. Also for small rotations, the result doesn't seem to be correct. I would expect the resulting rectangle to be centered on the center of rotation.

I did not touch the crop with aspect, it try to find some time and fix it.

Created attachment 96331 [details] [review]
Fix  for clip to result + aspect

The result is not yet centered around the rotation. 
To facilitate the calculation of the rectangle the coordinates are first translated before calculating. The reverse translation is not the same and should be different for each aspect ratio. I (or somebody else) still have to figure this out.

Is the expectation from comment #23 not false?  
  I would expect the resulting rectangle to be centered on the center of
  rotation.

The transform function does not receive the center of rotation. How can the result be centered around a point that is not know?

When I provided the last patch I forgot to put the previous on obsolete.

Geert

I am not sure if the expectation from comment #23 is correct. But when I rotate a rectangle and then look for the largest inscribed rectangle with the same aspect ratio, it seems to me that the original and the resulting rectangle will have the same center point. Just prove me wrong with a counter-example and I will drop this conjecture.

I found another bug in the clip with aspec, so I will review this part again.

Created attachment 96658 [details]
Prove that the inner rectangle does not has to have the same center point as the outern one

Image to demonstrate various possibilities in placing the cropped rectangle after crop-rotating it

Comment on attachment 96658 [details]
Prove that the inner rectangle does not has to have the same center point as the outern one

Sven, 
the resulting rectangle does not have to have the same center point. If the inner rectangle touches only two sides of the outer rectangle, there are unlimited possibilities to place the rectangle. 
See my little attached image made from a screenshot.

OK, thanks for the example.

Since there are several solutions to the problem, it would be nice if we could add another constrain to make the solution well-defined and predictable. Selecting the most centered rectangle seems like a good choice for such a constraint.

Sven, I think, for 2.4 this will be fine, and for some later version ywe could think about giving in a preview the joice to select the desired field. But i think this will mean lots of work and lots of new possible bugs. Maybe in 2.6?

(In reply to comment #18)
> I found another bug in the clip with aspec, so I will review this part again.
> 

geert, what problem, and how is your progress on it? I can try to finalize what you have if you lack the time

I'm indeed currently short on time, the bus is with clip & asect rotating 45° does  not give any result. 
Secondly I think that selecting the most centered aspect rectangle would also be a big algorithm change. And I did not have any smart idea's other then brute force or binary search. Which would lower the performance again.

Geert

(In reply to comment #24)
> I'm indeed currently short on time, the bus is with clip & asect rotating 45°
> does  not give any result.

45° is a special case, transition between landscape and portrait.
The biggest area is a square (which can slide along the main axis
of the picture, as jussuf said).

> Secondly I think that selecting the most centered aspect rectangle would also
> be a big algorithm change. And I did not have any smart idea's other then brute
> force or binary search. Which would lower the performance again.

Cropping a rectangle within a rectangle just needs a few plain old
trigonometry, and you get explicit formulas :

A LxH picture (same units, pixels, mm, whatever),
 0/ Origin at the center, a = L/2, b = H/2, axes horizontal and vertical.

 1/ Rotation needs --- angle free --- a circular workspace, diameter                  
  sqrt(L^2+H^2) or --- angle g fixed --- a rectangular envelope (cf.
  imagemagick, convert -rotate),
    length = L * abs(cos(g)) + H * abs(sin(g))
    height = L * abs(sin(g)) + H * abs(cos(g))

 2/ As the cropped centered rectangle is symmetric, it is included in
  the picture and also the symetrical, contra-rotated by an opposite
  angle -g ; for small angles, the intersection is an irregular octogon
  BUT if |g| > delta (angle between the diagonal of the picture and the
  nearest axis), it becomes diamond-shaped...
  Note that the centered crop don't depend on the sign of g.

 3/ Same ratio formulas --- (XP,YP) are the positive coordinates of the
    upper right corner of the crop, and r = long_side/short_side

    3.1) g near 0° (or 180°), portrait->portrait & landscape->landscape
         XP = a / abs(cos(g)) + r * abs(sin(g)))
         YP = b / abs(cos(g)) + r * abs(sin(g)))

     3.2)  |g| near 90°, switched orientation, landscape->portrait
           or vice-versa
         XQ = b / abs(sin(g)) + r * abs(cos(g)))
         YQ = a / abs(sin(g)) + r * abs(cos(g)))

 4. Maximal area formulas, two cases : first compare (g - n*90°)
    with G = asin(1/r) / 2
    Some examples:
      r	        G	      delta
      1/1 	45°	      45°
      4/3 	24,295°	      36,870°
      sqrt(2)	22,5°	      35,264°
      3/2 	20,905°	      33,690°
      16/9	17,114°	      29,358°
      2/1	15°	      26,565°

    4.1 Angle g near a 90° multiple: ("axial" crops)
     xI = (a*abs(cos(g)) - b*abs(sin(g))) / abs(cos(2g))
     yI = (b*abs(cos(g)) - a*abs(sin(g))) / abs(cos(2g))

    4.2 Angle g near an odd 45° multiple: the optimal area point M
     is the middle of the segment cut by axes on sides of picture.

     Landscape (b<a) or portrait (a<b) picture:
     xM = min(a,b)/(2*abs(sin(g)))
     yM = min(a,b)/(2*abs(cos(g)))

     These "diagonal" crops only get a small percent of the picture,
     and can slide along the main picture axis: automatic crop is not
     a good idea here...

HTH

>> Secondly I think that selecting the most centered aspect rectangle would also
>> be a big algorithm change. And I did not have any smart idea's other then brute
>> force or binary search. Which would lower the performance again.

>Cropping a rectangle within a rectangle just needs a few plain old
>trigonometry, and you get explicit formulas :

It is a rectangle in a polygon.

(In reply to comment #26)
[...
> It is a rectangle in a polygon.

What sort of polygon? Sorry, I computed the formulas in the context of rotate-and-crop rectangular photos.

There are several problems then:

 * When firstly you correct perspective, the original photography rectangle becomes a convex polygon, with four or five sides, depending on the position of the infinite line. But this polygon, generally, has no symmetry center.
 Anyway, the four or five sides can be defined by affine equations, and give explicit formulas...

 * If distorsion is also corrected, lines becomes curves --- if the picture is not  convex, the positions of the four crop corners will not ensure that the whole crop lies inside the picture.


 

*** Bug 504190 has been marked as a duplicate of this bug. ***

*** Bug 518216 has been marked as a duplicate of this bug. ***

I can reproduce the bug with 2.6.0, and the latest patch here (Fix for clip to result + aspect) doesn't apply. Care to update it?

how about setting the target to 2.6.1?

*** Bug 555934 has been marked as a duplicate of this bug. ***

*** Bug 565464 has been marked as a duplicate of this bug. ***

*** Bug 565686 has been marked as a duplicate of this bug. ***

Some days ago I was giving a GIMP course and was telling my students how great this Open Source program is when they run straight into this bug. Major egg on my face...

It looks like the last developer activity on this bug was more than a year ago. Is there any chance to get this fixed?

My students found another usability problem with the rotate tool:
In contrast to the other clipping options, when you use "Crop to result" or "Crop with aspect" it is very difficult to imagine the outcome. The problem is amplified by not using the center point of rotation as the center of the result. Therefore I fully support the idea presented in comment#22 to provide a live preview and the possibility to choose the position of the resulting area.

Unfortunately, not being a GIMP developer myself, I can only assist with testing.

I myself found it strange that when I was using "Adjust" as clipping option that I had to select the menu item "Image -> Fit Canvas to Layers" manually to actually see the full result. The rotate tool should to this automatically. Then users will also probably better understand the difference compared to the clipping option "Clip".

In an open source project there is always a chance of things getting fixed, someone just needs to write a patch for it. So far the core developers have had more important things to work on than this bug however and I foresee that this will continue to be the case.

Created attachment 127914 [details] [review]
fixes the problem with disappearing image (too much croping)

This is a patch that only fixes the bug that the image is cropped to zero size in certain cases.
It does not fix the problem where the rectangle is located if there are multiple possibilities.

Since this is really an annoying bug (especially for small angles, which is the most used case when correcting pictures just slightly) I propose this partly solution as to prevent user frustration in the majority of cases.

I personally face the bug when rotating a photo with -N angle, where N is small and close to 0. The workaround I use is to flip horizontally the picture, perform a +N rotation and flip the picture back. This saves me in 100% cases.

I have applied this patch to trunk now so that it gets some testing. Will merge the fix to the gimp-2-6 branch later unless someone finds a problem with it.

2009-02-14  Sven Neumann  <sven@gimp.org>

	Bug 472644 – Rotate with clipping crops the whole layer

	* app/core/gimp-transform-resize.c: applied patch as attached to
	bug #472644. Supposedly fixes the problem of the disappearing
	image.

astifter@gmx.at, do you have a real name so that we can credit your work or would you prefer to remain anonymous?

I have now merged this change to the gimp-2-6 branch:

2009-02-18  Sven Neumann  <sven@gimp.org>

	Merged from trunk:

	Bug 472644 – Rotate with clipping crops the whole layer

	* app/core/gimp-transform-resize.c: applied patch as attached to
	bug #472644. Supposedly fixes the problem of the disappearing
	image.

We should open a new bug report for the remaining issues and close this one as fixed.

I have Gimp 2.6.6 from Debian Testing and I also have noticed this clipping. For me it occurs when I have a picture in portrait and I try to rotate it with Clipping "Crop with aspect". Is my version supposed to have this fix already?

SVN Trunk GIMP still seems to have some problems with "crop with aspect" mode.

I don't have a reliable way to reproduce it or to make it not happen, but for me if the image is "portrait" and I rotate by, say, 15 degrees, with "crop to aspect" selected, I end up with a blank canvas.  This happens in both corrective and normal mode (as I'd expect), with "crop with aspect" selected.

But this might be a different bug.

(In reply to comment #39)
> astifter@gmx.at, do you have a real name so that we can credit your work or
> would you prefer to remain anonymous?
> 

Sorry, forgot to add me to CC list and only now read your comment. My name is Andreas Neustifter, but since liam@w3.org still reported a bug with this I will try to look further into it. I can't promise anything tough.

Okay,

I also patched the "Crop to Aspect" part.

I tested both "Crop to Result" and "Crop to Aspect" with portrait and landscape layers (see the attached files) for the rotation case 1, -1, 30, -30, 45, -45, 60, -60, 89, -89, 90, -90 degrees which I think are the most notorious cases.

(The attached files can be used to compare an uncroped rotation of a certain value with the "Crop to Result" and "Crop to Aspect" version by rotating the testlayer and making the respective reference layer visible.)

Grettings, Andi

Created attachment 133368 [details]
testfile for landscape rotations

Created attachment 133369 [details]
testfile for portrait rotations

Created attachment 133370 [details] [review]
fixes the problem with inproper "crop to aspect"

BTW: The "Placement Problem" (where the resulting rectangle is placed in case there are infinite possibilities due to parallel polygon edges) is still open.

I can't think of any good solution apart from doing some wizardry by detecting rectangles and, in case the resulting rectangle is not touching all four sides sliding it to the centre of the input rectangle (which must _not_ be the rotation centre tough....)

Comments welcome.

Created attachment 133401 [details] [review]
fixes the problem with inproper "crop to aspect", added safety net

I now also tested the shearing, this works too.

I still managed to get the file cropped to zero with general perspective transformations sometimes, so I added a "safety net" that resorts to adjusting if the croping for some reason does not find a rectangle.

Can you update your patch to not use C++ style comments, please? Thanks.

Created attachment 134773 [details] [review]
Updated rotate_crop_to_aspect_v2.patch to use C comments only.

Sorry, I tried to be as consistent as possible, this slipped my notice.

Looks good to me now, thanks. We should get this into git and make sure it makes it into the 2.6.7 release.

> Looks good to me now, thanks. We should get this into git and make sure it
> makes it into the 2.6.7 release.
> 
Is there any action required from me?

No, we just need to find time to review and test this change. We should get to that before another 2.6 release is made.

Thanks for the patch, I have now pushed it to both branches:

commit ed748fd383f96f6f0da0d9094bf7bea89f0eb60b
Author: Sven Neumann <sven@gimp.org>
Date:   Thu May 21 14:29:08 2009 +0200

    Bug 472644 – Rotate with clipping crops the whole layer
    
    Applied patch from Andreas Neustifter fixing outstanding issues.