Fix ReactNativeART arc drawing on Android #7049

MikeOrtiz · 2016-04-19T01:41:34Z

Motivation: Arc drawing has been broken on Android for some time. @dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works.

Reproducing the Bug: dgladkov made a simple test app which helps to illustrate the bug. The repo can be found here. The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. Direct Link to iOS Before. Direct Link to Android Before.

Proof The Bug is Fixed: Here is a direct link to Android After pic. You can see the wedges match the iOS Before screenshot.

What went wrong: dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should always be positive. Java returns the remainder, which can be negative. -120 % 360 will return 240 in Python, but -120 in Java. I created a modulus method which mimics Python's behavior. Math.floorMod(), introduced in Java 8, also gets the job done. See this StackOverflow answer for more information regarding modulus.

facebook-github-bot · 2016-04-19T01:42:19Z

By analyzing the blame information on this pull request, we identified @spicyj and @lexs to be potential reviewers.

facebook-github-bot · 2016-04-19T01:42:20Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

sophiebits · 2016-04-19T02:41:11Z

To be fair, lots of languages implement % like this.

sophiebits · 2016-04-19T02:42:01Z

ReactAndroid/src/main/java/com/facebook/react/views/art/ARTShapeShadowNode.java

          if (!clockwise) {
-            end = 360 - end;
+            start = end;
+            sweep = 360 - sweep;


If start === end already and sweep is 0, is this correct?

MikeOrtiz · 2016-04-19T19:08:09Z

@spicyj Good catch. That edge case would create a full circle in counter-clockwise mode. I have a fix in place, but first a question: what is the expected result of an arc with startAngle = -360 and endAngle = 20? In my implementation, the result is a full circle, but I guess the argument could be made for a 20 degree arc. The ART docs, unfortunately, do not describe Path.arc().

sophiebits · 2016-04-19T19:20:14Z

I think whatever SVG does.

https://github.com/sebmarkbage/art/blob/51ffce8164a555d652843241c2fdda52e186cbbd/modes/svg/path.js#L13

facebook-github-bot · 2016-04-20T00:33:39Z

@MikeOrtiz updated the pull request.

MikeOrtiz · 2016-04-20T00:33:58Z

I decided it was best to mimic the behavior of iOS. If the sweep > 360, then a full circle is always drawn. If sweep = 0, no wedge is drawn. Comparison screenshots: iOS and Android. The start/end angles are as follows:

Blue: start: 120, end: 220
Red: start: 120, end: 120
Green: start: -360, end: 20
Orange: start: 120, end: 20

facebook-github-bot · 2016-04-20T00:34:13Z

@MikeOrtiz updated the pull request.

facebook-github-bot · 2016-04-20T22:04:14Z

@MikeOrtiz updated the pull request.

MikeOrtiz · 2016-04-20T22:04:28Z

@spicyj, I realized I didn't mention you in my last post. Not sure if that makes a difference.

MikeOrtiz · 2016-04-21T19:12:38Z

The post isn't yet tagged as such, but I have signed the CLA.

facebook-github-bot · 2016-04-21T19:13:14Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

mkonicek · 2016-04-22T15:00:31Z

Assigning to @spicyj who started reviewing this PR.

tomauty · 2016-04-27T18:37:53Z

Hooray :) This is a blocker for my project.

sophiebits · 2016-04-27T18:57:53Z

Can you say whether this matches the SVG behavior now? I am fairly sure that is what ART intends to do for this path component.

tomauty · 2016-04-27T19:03:00Z

The fixed behavior seems to conform to intentions. The repo provided uses the Wedge component from react-art, in any case.

sophiebits · 2016-04-27T19:36:03Z

Yeah, that seems fair. @facebook-github-bot shipit

sophiebits · 2016-04-28T04:17:38Z

@facebook-github-bot shipit

ghost · 2016-04-28T04:20:48Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

sophiebits · 2016-04-28T22:46:35Z

It looks like this breaks circle rendering:

const {Surface, Group} = require('ReactART');
const Circle = require('Circle.art');

...

  <Surface width={200} height={200}>
    <Group x={100} y={100}>
      <Circle radius={50} stroke="green" strokeWidth={3} fill="blue" />
    </Group>
  </Surface>

(where Circle.art comes from https://github.com/reactjs/react-art/blob/d1eda4fd8e3ee9462c32d2f611923697195fee14/src/Circle.art.js).

There should not be a line in the middle. I have to revert this commit (13acd7e), but would you want to send a follow-up that fixes this?

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes #7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 6b85eb42389da6c344ec9723c7f81f61473246b0 fbshipit-source-id: 6b85eb42389da6c344ec9723c7f81f61473246b0

MikeOrtiz · 2016-04-29T00:43:59Z

Aww, yeah I'll look into it. When should I submit the fix to get it in for the upcoming release?

sophiebits · 2016-04-29T01:10:42Z

I think we cut a release every other Sunday, including this weekend.

MikeOrtiz · 2016-04-29T07:00:57Z

I just ran this on my machine and it looks fine. This is what it looks like for me. The call to close() in Circle.art is what will create the line segment to close off the final arc. If the path ends at the same point that it starts, there will be no line segment. This functionality is described in the Android Path docs here. The arcs are centered around the same coordinate and the second ends at the same angle the first begins, so the starting points should be identical. I printed some logs of Circle.art to confirm that fact:

04-29 01:54:38.488 1972-2038/com.testproject D/dsf: x: 140.0 y: 280.0 r: 140.0 start: -90.0 end: 90.0 sweep: 180.0
04-29 01:54:38.488 1972-2038/com.testproject D/dsf: x: 140.0 y: 280.0 r: 140.0 start: 90.0 end: -90.0 sweep: 180.0

I don't understand how you got this rendering, but I can't reproduce it.

One simple fix would be to remove the call to close() in Circle.art as it doesn't achieve anything, anyway.

sophiebits · 2016-04-29T21:29:33Z

Hm, I'm not sure. I was testing on a Nexus 4 simulator on 4.4.4 (API 19).

MikeOrtiz · 2016-05-01T21:42:56Z

@spicyj It looks like my changes weren't what broke this. The circle draws correctly up to #4e5c85 of my repo. At some point after that, someone introduced breaking code and my change exposed that break. I will run git bisect when I have time later today to pinpoint the breaking commit.

sophiebits · 2016-05-02T00:38:39Z

Sounds good.

MikeOrtiz · 2016-05-02T19:22:10Z

I couldn't successfully bisect the commits. After commit #47a470a, I get the red screen of death with the message "Couldn't get the native call queue...". Restarting package manager, reinstalling, etc didn't fix the issue. The message shows for this commit and all subsequent commits. Any hint would could be going wrong?

Circle rendered fine in the commit before that though.

sophiebits · 2016-05-02T20:12:50Z

I'm not sure, sorry. Does master work for you? (I don't really work on Android, or on React Native for that matter.)

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 4974b818dc49d9d16f2483c49b462c459a8bb479 fbshipit-source-id: 4974b818dc49d9d16f2483c49b462c459a8bb479

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 6b85eb42389da6c344ec9723c7f81f61473246b0 fbshipit-source-id: 6b85eb42389da6c344ec9723c7f81f61473246b0

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 4974b818dc49d9d16f2483c49b462c459a8bb479 fbshipit-source-id: 4974b818dc49d9d16f2483c49b462c459a8bb479

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 6b85eb42389da6c344ec9723c7f81f61473246b0 fbshipit-source-id: 6b85eb42389da6c344ec9723c7f81f61473246b0

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 4974b818dc49d9d16f2483c49b462c459a8bb479 fbshipit-source-id: 4974b818dc49d9d16f2483c49b462c459a8bb479

Summary: **Motivation**: Arc drawing has been broken on Android for some time. dgladkov submitted a PR, which ended up having a bug and was never merged. This PR should fix that bug as well as provide screenshots to prove it works. **Reproducing the Bug:** dgladkov made a simple test app which helps to illustrate the bug. The repo can be found [here](https://github.com/dgladkov/RNArtArcDrawingBug). The demo app illustrates that on iOS, wedges are drawn correctly, but Android only draws full circles. [Direct Link to iOS Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/ios.png). [Direct Link to Android Before](https://github.com/dgladkov/RNArtArcDrawingBug/blob/master/images/android.png). **Proof The Bug is Fixed:** [Here is a direct link to Android After pic.](http://i.imgur.com/9dTU2Xn.png) You can see the wedges match the iOS Before screenshot. **What went wrong:** dgladkov's solution relied on Java's modulus, which in fact, implements modulus in a non-standard way. Modulus should a Closes facebook#7049 Differential Revision: D3234404 Pulled By: spicyj fb-gh-sync-id: 6b85eb42389da6c344ec9723c7f81f61473246b0 fbshipit-source-id: 6b85eb42389da6c344ec9723c7f81f61473246b0

Fix ReactNativeART arc drawing on Android

eee2fd7

facebook-github-bot added the GH Review: review-needed label Apr 19, 2016

MikeOrtiz mentioned this pull request Apr 19, 2016

fix ReactNativeART arc drawing on Android #5856

Closed

sophiebits reviewed Apr 19, 2016
View reviewed changes

Merge remote-tracking branch 'facebook/master'

58d5d24

Arc drawing edge case fixed on Android. Functionality matches iOS.

417a3e2

Merge branch 'master' into master

6aada34

Merge branch 'master' into master

e998c15

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 21, 2016

mkonicek assigned sophiebits Apr 22, 2016

ghost added GH Review: accepted and removed GH Review: review-needed labels Apr 28, 2016

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 28, 2016

ghost closed this in 1af4760 Apr 28, 2016

mehdichamouma mentioned this pull request Jun 11, 2016

Implementation of ART.Path.arcTo on Android Renders Incorrectly #5311

Closed

This pull request was closed.

Fix ReactNativeART arc drawing on Android #7049

Fix ReactNativeART arc drawing on Android #7049

Uh oh!

Conversation

MikeOrtiz commented Apr 19, 2016

Uh oh!

facebook-github-bot commented Apr 19, 2016

Uh oh!

facebook-github-bot commented Apr 19, 2016

Uh oh!

sophiebits commented Apr 19, 2016

Uh oh!

sophiebits Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

MikeOrtiz commented Apr 19, 2016

Uh oh!

sophiebits commented Apr 19, 2016

Uh oh!

facebook-github-bot commented Apr 20, 2016

Uh oh!

MikeOrtiz commented Apr 20, 2016

Uh oh!

facebook-github-bot commented Apr 20, 2016

Uh oh!

facebook-github-bot commented Apr 20, 2016

Uh oh!

MikeOrtiz commented Apr 20, 2016

Uh oh!

MikeOrtiz commented Apr 21, 2016

Uh oh!

facebook-github-bot commented Apr 21, 2016

Uh oh!

mkonicek commented Apr 22, 2016

Uh oh!

tomauty commented Apr 27, 2016

Uh oh!

sophiebits commented Apr 27, 2016

Uh oh!

tomauty commented Apr 27, 2016

Uh oh!

sophiebits commented Apr 27, 2016

Uh oh!

sophiebits commented Apr 28, 2016

Uh oh!

ghost commented Apr 28, 2016

Uh oh!

sophiebits commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeOrtiz commented Apr 29, 2016

Uh oh!

sophiebits commented Apr 29, 2016

Uh oh!

MikeOrtiz commented Apr 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sophiebits commented Apr 29, 2016

Uh oh!

MikeOrtiz commented May 1, 2016

Uh oh!

sophiebits commented May 2, 2016

Uh oh!

MikeOrtiz commented May 2, 2016

Uh oh!

sophiebits commented May 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sophiebits commented Apr 28, 2016 •

edited

Loading

MikeOrtiz commented Apr 29, 2016 •

edited

Loading