Calculate scale for width and height separately when exporting #18784
Conversation
…ure exactly correct value is used for each
|
There was a problem hiding this comment.
This PR is wrong, if you export with max size 2048x2048 you'll get a corrupt image having a size of 2048x2048 which is wrong. The output is:
Note that in darktable when you set export size to 2048 x 2048 (for example) it means that the larger side will get 2048 and the other will be scaled to keep the aspect ratio of the original image.
|
Indeed it fails for setting dimensions. Please check again if the issue is really persisting after latest commits and re-setting the "crops" in gui. This all has a long story, 4f84542 as a reference to start from. |
|
Unfortunately I can still reproduce it even after pulling from
Should this separate scale for each dimension only be calculated if the max size is greater than the specified output dimensions? Or what would you recommend? |
|
I really think you are looking at the wrong code part. For me the problem is: What dimension/ratio do we get while cropping? You cropped to 6014x3382 which can't be downscaled to a precise 16/9 ratio. So we loose one line here. How to solve this? To me it seems we can either
|
|
@jenshannoschwalm : Option 2 is what we have today? But you propose to display a log message, that's it? I'm not for option 3. Let's not add another button. Maybe 1 is the best, no? I mean at least you get the dimension you asked and even if not 100% 16/9 (or whatever) that's close enough. Frankly I don't have strong opinion and cannot decide just now what the best between 1 or 2. Let's have some more time to think about this. |
Nope. Yes - we now report the correct crop dimension since late commit. Lets assume a ratio of 16/9 and we now crop to 6014x3382 which is "accepted" but as both - width and height - are not exact multiples of 16 / 9 all later scaling while exporting will possible lead to a line/row missing. What i would propose is, report the crop dimension (in expose hook) maybe in different colors, lets say in green for an exact match, otherwise as we have it now. |
|
While 1) would break builds, this feels like a bug (you asked for 16:9 but didn't actually get 16:9) so it seems okay to fix the bug going forward |
|
I'm not sure that to qualify this as a bug is OK. You asked for 16:9 sure, but the dimension of the image is not 16:9 and for one line of pixel who really cares? Now maybe we can be smarter indeed and do 1 at cropping time. This at least won't break old edits but it will ensure that new ones will get proper dimensions given the specified ratio. @jenshannoschwalm : Would doing 1 as described here will keep old edits? If so maybe the best option. |
How to proceed?
|
|
I still think 2 is better, but yes please can you create a draft PR to evaluate 1. TIA. |
|
@TurboGit i got it working :-) This means
There are a few questions remaining before doing the PR
|
|
@jenshannoschwalm : I would say:
|
|
@jenshannoschwalm I tested out your PR and it appears to fix the issue for me; thank you! |
As described in #18781, it's possible that the exact decimal value of the width scale and height scale can differ, for example:
By just taking the
min()of these two values and using it for both dimensions, we can arrive at the incorrect scaled value for one of them. Instead, calculate the exact scale value for each dimension separately so we do not lose this precision.