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

Improve edge detection when rotation is used #52

Closed
ValentinH opened this issue Aug 19, 2019 · 7 comments
Closed

Improve edge detection when rotation is used #52

ValentinH opened this issue Aug 19, 2019 · 7 comments
Labels
enhancement New feature or request PR-welcome help is welcomed for this issue

Comments

@ValentinH
Copy link
Owner

ValentinH commented Aug 19, 2019

The current restrictPosition() function (here) does not take care of the rotation prop and is too restrictive.

We need to handle the rotation properly to have correct edge detections.

@ValentinH ValentinH added enhancement New feature or request PR-welcome help is welcomed for this issue labels Aug 19, 2019
@MattyBalaam
Copy link
Contributor

Not PR ready, but I had a play around this. It may or not be useful. I have the crop-area successfully tracking-around the area an image has roatated in. I’m guessing that instead of cropping like this, the ideal would be to crop without showing any areas outside on angles outside of 0/90/180/270

MattyBalaam@f2bdb2d

@ValentinH
Copy link
Owner Author

Thanks for this, I'll check it later today 🙂

@ValentinH
Copy link
Owner Author

I just checked it and I'm actually not sure to understand why you are modifying the crop area size based on the rotation. This behaviour does not feel really familiar to me. 🤔

I would expect to keep the crop area fixed but only adapt the rules of the edge detection.

Anyway, thanks for giving this a try! 👍

@MattyBalaam
Copy link
Contributor

I wasn't sure where to start as I'm not totally sure how everything works conceptually behind the scenes, so just tried at the first place I found I could manipulate things. I'll take a look at the edge detection tomorrow.

@MattyBalaam
Copy link
Contributor

MattyBalaam commented Aug 22, 2019

I adapted the code to be in the restrict function based on the rule of keeping at least part of the image without the crop area.

MattyBalaam@0e665da

A caveat with this is that when the demo portrait image is rotated 90 degrees then it is possible to manipulate the zoom so that the width of the image becomes smaller than the crop. Do you have pointers about the best place to adjust the zoom settings in this situation?

Screen Shot 2019-08-22 at 17 10 52


I also spent quite a while attempting to figure out way to restrict based on keeping the crop entirely within the image, but I’ve not been able to quite work out how to convert the co-ordimates properly. I might take another look, but it has been quite a slog trying to work it out.

e.g. in the below image at this point you would no longer be able to drag the image either down or right.

Screen Shot 2019-08-22 at 17 14 43

@ValentinH
Copy link
Owner Author

WOW thank you so much, this looks awesome!! Could you already submit a PR for better visibility so that we can iterate there? 🙂

Automatically setting the zoom higher when needed should not be an issue 🙂

@ValentinH
Copy link
Owner Author

This is done in v1.15 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR-welcome help is welcomed for this issue
Projects
None yet
Development

No branches or pull requests

2 participants