-
Notifications
You must be signed in to change notification settings - Fork 24.8k
Android support for perspective transform #6159
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
Conversation
kevinstumpf
commented
Feb 26, 2016
- Motivation: "Perspective" Transform was so far supported only on iOS
- Closes [Android] Perspective transform styles not working #2915
- Test Plan: I built a react-native-flip-view and ensured that the same perspective transform results in the same camera perspective on both iOS and Android:
By analyzing the blame information on this pull request, we identified @kmagiera, @janicduplessis and @dgladkov to be potential reviewers. |
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@@ -149,6 +152,7 @@ public void setAccessibilityLiveRegion(T view, String liveRegion) { | |||
} | |||
} | |||
|
|||
private static final float MatrixPerspectiveToCameraDistanceMultiplier = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This should probably be written LIKE_THE_OTHER_CONSTANTS and put at the top of the file.
This looks great, thanks for working on this! cc @mkonicek |
@@ -164,6 +168,16 @@ private static void setTransformMatrix(View view, ReadableMap matrix) { | |||
(float) matrix.getDouble(PROP_DECOMPOSED_MATRIX_SCALE_X)); | |||
view.setScaleY( | |||
(float) matrix.getDouble(PROP_DECOMPOSED_MATRIX_SCALE_Y)); | |||
|
|||
if(matrix.hasKey(PROP_DECOMPOSED_MATRIX_PERSPECTIVE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Put a space after if
keywords.
@kevinstumpf updated the pull request. |
@janicduplessis -- thanks for the fast and thorough review. Addressed your comments and pushed an update. Thanks! |
cc @dmmiller |
Thanks for adding a great summary on the PR @kevinstumpf. Does the @facebook-github-bot shipit |
The perspective prop allows to flip the view along both the vertical and the horizontal axis. The react-native-flip-view module actually supports both (by specifying the "flipAxis" prop). Thanks! |
@kevinstumpf This is awesome. One think i was really stuck with when building flip on android was |
@chirag04 I did figure a way around backfaceVisibility. I will submit a PR that adds android support over the next few days! |
@mkonicek I see that this PR is still labeled as "review-needed", despite your msg to the github bot. Is there anything left on my end to satisfy the bot? (I'm seeing that the branch is out-of-date with the base branch -- do I need to update it or will the bot take care of that automatically?) Thanks! |
@kevinstumpf updated the pull request. |
@mkonicek seems like your import failed. Can you ship again if everything seems right. |
@kevinstumpf updated the pull request. |
1 similar comment
@kevinstumpf updated the pull request. |
Thanks for the ping @chirag04! The bot will now add the Accepted label so we can find PRs with the label and they don't get forgotten about. |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to Phabricator to review. |
Summary:This reverts "Android support for perspective transform": 3e760f4, PR #6159, D3053532. It broke negative scale transforms: #6622 Reviewed By: bestander Differential Revision: D3120627 fb-gh-sync-id: 727528d39c049180fe4862d006f2089c997afd45 fbshipit-source-id: 727528d39c049180fe4862d006f2089c997afd45
@kevinstumpf This commit was reverted. can you take a look here: ae3dad8 |
Thanks for commenting @chirag04! @kevinstumpf Happy to merge a new PR with an updated test plan and ideally with a test. |
Summary:This reverts "Android support for perspective transform": 3e760f4, PR facebook#6159, D3053532. It broke negative scale transforms: facebook#6622 Reviewed By: bestander Differential Revision: D3120627 fb-gh-sync-id: 727528d39c049180fe4862d006f2089c997afd45 fbshipit-source-id: 727528d39c049180fe4862d006f2089c997afd45