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

Fix low-quality masks on iOS #1658

Merged

Conversation

simontreny
Copy link
Contributor

@simontreny simontreny commented Oct 26, 2021

Summary

This PR fixes low-quality masks on iOS, as reported in #1098.

This was caused by the bitmap for the mask being created with CGBitmapContextCreate() which does not take into account the device's pixel-density, unlike UIGraphicsBeginImageContextWithOptions().

To fix it, the pixel-density is retrieved using [[UIScreen mainScreen] scale] and it is used as a scale factor for the mask bitmap.

Test Plan

I've tested using this sample:

<Svg width={300} height={180}>
  <Defs>
    <LinearGradient id="redGrad" x1="0" y1="0" x2="1" y2="0">
      <Stop offset="0" stopColor="#FF0000" stopOpacity="0" />
      <Stop offset="1" stopColor="#FF0000" stopOpacity="1" />
    </LinearGradient>
    <LinearGradient id="greenGrad" x1="0" y1="0" x2="1" y2="0">
      <Stop offset="0" stopColor="#00FF00" stopOpacity="0" />
      <Stop offset="1" stopColor="#00FF00" stopOpacity="1" />
    </LinearGradient>
    <LinearGradient id="blueGrad" x1="0" y1="0" x2="1" y2="0">
      <Stop offset="0" stopColor="#0000FF" stopOpacity="0" />
      <Stop offset="1" stopColor="#0000FF" stopOpacity="1" />
    </LinearGradient>
    <Mask
      id="mask"
      x="0"
      y="0"
      width="300"
      height="180"
    >
      <Text
        fill="url(#redGrad)"
        fontSize="40"
        fontWeight="bold"
        x="20"
        y="45"
      >
        TEST MASK
      </Text>
      <Text
        fill="url(#greenGrad)"
        fontSize="40"
        fontWeight="bold"
        x="20"
        y="105"
      >
        TEST MASK
      </Text>
      <Text
        fill="url(#blueGrad)"
        fontSize="40"
        fontWeight="bold"
        x="20"
        y="165"
      >
        TEST MASK
      </Text>
    </Mask>
  </Defs>
  <Rect x="0" y="0" width="300" height="180" fill="red" mask="url(#mask)" />
</Svg>

Results:

Before After
Simulator Screen Shot - iPhone 13 - 2021-10-25 at 23 15 36 Simulator Screen Shot - iPhone 13 - 2021-10-25 at 23 19 28

@bhishaksanyal
Copy link

Do you plan a new release soon? I have great hopes that this PR could solve many issues in my app 🙏🏻

@@ -202,18 +202,23 @@ - (void)renderTo:(CGContextRef)context rect:(CGRect)rect
CGSize boundsSize = bounds.size;
CGFloat height = boundsSize.height;
CGFloat width = boundsSize.width;
CGFloat scale = [[UIScreen mainScreen] scale];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we want to support macOS, you should not use UIScreen, but rather add the option for both platforms analogous to how it is done e.g. with UIColor, so just add new define #define RNSVGScreen UIScreen here: https://github.com/react-native-svg/react-native-svg/blob/39c8332f4dac8acfe1c60338ad4a8f8231538c91/apple/RNSVGUIKit.h#L12 and #define RNSVGScreen NSScreen here: https://github.com/react-native-svg/react-native-svg/blob/39c8332f4dac8acfe1c60338ad4a8f8231538c91/apple/RNSVGUIKit.h#L20 and then use RNSVGScreen instead of UIScreen in your PR.

@simontreny
Copy link
Contributor Author

Thanks for the feedback @WoLewicki ! I took it into account but I had to use a different approach as the scale property is called backingScaleFactor on macOS. I was not able to test the changes on macOS though. Please let me know what you think.

@simontreny simontreny requested a review from WoLewicki February 27, 2022 21:28
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

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.

4 participants