Image resize not correctly honoring aspect ratio and staff space unit settings

• Dec 13, 2018 - 20:00
Reported version
3.0
Priority
P1 - High
Type
Functional
Frequency
Many
Severity
S3 - Major
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project

When resizing images, the button "maintain aspect ratio" has no effect when you enter new sizes in the inspector menu. When you resize the image draging with the mouse, aspect ratio is maintained.

When file with the resized image is saved and reopenend , image size is back to the original value.

Happens with this nighly as well as with the latest 3.0 Beta


Comments

Status active needs info

I didn't have any trouble resizing images with or without the aspect ratio option, and my changes were saved. Can you attach your score and precise steps to reproduce the problem?

Here are the detailed steps and results. Scores and screen snippets attached
- Load score "Bajazzo wo img.mscz"
- Click on title frame, right-click -> add -> picture: selects file "Männerchor Logo.JPG"
- Inspector looks like this: "Inspector1.JPG"
- Changing the value for width in the inspector menu does not change the height to maintain the aspect ratio
(Musescore 2 does this automatically)
- Dragging the corner of the image with the mouse does change both dimensions width and height correctly when the "Lock aspect ratio" button is set.
This was the resize issue - now for the save and reload.
- Change the image size with mouse
- New size see inspector clip "Inspector2.JPG"
- Save file as "Bajazzo with img.mscz"
- Close musescore, start new, reload file "Bajazzo with img.mscz"
- Image size see "Inspector3.JPG"
- Image size is neither the one when the image was first imported nor the one when the file with the resized image was saved.

Attachment Size
Bajazzo wo img.mscz 90.73 KB
Männerchor_Logo.JPG 63.16 KB
Inspector1.JPG 31.52 KB
Inspector2.JPG 30.52 KB
Bajazzo with img.mscz 91.03 KB
Inspector3.JPG 32.55 KB
Priority P2 - Medium

OK, I see, "Lock aspect ratio" works fine for dragging but it doesn't apply to adjustments made in the Inspector.

The save/reload issue I cannot reproduce with a current build, however. I followed your steps and on reload got exactly what I expected. Can you try again with today's beta update?

As an IT-consultant of more than 30 years I knew we must be doing something different if we get different results.
I finally found the small difference. I get the correct behavior when I leave the check in the box "Size in staff space units". The image size remains unchanged if I save and reload the score. When I remove the checkmark and do save and reload the image size has changed. The factor between the size before and after reloading the file is exactly the value for scaling in the page settings "Staff space". I could reproduce this when I change the scaling in the page settings. The factor before/after equals the new value for "Staff space". I think this information could give the developer some hints where to look for the origin of this behavior. During my test Musescore crashed once when I removed the check "Size in staff space units" - but this could not be reproduced.

Title Resizing Images Image resize not correctly honoring aspect ratio and staff space unit settings
Priority P2 - Medium P1 - High
Status needs info active

Thanks, that explains things and should help in fixing the issue.

For me, editing the image directly - double-click and drag handles - works fine. So does placing the image in a frame and using the "scale to frame size" button. So that's a couple of workarounds, no?

Meanwhile, though, should be a pretty easy fix I would have to imagine, although image processing isn't my forte.

I have a functional fix for this, but I don't like it because it accumulates rounding errors, so the aspect ratio starts to drift as you type in new values or spin then up and down. Sadly, this happens even if they start out with the exact same value (e.g., 100.0 sp × 100.0 sp).

The main reason for this is that the image does not have any memory of its “real” aspect ratio, so the code has to continually recalculate it on the fly based on whatever the most recent width and height values were.

I would really like to change the way this feature works by always locking to the original aspect ratio of the image rather than whatever the aspect ratio was when the user decided to check the “lock aspect ratio” box (as well as whatever the aspect ratio morphs to over time due to accumulated rounding errors).

Another option is to “lock in” the desired aspect ratio by taking a snapshot of it at the moment the user checks the “lock aspect ratio” box, and then use it for all subsequent calculations. If we do this, this cached aspect ratio will have to be stored in the score file.

Thoughts?

> Another option is to “lock in” the desired aspect ratio by taking a snapshot of it at the moment the user checks the “lock aspect ratio box”

Yes, I did that

> Ah, good. Are you persisting this in the score file?

Unfortunately VS basically started a rebuild after I switched to a new branch to commit it. It's gonna take some while.

In reply to by Howard-C

OK. Definitely needs to be stored in the score file, or we end up with the original problem again next time the score is opened.

Also, don't forget to apply the locked aspect ratio to the mouse-drag-resize calculations as well.

Just keep in mind anything changed about how the file is written comes with a compatibility concern, so make sure reasonable things happen in both directions.

If you're not saving the aspect ratio in the score file, where is the value coming from when you open the file? Is it being recalculated from the current width and height? If so, you'll get rounding errors — really bad rounding errors if the image has been resized to a tiny size prior to saving.

@BSG But even the smallest images are by the order of magnitude of 0.1sp, right? The fix works completely fine for things over 1sp, and drifts a tiny bit (about 0.01sp) for things below 1sp but over 0.1sp. I don't think that makes the fix broken.

In reply to by Howard-C

> An extreme example doesn't make the fix broken or incomplete.

Of course not. The extreme example is supposed to make it easier to understand that the fix is broken/incomplete, which it is.

Why did you create a variable to store the aspect ratio? Because otherwise you would quickly accumulate rounding errors. If you throw away the value of this variable at any point while it's still in use (such as when closing and reopening the score), you get a rounding error when you try to recalculate it. Repeatedly closing the file, reopening it, and resizing the image will still cause you to accumulate rounding errors, only more slowly. Mathematically speaking, it's the same as not having the aspect-ratio variable at all.

Tiny size is a red herring (and I regret using it as an example), because the rounding-error accumulation occurs regardless of size.

In reply to by Spire42

I think you misunderstood what's going on in the fix I provided. The ratio variable has to be defined even if there would be no rounding errors, because we need a way to remember the ratio. If there's no such variable, we cannot retrieve the original ratio value after one of the size values has been changed. (Although, if you have a way to retrieve it, please do tell me :-) )

> The extreme example is supposed to make it easier to understand that the fix is broken/incomplete, which it is.

It is not, if it only fails at extreme cases which no one would create other than for showing "a fix is broken/incomplete". The failure doesn't lead to a crash, nor a corruption, nor anything critical.

In reply to by Howard-C

What I'm saying is that the rounding errors that you're preventing by introducing the aspect-ratio variable are the exact same rounding errors that will occur when you close and reopen the file and then resize the image (without saving the aspect-ratio variable to the score). This is not an extreme or artificial case; it's 100% normal usage when working with any image at any size.

In reply to by Howard-C

> Aren't the rounding errors because of the omission of digits way behind decimal point?

Yes, but with accumulation, the errors can very quickly propagate to the visible range of the number (i.e., what the user sees in the spinboxes). This is compounded by the fact that the spin buttons themselves introduce further rounding errors because those buttons increment/decrement by 0.1 (which is not representable in floating point) and then “round” the resulting value to the nearest 0.01 (also not representable).

In my testing I was easily able to introduce numerically visible rounding errors by clicking the spin buttons fewer than five times in some cases (again, using normal-sized images with reasonable aspect ratios, including 1:1).

In reply to by Spire42

> Also, don't forget to apply the locked aspect ratio to the mouse-drag-resize calculations as well.

@Howard-C, are you planning to do this? The relevant code is inside Image::editDrag(), which is in libmscore.

FWIW, in my version of this fix, I made all of my changes in the Image class, and I left the InspectorImage class untouched.

> Is this bugged as well?!

In the sense that it has the same kind of accumulated rounding errors that the other one has, yes.

> In my opinion it isn't the best choice to fix this in libmscore because the issue is all about inspector control instead of element behaviour.

I disagree. “Lock aspect ratio” is a property of the element, not the Inspector. The Inspector is just one of multiple ways for the user to change the image's properties (including size). Enforcement of the locked aspect ratio should happen centrally, inside the actual element.

@Spire42 You can submit your fix as well and let's compare.

The issue emerges when an inspector action is done, and more precisely, it is about the relation between two values when one of them is changed in the inspector. I feel it's more natural to use the signal-slot system of Qt to do this.

In reply to by Howard-C

Thanks. I didn't want to step on your toes since you beat me to the punch with the PR, but I'll go ahead and do that. I'm working on IRL stuff at the moment so I won't be able to get it completely done and tested for maybe a day, so I hope yours doesn't get merged in the meantime. (Is there a way for you to request that it not be merged yet?)

In reply to by Spire42

An admin can add the "under discussion" label to it, but I'm not an admin :)

You can request for it in the developers' chat, I'm having trouble connecting to Telegram recently.

And I didn't mean to "beat you to the punch", it's just I'd already finished the commit before I saw this post. I was looking at a duplicate thread before.

Status PR created fixed

Fixed in branch 3.x, commit 17e709a477

_fix #279967: image resize not correctly honoring aspect ratio

A new member variable _aspectRatio of InspectorImage is added to remember the image's original ratio. It is also updated when "Lock aspect ratio" is checked in lockAspectRatioClicked(). The new two slots widthChanged() and heightChanged() help changing the value other than the one changed by user._

Fix version
3.6.0