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

always set camera distance on transforms, to default 1280 if 0 #14646

Conversation

Projects
None yet
6 participants
@bartolkaruza
Copy link
Collaborator

commented Jun 20, 2017

This is the better fix for the same issue as mentioned in PR #14560

Certain rotateX, rotateY, scaleX and scaleY animations do not work correctly on some phones in Android 7.0.0, causing issues such as #14462 and #13522.

The issue can be fixed on JS side by setting an additional transform for perspective, eg. {perspective: 1} which triggers a setCameraDistance call in native code.

The fix in this PR always sets the camera distance on transforms, even when no perspective transform was specified. The default camera distance is set before the scale multiplication, to make sure that the value is appropriate for the phones density. The value calculates to an Android 'default' camera distance of 1280 * scale multiplier; https://developer.android.com/reference/android/view/View.html#setCameraDistance(float)

If a perspective transform is specified, this value will be used correctly still.

This fix was tested on the RNTester. Before the fix, on some devices, the FlatList example, with inverted turned on, will not display the list.

Devices that have been confirmed to have this issue:
FRD-AL10(honor 8) EMUI:5.0 android: 7.0
MHA-AL00(Mate9) EMUI:5.0 android:7.0
Huawei P10 VTR-L09, running Android 7.0

After the fix, the inverted FlatList displays correctly.

@bartolkaruza

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 24, 2017

@janicduplessis Could you help getting this looked at by someone? The pull request has been sitting in 'waiting for review' for more than a month. Thank you

@pull-bot

This comment has been minimized.

Copy link

commented Jul 24, 2017

Warnings
⚠️

📋 Test Plan - This PR appears to be missing a Test Plan.

Generated by 🚫 dangerJS

@janicduplessis

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2017

@bartolkaruza Sorry about the delay, I forgot about this.

@hramos Could you get someone @ fb to look at this?

@hramos

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

float scale = DisplayMetricsHolder.getScreenDisplayMetrics().density;
if (invertedCameraDistance == 0) {
// Default camera distance, before scale multiplier (1280)
invertedCameraDistance = 0.00078125f;

This comment has been minimized.

Copy link
@shergin

shergin Jul 25, 2017

Contributor

Should It be negative?

This comment has been minimized.

Copy link
@bartolkaruza

bartolkaruza Jul 25, 2017

Author Collaborator

This is the exact value that comes through if you pass a {perspective: 1280} value in the transform array on the JS side. When omitting a perspective transform, the value is 0. With this change, the value is forced from 0 to the default 1280 value, forcing the . setCameraDistance call.

Not calling .setCameraDistance is causing the rendering issue, which is caused by a 0 invertedCameraDistance.

if (invertedCameraDistance < 0) {
float cameraDistance = -1 / invertedCameraDistance;
float scale = DisplayMetricsHolder.getScreenDisplayMetrics().density;
if (invertedCameraDistance == 0) {

This comment has been minimized.

Copy link
@shergin

shergin Jul 25, 2017

Contributor

In which cases invertedCameraDistance is 0 and why?
Does 0 make any logical sense as invertedCameraDistance value?

This comment has been minimized.

Copy link
@bartolkaruza

bartolkaruza Jul 25, 2017

Author Collaborator

0 is the value when no perspective transform is passed in the array in javascript. The deleted line if (invertedCameraDistance < 0) { was preventing that case from causing a division by 0 and a crash, but it was also preventing a call to .setCameraDistance which is causing the rendering issues. This change affects the 0 case, by forcing it to a sensible default other than 0.

@shergin

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

@bartolkaruza Seems reasonable to me. Thank you!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jul 25, 2017

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

kashyapp added a commit to udaan-com/react-native-invertible-scroll-view that referenced this pull request Sep 15, 2017

Fix for not rendering on some Android 7 devices
such as Huawei Honor

Full explanation is here facebook/react-native#14646

This patch is not needed once we move to RN 0.48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.