search

viliusle / miniPaint

online image editor
http://viliusle.github.io/miniPaint/
Other
2.6k stars 607 forks source link

Issue #279 - Fix resize bug on certain rotate degree #281

Closed kmanaseryan closed 2 years ago

kmanaseryan commented 2 years ago

Not sure why there was a if condition on rotate value being positive. That's why for clockwise rotation it didn't work. After removing it bug got fixed. Tested other use cases looks like removing that condition didn't break other stuff.

Related issue: https://github.com/viliusle/miniPaint/issues/279

kmanaseryan commented 2 years ago

@viliusle please review

viliusle commented 2 years ago

Not sure why there was a if condition on rotate value being positive

If I remember correctly, resize handles on rotated layers required much more calculation so it was disabled. I will check it again.

That's why for clockwise rotation it didn't work.

Can you give more details how to reproduce it? I had no issues with rotation.

I will review this PR.

viliusle commented 2 years ago

I can not see any improvements on rotation (still waiting on your clarification), but resizing rotated object works really badly.

kmanaseryan commented 2 years ago

@viliusle as it's mentioned in #279 the resize doesn't work when it's rotated at some degrees, e.g. if you rotate right just a little bit and try to resize it won't work. You can also check the screencast provided in the issue.

Maybe the PR title is a little confusing. The bug is related to resize which happens on specific degree of rotated layer. There is no improvements indeed, just bug fixing related to resize.

viliusle commented 2 years ago

Resize feature for rotated layers requires much more work, and because I could not find time and because you still can change width, height, X, Y on right sidebar, it was disabled. Your PR enable to resize layer, but it does not add that additional functionality, so user experience would be even worse.

kmanaseryan commented 2 years ago

Resize feature for rotated layers requires much more work, and because I could not find time and because you still can change width, height, X, Y on right sidebar, it was disabled. Your PR enable to resize layer, but it does not add that additional functionality, so user experience would be even worse.

@viliusle I understood, but actually the current version also allows resize on rotate. To reproduce it, you can rotate counterclockwise direction it still let you to resize.

viliusle commented 2 years ago

Now I see, that it IS possible to resize rotated object in some cases, I will fix it. But still we should not enable this feature until resizing rotated layer would work perfectly.

Thank you for notice.

JordanMagnuson commented 2 years ago

@viliusle I have tested @kmanaseryan's PR, and resizing and rotating seems to work fine now with this fix. It is a big improvement over the current implementation (where you can only resize after rotating in counter-clockwise direction). I think this should be commited, as it does not break any current functionality, but only offers improvement. Additional improvements could be made later if desired. Just my two cents.

JordanMagnuson commented 2 years ago

Thanks for the merge. I can see where resizing rotated layers can be improved further, but I think the current functionality is not bad. Here is a brief video showing the current functionality: https://www.loom.com/share/8af5d40356e1402fab7d7f03ddd081ce

Basically, resizing rotated layers using the corner handles (to keep aspect ratio) seems to work fine. The side handles used to transform the rotated layer in one direction (vertically or horizontally) are not perfect, perhaps, but the effect they produce is generally not bad.