Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keepingCropAspectRatio doesn't keep aspect ratio if crop area is shrunk too much #51

Open
mariusc opened this issue Apr 11, 2014 · 5 comments · May be fixed by #52
Open

keepingCropAspectRatio doesn't keep aspect ratio if crop area is shrunk too much #51

mariusc opened this issue Apr 11, 2014 · 5 comments · May be fixed by #52

Comments

@mariusc
Copy link

mariusc commented Apr 11, 2014

I took the demo code and forced it to keep the aspect ratio and gave it a hardcoded CGRect for the controller.imageCropRect. You can see the source in https://github.com/mariusc/PEPhotoCropEditor/tree/d892059bb09f870152856d727cfc170b8fe2a913/DemoApp

Generally it works ok, but if I shrink the cropping area too much, the cropRect stops shrinking in height (if height < width) and only changes its width, so the aspect ratio is lost. It seems that it either tends to go towards a square cropRect, or a cropRect with the reversed aspect ratio (interchanges width with height). I tried to debug it for some time, but gave up in the end.

Added a screen capture to my fork showing the bug. Not sure if the movie works in browser, you might have to download the archive or clone the repo. https://github.com/mariusc/PEPhotoCropEditor/blob/a15321913e86d798876217c82cdc775f807c10d1/cropBug.mov

Later edit: I think one of the problem is in PECropRectView.m, cropRectMakeWithResizeControlView:, where it checks if the current rect is too small (line 294 - ..). But there may be other issues, because if I comment out from line 294 to the end of the function - without the return statement-, it doesn't change the aspect ratio. But I can still notice a change in the orientation of the cropping rect (width/height = aspectRatio or width/height = 1/aspectRatio). If I manage to track down the bug, I'll come back with a pull request :)

Later later edit: The other problem was in function constrainedRectWithRectBasisOfWidth: aspectRatio: from PECropRectView, line 327. In my opinion, the condition in that 'if' should check the relation between the width and height of the original image, not the scaled cropping rectangle. Usually, if I want to crop something and preserve its aspect ratio, I also want to preserve its orientation. I'll come back these days with a pull request, and then it's your choice if you think it's ok or not. Anyway, good job with this pod. Despite the fact that I stayed a couple of hours to figure this out, it still saved me a LOT of time :). Thanks

@mariusc mariusc linked a pull request Apr 14, 2014 that will close this issue
@mariusc
Copy link
Author

mariusc commented Apr 14, 2014

Issued the pull request #52

@stoneark
Copy link

I came across the same issue, mariusc's pull request #52 have solved this issue perfectly.

@padamthapa
Copy link

@mariusc @stoneark In "freeform" mode, frames of crop rect gets restricted. Also sometimes during aspect ratio mode; at some size while resizing, portrait rect suddenly jumps and becomes landscape and vice versa. Have you come across that ?

Thanks for the pull.

@padamthapa
Copy link

@mariusc After looking into changes in this pull on PECropRectView, I did make change on constrainedRectWithRectBasisOfHeight method too.
I Replaced if (width < height) with if (self.initialRect.size.width < self.initialRect.size.height) {

Now this issue for portrait aspect mode seems fixed.
Only issue is crash happens only on 64 bit device when it is resized to minimum size it could get (Portrait aspect ratio mode only).
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@mariusc @padamthapa @stoneark and others