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

Closed
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -184,16 +184,18 @@ private static void setTransformProperty(View view, ReadableArray transforms) {

if (perspectiveArray.length > PERSPECTIVE_ARRAY_INVERTED_CAMERA_DISTANCE_INDEX) {
float invertedCameraDistance = (float) perspectiveArray[PERSPECTIVE_ARRAY_INVERTED_CAMERA_DISTANCE_INDEX];
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.

// 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.

}
float cameraDistance = -1 / invertedCameraDistance;
float scale = DisplayMetricsHolder.getScreenDisplayMetrics().density;

// The following converts the matrix's perspective to a camera distance
// such that the camera perspective looks the same on Android and iOS
float normalizedCameraDistance = scale * cameraDistance * CAMERA_DISTANCE_NORMALIZATION_MULTIPLIER;
// The following converts the matrix's perspective to a camera distance
// such that the camera perspective looks the same on Android and iOS
float normalizedCameraDistance = scale * cameraDistance * CAMERA_DISTANCE_NORMALIZATION_MULTIPLIER;
view.setCameraDistance(normalizedCameraDistance);

view.setCameraDistance(normalizedCameraDistance);
}
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.