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

discrepancy between debug and release rendering #12

Open
mcconkiee opened this issue Nov 9, 2017 · 9 comments
Open

discrepancy between debug and release rendering #12

mcconkiee opened this issue Nov 9, 2017 · 9 comments

Comments

@mcconkiee
Copy link

Hi - I have an effect to which I am unclear the issue. When building in debug the module works as expected:
dev
however in release build, it looks like this:
release

the relative code :

render(){
const bubble = StaticIcons.imageForIcon("chat-bubble-light");
const sz = 8;
....
<ImageCapInset
          style={{
            padding: 20
          }}
          source={bubble}
          capInsets={{
            top: sz,
            right: sz,
            bottom: sz,
            left: sz
          }}
        >
          <View>
            <Text>This is a test</Text>
            <Text>This is a test</Text>
            <Text>This is a test</Text>
          </View>
        </ImageCapInset>
....
}

I am unclear how to resolve this issue. Any help is appreciated!

@hblumberg
Copy link

I have also noticed this! Any insights since your post, @mcconkiee?

@tcherna
Copy link

tcherna commented Apr 6, 2018

Hi. We're also seeing this. I think it is related to: issue on stackoverflow
and the fact that the code in RCTImageCapInsetView.java is storing the density ratio in an int.

int ratio = Math.round(bitmap.getDensity() / 160);
int top = mCapInsets.top * ratio;

@EyMaddis
Copy link

@hblumberg can you open a PR with your changes please?
Khan@063c3d5

@tcherna
Copy link

tcherna commented Apr 10, 2018

I've been trying to read about the difference in bitmap density vs. device density, re @hblumberg's change. It seems to me that ordinarily, they'd match, but if the app didn't provide all the appropriate
sized images, then wouldn't the closest bitmap be provided (with the system scaling as required). At that point, wouldn't the bitmap density be a better choice? Thoughts?

@hblumberg
Copy link

hblumberg commented Apr 10, 2018

I'd be happy to open a PR with my change, @EyMaddis, but unfortunately it doesn't fix the debug vs. release discrepancy. 1.5x devices (for example) still look distorted on debug builds, but correct on release builds.

I didn't make a PR because of @tcherna's point. I do think it's worth rounding later (after multiplying by the insets), but I'm not sure if folks rely on the existing density calculation.

@tcherna
Copy link

tcherna commented Apr 10, 2018

@hblumberg, Had you tried your code with the later rounding but using the bitmap density calculation to see how it performed with debug/release?

@tcherna
Copy link

tcherna commented Apr 12, 2018

I did some logging and testing and I actually don't see any difference in the values from getDisplayMetrics().density vs bitmap.getDensity()/160.0f. On the device where I do see a rendering error in debug, the issue is that the bitmap returned is the wrong size compared with release (it looks like its a 2x image vs the scaled image), which doesn't make sense since the factory is always supposed to return an image scaled to the target density (by default).

HotWordland added a commit to HotWordland/react-native-image-capinsets that referenced this issue May 27, 2019
madsleejensen#12
reference - 
Summary:
The previous calculation rounded the `ratio` / `density` immediately, which affected calculations. (1.5x devices were treated as 2x.)

I don't quite understand why the calculation used the bitmap density instead of the device density, but I think this is a more reliable calculation (since it doesn't depend what assets are used).

Test Plan: - (See RN side changes to the popover background.)

Reviewers: jared, nrowe

Reviewed By: jared

Subscribers: #mobile

Differential Revision: https://phabricator.khanacademy.org/D43038
@DevopsElectricJukebox
Copy link

We found this an issue too, our solution was to remove the ratio calculation and so the configured nine patch rect area always operates on actual image pixels.

DevopsElectricJukebox@e872440

@mayconmesquita
Copy link

@DevopsElectricJukebox this is working at production env? 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

No branches or pull requests

6 participants