-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
CropResize #3903
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
CropResize #3903
Conversation
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a nice addition! I think you have already said that you want to do this, but we should probably figure out a better place for the documentation about the image functionality sometime.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could also add a test that if we crop/resize a 360x360 image to 180x180, that the pixels we get are the center 180x180 of the original image? And the same for, e.g., 360x180 -> 180x180 and 180x360 -> 180x180. I can help write that test if you want, just let me know. But I am a little paranoid that we should actually check that we are getting the correct pixels as output, because it would be really easy to modify this function in the future in a way that causes these tests to pass, but actually garbles the pixel outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, so if the dimensions are equal then only resize will happen no need to crop the images
Crop only happens to keep the aspect ration without having a resolution problem,
The best as you said is to show before and after it will be much easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand now; thanks for the clarification. Still though, if we crop and resize from 360x180 -> 180x180, then we can still check that we get the center of the image: the output pixel (x, y) should be the same as the input pixel (90 + x, y). Do you think we can add a test for that?
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the slow review on this. I didn't have a chance to look into what is failing, but at least the implementation of cropping seems just fine to me when I looked through it. I had some comments about the scope of the function---I wonder if it would be clearer to users if we kept the functionality as just cropping to the center of the image, and then allowing users to resize images with existing functionality. But maybe there is something I missed with that idea; let me know what you think.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand now; thanks for the clarification. Still though, if we crop and resize from 360x180 -> 180x180, then we can still check that we get the center of the image: the output pixel (x, y) should be the same as the input pixel (90 + x, y). Do you think we can add a test for that?
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Co-authored-by: Ryan Curtin <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
shrit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good we can approve it
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
Signed-off-by: Omar Shrit <[email protected]>
rcurtin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a quick change to HISTORY.md to point out that we added the feature. Everything else looks great---let's get it merged 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second approval provided automatically after 24 hours. 👍
Adding cropping functionality for images, in addition to the previous resizes added previously