Bug 128293

Created attachment 17912 [details]
demonstration screencast: ratio crop refuses given width/heigth values

same here - also occurs with digikam 0.9.0-beta2. The "ratio crop" tool has
difficulties to accept values if a fixed ratio (e.g. 1:1) is set. Changing the
values for width or height (with the spin boxes, the text entry field or the
sliders) always leads to a recalculation plus/minus some pixels of the changed
value (after approx. 0.5s). Therefore no exact cropping to a given size is
possible. Decreasing one dimension therefore alwalys results in an additional
decrease. The attached screencast should explain the described behaviour
(should be played with only 5fps 'cause of dropped frames during encoding). It
seems that the values are veryfied/recalculated in a wrong way...

I've been looking into this problem. I found that 640x480, 800x600, 1200x900, 1280x960 always work correctly but 1024x768 and 1152x864 (3:4 aspect ratio, i.e. same as a standard PC screen) never work.

If you resize, e.g. maximize, the Aspect Ratio Crop dialogue window then you get different errors and even 640x480, 800x600 etc. have errors too.

Looking at the code, what seems to be happening is that it takes the value you enter in the Height or Width spin-box and the selected aspect ratio then draws the selection rectangle in the preview window. Since the preview is not full-size the values have to be scaled. It does this calculation using floating point values then casts the results back to integers. Then, it takes the actual size of the selection rectangle in the preview window and scales the size back up to full-size (again as a floating point calculation with the results cast back to integers) and uses these values to populate the Height and Width spin-boxes. Both of these calculations have rounding errors so you don't get the values you expect.

What it should do is to apply the aspect ratio to the value - Height or Width - that you enter to calculate the other value and use those to draw the selection rectangle (which will only be an approximation as the preview is not full-size).

I am working to produce a patch to fix this.

I also came across this problem last week.
Presumably, the problem of the golden mean helper lines changing their
position slightly on horizontal/vertical flip, reported
in  http://bugs.kde.org/show_bug.cgi?id=120450, is related to this.

Mark, have you been able produce a patch 
(I would very much appreciate one ;-)?

Setup: Fedora Core 6, Showphoto 0.5.0, Digikam 0.9.1

What I do: I define 4x3 or 800x600 aspect ratio.

What I get: 798x600 or 600x602 crop boxes.

I mention both Digikam and Showphoto, as the Digikam editor seems to be Showphoto.

Mark,

could you give a brief update on the status of the patch you mentioned in #2?

I also had a look at the code, and I think your analysis is 
absolutely correct.


Best, Arnd

This bug still exists in .9.3-final. The accuracy of aspect-ratio crops is off by as much as 9 pixels. All one needs to do is click the "Max Aspect" button to see that the crop tool is not working correctly. This is problematic for anyone who needs accurate aspect-ratio prints.

UFRaw's crop tool is a good example of one that is accurate and works correctly IMHO.

Scott, you could have a look at the code in    http://websvn.kde.org/branches/extragear/kde3/graphics/digikam/imageplugins/coreplugin/ratiocrop/imageeffect_ratiocrop.cpp?view=log
and see if you can fix the problem.
Apparantly none of the developers has managed to find time to
hunt down this bug. So if you could help in solving this issue,
this would surely very much appreciated!

Best, Arnd

The real problem is with ImageSelectionWidget class. I'm working on it to use real image dimensions when computing values. The patch is almost working: I will post it here within few days for testing.

Thanks for the hard work, Roberto. It is appreciated.

Created attachment 23118 [details]
digikam-0.9.3-ratiocrop.patch

Please test!

Changes:
- for any ImageSelectionWidget class method, real image dimensions are used to
compute values (aspect-ratio, ...)
- convertion from real to local region selection is performed only when
updating the pixmap.
- when the mouse is used, his pointer position is first converted to a virtual
position within real image coordinates and the latter is used to compute values
(opposite corner, ...). Local pointer position is still used to check if it is
within a corner to select the cursor icon. No more needed to convert from local
to real region selection.
- the range for the width|height spin-boxes is from 0 to maxRange where the
latter is computed taking care of top-left selection position and his
aspect-ratio. When the width|height is 0, the selection border is not drawn and
 the OK button is disabled.
- minor improvements for the region selection when it is|starts out of the
image border.

Roberto,

Thanks for the patch. I very appreciate your work... I have not really work on Ratio crop tool implementation since a long time. So all contributions are welcome. Thanks again.

I let's Arnd review your patch in details. I'm too busy to finalize the new Time-Line view for next 0.9.4 release:

http://digikam3rdparty.free.fr/Screenshots/timelineconcept3.png

Regards

Gilles Caulier

Roberto, 

I have tested the patch and I do think that the approach is really the
correct one. However, there are some issues, which are caused by 
the underlying problem of cropping to a specific aspect ratio.
Namely, this can only be achieved properly for certain combinations of
height/width:
For example, consider a 3 to 2 ratio. Changing the rectangle,
one may get to 1111/740 = 1.501351...
In contrast 1110/740 = 1.5 or 1119/746 =1.5 are ok.
So the question is, whether one wants to achieve the aspect ratio
only approximately, or precisely.
If we want to do it precisely, we have to think a bit harder.
Essentially it boils down to a restriction of the possible values for 
height and width, in dependence on the ratio:
  height = i * r_h
  width  = i * r_w
where the ratio = r_h : r_w .

But would it be possible to restrict the motion of the mouse to just
these values? Maybe one could just draw the rectangle
with the right height and width, which match closest
to the current mouse selection?

The same problem also arises with the input boxes: Here
all values are possible, but not necessarily lead to the precise
aspect ratio.I am not sure whether there is any good way to ensure
this properly for the text input?
One approach might be: after changing
a value in the input box the closest matching correct values
for H and W are determined and used.
This might give a slightly unusual feel, that
the values in the input box magically adapt. But maybe that's
the only way of ensuring precise values for the aspect ratio.

Another issue: Changes to the text input boxes
are not reflected in changes of the selected region.
I think that's what the timers were used for
(can't confirm this right now, because I only have the patched
svn version on  my machine ;-). So currently one has to use the
arrows near the entry box to activate the change.

Also, Dotan's example from #c4 with the aspect ratio 4:3:
Specifying 400 for the height, gives 299 for the width,
and 300 for the width 400 gives for the height.

Another minor issue: why do the sliders for width/height change
their position, if the selection region is moved with the mouse?

First we have to decide if we want to only use the precise
aspect ratio. I think we should, what do you think?

Best, Arnd

Arnd, I'd recommend a checkbox for "Force exact aspect ratio" or "Allow aspect ratio approximation" with a good description of what it's for. I've been in situations where I've needed both.

Hm, sounds like a good idea, but I think that first we should try to fix this bug keeping the approximation behaviour and ensuring that if ratio=r_h:r_w and we specify height=i*r_h, we will always get width=i*r_w. After that we can work on it.

> Specifying 400 for the height, gives 299 for the width
What are dimensions of the image you are using for test? I'm using a 2592x1728 image and I cannot reproduce it.

> Changes to the text input boxes are not reflected in changes of the selected
> region. I think that's what the timers were used for.
It seems to me that input box widget sends valueChanged signal only when he lose the focus. As soon as I press Tab key, changes are reflected in the selection region.
I think the times were used to let users to select the value with the spin-box, waiting a little before to change it again. With my patch, when you select the width (height), only the height (width) is changed so times are no more needed.

> Why do the sliders for width/height change their position, if the selection
> region is moved with the mouse?
Because the max range is changed.

Best, Roberto

The image dimensions are: 3456 x 2304

Input boxes: ok (have to admit that I wasn't aware of the TAB; well
I usually only use the mouse anyway).

Slider position change: yes, does make sense - thanks for the explanation.

Actually, one more remark: Sometimes it would be nice to do really precise
crops which would require to zoom into the image.While
there is a widget to zoom into an image, this might be really difficult 
to implement. (If we really would like to have this, this would
be better discussed in a separate wish - still I wanted to mention it
here ... ;-)

Created attachment 23349 [details]
ratiocrop-width-height-ratio.patch (add-on)



To be applied after digikam-0.9.3-ratiocrop.patch.

With this patch, aspect ratio is computed using width ratio value and height
ratio value. This should ensure that if ratio=r_h:r_w and we specify
height=i*r_h, we will always get width=i*r_w solving an issue with custom
aspect ratio Arnd reported to me.

Further it changes Aspect ratio ComboBox text to take orientation into account:
if current orientation is Landscape, you will read 3:2 - 4:3 - 5:4 - 7:5 - 10
:7; if current orientation is Portrait, you will read 2:3 - 3:4 - 4:5 - 5:7 -
7:10.

Even custom aspect ratio take orientation and auto checkButton into account.
Suppose current custom ratio is 800:600 (landscape orientation):
- if you change orientation, ratio values order is reversed (600:800)
- if you change first value to 400 and auto is checked, custom ratio became
400:600 and orientation became Portrait
- if you change first value to 400 and auto is unchecked, second ratio value is
changed to 400 (ratio became 400:400) to ensure that with landscape orientation
first value >= second value.

At last, I moved width and height center buttons near to X and Y input widgets
(by now they are near Width and Height input widgets) since they change
selection position and not his size.

Please test and let me know if it introduces new bugs.

Created attachment 23400 [details]
ratiocrop-aspect-ratio-fix.patch (for rev. 768751)

To be applied with current svn revision version (768751 for ratiocrop plugin).

It merges previous patches and fixes few minor issues I have introduced with my
last patch.

SVN commit 770107 by abaecker:

When cropping images, fullfill the given aspect ratio as good as possible (patch by Roberto Castagnola).

CCBUGS: 128293
TODO:KDE4PORT



 M  +134 -65   imageeffect_ratiocrop.cpp  
 M  +15 -16    imageeffect_ratiocrop.h  
 M  +363 -357  imageselectionwidget.cpp  
 M  +16 -17    imageselectionwidget.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=770107

Thanks a lot for this nice patch, Roberto!

I applied this patch together with the one you sent me by mail to 
ensure that the current selection values are only saved when the selection
is applied (i.e not also on Cancel). 
All tests I did worked fine, but due to the complexity and number
of possibilities there still good be some overlooked cases.

As last step, before this bug can be finally closed, Roberto
plans to provide a patch which (optionally) ensures precise
aspect ratios as discussed in #c12 above.

Best, Arnd

Ok Arnd,

Thanks to Roberto. Excelent job...

Tomorow, i will backport patch to KDE4 as well

Gille Caulier

Arnd, Gilles,

I think it will take longer to ensures precise aspect ratios as discussed in #c12 above. However I consider it as a new feature and I will open a new bug/wish as soon as the patch is ready. In the meantime, I think this bug can be closed.

Anyway, feel free to add me to CC list if a new bug/wish is opened for this plugin.

Best, Roberto

SVN commit 770517 by cgilles:

digiKam from trunk: backport commit #770107 from KDE3 branch
BUG: 128293


 M  +169 -107  imageeffect_ratiocrop.cpp  
 M  +21 -21    imageeffect_ratiocrop.h  
 M  +364 -361  imageselectionwidget.cpp  
 M  +30 -34    imageselectionwidget.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=770517