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

[Image] Resize mode contain produces glitches when dimension is float #1870

Closed
grabbou opened this issue Jul 4, 2015 · 11 comments
Closed

[Image] Resize mode contain produces glitches when dimension is float #1870

grabbou opened this issue Jul 4, 2015 · 11 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@grabbou
Copy link
Contributor

grabbou commented Jul 4, 2015

Steps to reproduce:
  1. Load image over HTTP with following size:
    • width: 350
    • height: 460
  2. Attach it to a styled Image element with:
    • width: 200
    • height: 200
    • resizeMode: contain
  3. Run - https://rnplay.org/apps/Z4KTfg

Output:
screen shot 2015-07-04 at 20 16 08

Image is resized and centred correctly, but because target is wider then the content, width is calculated like so:

sourceSize.width = sourceSize.height * aspect;

This makes the width to be a float 152.17391304347828 and causes a grey (I think 1px) border to appear at the right of the image. If I manipulate the content size, the grey border may appear at the bottom respectively.

[Update] Possible fixes

Ok, so I've played around with this for a while to see what's exactly causing this behaviour.

Commenting these lines https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageDownloader.m#L145-L173 solves the problem, so the issue lives there (Yay, CSS & LayoutConstraints for the win!)

The calculated boundaries should be rounded to avoid above artefacts from happening by either:

  • explicitly rounding every calculation
  • passing calculated imageRect through CGRectIntegral which will require less modifications as there are multiple return points from RCTClipRect. Not a huge fun of it though as in my understanding, RCTClipRect should return already adjusted values w/o further manipulation, but anyway worth discussing. Below is a simple demonstration how line 149 can be modified from this:
CGRect imageRect = RCTClipRect(image.size, image.scale, size, destScale, resizeMode);

to this:

CGRect imageRect = CGRectIntegral(RCTClipRect(image.size, image.scale, size, destScale, resizeMode));

If that's something that matches one of the possible ways of fixing this solution, I can submit a PR.

@nicklockwood
Copy link
Contributor

I agree that it would be better if the solution was contained within RCTClipRect. Ideally it would also take destScale factor into account, so that if the scale was 2.0, it would round to the nearest 0.5 instead of the nearest integer, if that makes sense.

@grabbou
Copy link
Contributor Author

grabbou commented Jul 7, 2015

That's actually a good point about the scale factor. I can submit a PR and we can move discussion over there.

@nicklockwood
Copy link
Contributor

Sounds good, thanks!

@grabbou
Copy link
Contributor Author

grabbou commented Jul 17, 2015

Just to let you know, I'll be working on that tomorrow (we already have fix in our forked temporary version we use to test it out before submitting).

@michaelcontento
Copy link

👍

@cssoul
Copy link

cssoul commented Jul 25, 2015

good job 👍

@nicklockwood
Copy link
Contributor

I added some fixes for this in RCTClipRect, so hopefully images should be OK now. My fixes won't affect text or border drawing though.

@ssssssssssss
Copy link

@nicklockwood, If that's the case, do you mean #2018 needs another standalone fix?

@nicklockwood
Copy link
Contributor

Yes, or at least I haven't done any work to fix that (it may have been fixed by someone else).

@ssssssssssss
Copy link

Thanks. 👍

@grabbou
Copy link
Contributor Author

grabbou commented Sep 28, 2015

@brentvatne @nicklockwood the rnplay example has no glitches on React 0.11. Closing, thanks!

@grabbou grabbou closed this as completed Sep 28, 2015
@facebook facebook locked as resolved and limited conversation to collaborators Jul 22, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

6 participants