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

feat: add different resize modes and onlyScaleDown options #233

Merged
merged 4 commits into from Nov 15, 2020

Conversation

pxpeterxu
Copy link
Contributor

@pxpeterxu pxpeterxu commented Apr 19, 2020

Motivation

I was resizing images for a Tensorflow model, which required images to be exactly 224x224, rather than some size smaller than 224.

Changes

  • Add options.mode option to createResizedImage with similar values to those for react-native 's resizeMode
    • contain: existing behavior: will resize image to be fit within width/height, preserving the aspect ratio
    • cover: resize image to have one dimension be width/height, and the other be bigger, preserving the aspect ratio
    • stretch: will resize to exactly the width/height given, ignoring the aspect ratio
  • Add options.onlyScaleDown for cases where we only want the resulting image to be smaller or the same size as the original
  • Updated docs
  • Got the example app to build on Android and iOS again and have more options for testing

Testing

Tested using an actual Android device (Pixel 3a, Android 10) and iOS Simulator (iPhone 8, iOS 13.3)

Android

Android screen capture

iOS

iOS screen capture

@pxpeterxu pxpeterxu force-pushed the peter-mode branch 2 times, most recently from 6b4f59b to 5f34733 Compare April 20, 2020 01:10
@pxpeterxu
Copy link
Contributor Author

@PierreCapo I just updated the code to be rebased on the latest master and conflict-free! Let me know if you might be able to take a look here; I'd really appreciate it 😀

@PierreCapo
Copy link
Contributor

Hello @pxpeterxu sorry I'm super sad for not having reviewed this pr sooner. And honestly it's a pretty astonishing PR, thank you so much for having taken thee time to open it. I'll try to review it and make a release this week

@pxpeterxu
Copy link
Contributor Author

Hey, no worries @PierreCapo! Thanks for writing a great library in the first place; it works a lot better than expo-image-manipulator's resizer, especially on Android

@crobinson42
Copy link

This is a good PR!

@hsource
Copy link

hsource commented Aug 17, 2020

@PierreCapo - is there any chance of getting this merged still? Let me know! Happy to do a review of my own too. I'm using a patch of this PR in the meantime

@PierreCapo
Copy link
Contributor

PierreCapo commented Aug 17, 2020

@hsource I forgot about this PR, but yes please I'd need some help to review this PR. I didn't write this lib, I'm just managing it and my knowledges in Java are not great. I'm better in Objective-C but I'd need some help there too.

By the way, I think with this big PR merged, we could start migrating from Obj-C to Swift without losing too much work in progress, since at bam we are mostly using Swift (in which I'm far more comfortable with too).
So yeah, merging this PR would be a great milestone.

@cristianoccazinsp I know you are pretty familiar with the codebase, if you got time, could you help reviewing this PR too please?

@hsource did you face any issue while using the fork?

Waiting for your green lights guys

@hsource
Copy link

hsource commented Nov 15, 2020

@PierreCapo - heads up, the fork has been running well on our app for the past few months. I totally forgot about this until now, so sorry for the delay!

I ran the example app and looked through the code, and honestly this looks like a really great change. I recommend just pulling this!

@PierreCapo PierreCapo merged commit 7b10327 into bamlab:master Nov 15, 2020
@PierreCapo
Copy link
Contributor

I've tested and I've found nothing wrong on my side too 👍

Merged and release under 1.4.0.
Thanks a lot to everyone involved and particularly to @pxpeterxu and @hsource for this super nice improvement. Also thanks for spending time improving the example app and the readme 😃

Have a good day!

@cristianoccazinsp
Copy link
Contributor

Lovely PR! I will be giving a try within the following days (I also forgot about it for a long time :() and will provide feedback if anything breaks. Our app heavily depends on image processing so I think it will be a good test.

@cristianoccazinsp
Copy link
Contributor

cristianoccazinsp commented Nov 17, 2020

@PierreCapo @pxpeterxu I was just trying this update. Looks like after upgrading to 1.4.0, all my image resizing are suddenly not preserving aspect ratio on Android and iOS. The update was a breaking one for our app.

The following shows the before and after without any code changes (just a lib update). Looks like the defaults are not appropriate to preserve the previous behaviour:

image

Also, I'm sorry I didn't test it earlier, could have caught this before the release if I did :(

@pxpeterxu
Copy link
Contributor Author

@cristianoccazinsp I can take a look at this! Which mode were you using (stretch, cover, contain), and was it an upsize or downsize?

@cristianoccazinsp
Copy link
Contributor

@pxpeterxu I did not set any mode at all, just default options. I was expecting the updates to be non-breaking.

@cristianoccazinsp
Copy link
Contributor

Any luck reviewing this? The update has been live for some time now and there was also another report about the issue here: #265

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 this pull request may close these issues.

None yet

6 participants