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

feat: transform-origin #37606

Conversation

intergalacticspacehighway
Copy link
Contributor

@intergalacticspacehighway intergalacticspacehighway commented May 27, 2023

Checklist

  • iOS new arch
  • Android new arch
  • iOS/Android old arch
  • Add examples

Summary

  • This PR adds transform-origin support, similar to web. Spec

Why?

By default, rotate/scale/skew transforms occur around the center of the View. If we want to perform them around the top-left of the view, we need to first translate to the top/left point, perform the transform, and then translate back to the center. This can be achieved with the following steps:

 transform: [
   { translateX: -viewWidth / 2 }, 
   { translateY: -viewHeight / 2 }, 
   { scale: animatedV.value }, 
   { translateX: viewWidth / 2 }, 
   { translateY: viewHeight / 2 }
]

The above approach requires View dimensions, that need to be calculated using onLayout, measure, or using constants. Transform origin can simplify it and also bring it closer to the web feature parity.

Changelog:

[GENERAL] [ADDED] - transform-origin.

Test Plan:

  • Run pod install/rebuild android RN tester app and test transform origin example in Transform examples.
  • Test on fabric/paper architecture.

intergalacticspacehighway referenced this pull request in intergalacticspacehighway/react-native May 27, 2023
@facebook-github-bot 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 May 27, 2023
@analysis-bot
Copy link

analysis-bot commented May 27, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,043,848 -1,379
android hermes armeabi-v7a 8,292,445 -2,054
android hermes x86 9,561,193 -227
android hermes x86_64 9,401,751 -2,002
android jsc arm64-v8a 9,603,220 -1,199
android jsc armeabi-v7a 8,729,244 -1,890
android jsc x86 9,691,421 -44
android jsc x86_64 9,935,977 -1,831

Base commit: c803a5b
Branch: main

@intergalacticspacehighway intergalacticspacehighway marked this pull request as ready for review May 28, 2023 17:51
Copy link
Member

@javache javache left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this! I have some concerns about how this works when you just change the transformOrigin prop and the transform remains stable.

@javache
Copy link
Member

javache commented May 31, 2023

High-level thought on the approach here: Could we applytransformOrigin as we calculate the transform matrix on the ShadowNode/ShadowView instead, and avoid having to mutate the transform in the mounting layer?

An example of what's going to be incorrect here now is any of the measure callbacks which respect transforms:

layoutMetrics.frame = layoutMetrics.frame * ancestorNode.getTransform();

@intergalacticspacehighway
Copy link
Contributor Author

intergalacticspacehighway commented Jun 4, 2023

High-level thought on the approach here: Could we applytransformOrigin as we calculate the transform matrix on the ShadowNode/ShadowView instead, and avoid having to mutate the transform in the mounting layer?

i am currently mutating it from the mounting layer because we need the View dimensions for transform origin. From high level i can think of two approaches, either get the transform in shadow node calculation from the native view or create a state for transforms in shadow node and reset it from the mounting layer. Does this make any sense?

@javache
Copy link
Member

javache commented Jun 5, 2023

i am currently mutating it from the mounting layer because we need the View dimensions for transform origin.

ShadowNode / ShadowView have the full layout information available, so ideally getTransform correctly represents the transform, accounting for the transformOrigin. I'd recommend making the change at that layer.

Please also add a test that shows this works correctly when animations of the transform property are used.

@intergalacticspacehighway
Copy link
Contributor Author

@javache can you check this approach? https://github.com/facebook/react-native/compare/main...intergalacticspacehighway:react-native:f/transform-origin-resolve?expand=1

It adds a resolveTransform function that returns a new transform based on the transform-origin and layoutMetrics. This makes sure we get the updated transform in shadow node's getTransform. This won't require us to mutate the transform prop. (only tested on fabric iOS. If this looks good, I can try to add it for Android/old Arch)

@javache
Copy link
Member

javache commented Jun 13, 2023

@intergalacticspacehighway That looks very promising! Can you add it here, and I'll add some comments?

@intergalacticspacehighway
Copy link
Contributor Author

@javache sorry for the delay, been out for a trip. I just merged the new approach here so you can review. I'll add the support for old arch and android.

@javache javache requested a review from sammy-SC June 19, 2023 09:54
Comment on lines +446 to +450
std::istringstream iss(transformOrigin);
std::string part;
for (int i = 0; std::getline(iss, part, ' ') && i < 3; i++) {
auto percentPos = part.find('%');
bool isPercent = percentPos != std::string::npos;
Copy link
Member

Choose a reason for hiding this comment

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

istringstream and getline are pretty inefficient string parsing methods.

I'd use std::find instead to find the next space (or end), then use string_view (like here) to parse it.

Copy link
Member

@javache javache Jun 19, 2023

Choose a reason for hiding this comment

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

Instead of parsing this on every transform calculation, consider parsing it as and storing it as a YGValue, using YGUnitPercent, YGUnitPoint and YGResolveValue

Copy link
Member

Choose a reason for hiding this comment

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

TODO: this still needs to move away from getline, and look at adopting YGValue to avoid parsing this on every layout change

Comment on lines +453 to +460
} else if (part == "top") {
origin[1] = 0.0f;
} else if (part == "bottom") {
origin[1] = static_cast<float>(viewHeight);
} else if (part == "left") {
origin[0] = 0.0f;
} else if (part == "right") {
origin[0] = static_cast<float>(viewWidth);
Copy link
Member

Choose a reason for hiding this comment

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

This will currently accept the input "top left", which is not spec complaint. Do browsers accept?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, the top left should work. It'll result in -viewWidth/2 , -viewHeight/2 as the offsets. It also works in any order in browser i.e. top left, left top. Invalid inputs could be left left, that results in -viewWidth/2 0 offsets so it also works same as browsers. left, right affects horizontal offset and top ,bottom vertical and center is the initial value - Spec. So, right always take priority over center even if comes later in order e.g. right center, center right results in the same origin. I can surely add test cases!
I am out this week as well. I'll pick this up as soon as I am back home! 😅

@intergalacticspacehighway
Copy link
Contributor Author

@javache I've updated the code so it works on old/new arch + android/ios. However, I think there is a fundamental issue with the approach I have taken to make it work on Android and iOS old arch. Ideally, I want to implement a solution where it triggers transform updates from cpp/shadow node realm, so I don't have to repeat things for respective platforms.
i.e. once we have the layout info in shadow nodes we can trigger an imperative update call to native side to update the view transforms and not use the existing setter prop approach. wdyt? I'll have to dig more for this. Let me know what you think! The current approach works with native driver as well but has some redundancy/not very clean.

Copy link
Member

@javache javache left a comment

Choose a reason for hiding this comment

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

The duplication for the old architecture is fine. No way to avoid that right now.

Do you want to split this in a separate diff for iOS and Android (and potentially even old and new architecture) to make this more reviewable?

@@ -9,6 +9,6 @@

@interface RCTConvert (Transform)

+ (CATransform3D)CATransform3D:(id)json;
+ (CATransform3D)CATransform3D:(id)json viewWidth: (CGFloat) viewWidth viewHeight: (CGFloat) viewHeight transformOrigin: (NSString*) transformOrigin;
Copy link
Member

Choose a reason for hiding this comment

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

For backwords compatibility we should keep the old method signature around too (but have it just call the new method with 0 widht/height and nil origin)

return transform;
}

+ (NSArray *)getTranslateForTransformOrigin:(CGFloat)viewWidth viewHeight:(CGFloat)viewHeight transformOrigin
Copy link
Member

Choose a reason for hiding this comment

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

Put this in a static (local) method, no need to have it live on RCTConvert

+ (NSArray *)getTranslateForTransformOrigin:(CGFloat)viewWidth viewHeight:(CGFloat)viewHeight transformOrigin
:(NSString*)transformOrigin {
if (transformOrigin.length == 0 || (viewWidth == 0 && viewHeight == 0)) {
return @[@(0.0), @(0.0), @(0.0)];
Copy link
Member

Choose a reason for hiding this comment

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

Can we return nil in this case, and use that to avoid a call to CATransform3DTranslate?

Comment on lines +71 to +74
CGFloat translateX = [offsets[0] floatValue];
CGFloat translateY = [offsets[1] floatValue];
CGFloat translateZ = [offsets[2] floatValue];
transform = CATransform3DTranslate(transform, translateX, translateY, translateZ);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CGFloat translateX = [offsets[0] floatValue];
CGFloat translateY = [offsets[1] floatValue];
CGFloat translateZ = [offsets[2] floatValue];
transform = CATransform3DTranslate(transform, translateX, translateY, translateZ);
if (offset) {
transform = CATransform3DTranslate(transform, [offsets[0] floatValue], [offsets[1] floatValue], [offsets[2] floatValue]);
}

@@ -141,7 +148,50 @@ + (CATransform3D)CATransform3D:(id)json
RCTLogInfo(@"Unsupported transform type for a CATransform3D: %@.", property);
}
}

transform = CATransform3DTranslate(transform, -translateX, -translateY, -translateZ);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
transform = CATransform3DTranslate(transform, -translateX, -translateY, -translateZ);
if (offset) {
transform = CATransform3DTranslate(transform, -translateX, -translateY, -translateZ);
}

MatrixMathHelper.multiplyInto(result, result, helperMatrix);
}

public static float[] getTranslateForTransformOrigin(float viewWidth, float viewHeight, String transformOrigin) {
Copy link
Member

Choose a reason for hiding this comment

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

private

@@ -60,6 +60,10 @@ public static void processTransform(ReadableArray transforms, double[] result) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Is the transformOrigin already applied in the codepath above?

Comment on lines +64 to +65
MatrixMathHelper.applyTranslate3D(helperMatrix, offsets[0], offsets[1], offsets[2]);
MatrixMathHelper.multiplyInto(result, result, helperMatrix);
Copy link
Member

Choose a reason for hiding this comment

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

result is the identity matrix at this point, no multiplication should be required to do this.

Comment on lines +432 to +437
auto newTransform = Transform{};
std::array<float, 3> translateOffsets = getTranslateForTransformOrigin(viewWidth, viewHeight);
newTransform = newTransform * Transform::Translate(translateOffsets[0], translateOffsets[1], translateOffsets[2]);
newTransform = newTransform * transform;
newTransform = newTransform * Transform::Translate(-translateOffsets[0], -translateOffsets[1], -translateOffsets[2]);
return newTransform;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto newTransform = Transform{};
std::array<float, 3> translateOffsets = getTranslateForTransformOrigin(viewWidth, viewHeight);
newTransform = newTransform * Transform::Translate(translateOffsets[0], translateOffsets[1], translateOffsets[2]);
newTransform = newTransform * transform;
newTransform = newTransform * Transform::Translate(-translateOffsets[0], -translateOffsets[1], -translateOffsets[2]);
return newTransform;
std::array<float, 3> translateOffsets = getTranslateForTransformOrigin(viewWidth, viewHeight);
auto newTransform = Transform::Translate(translateOffsets[0], translateOffsets[1], translateOffsets[2]);
newTransform = newTransform * transform;
newTransform = newTransform * Transform::Translate(-translateOffsets[0], -translateOffsets[1], -translateOffsets[2]);
return newTransform;

Comment on lines +446 to +450
std::istringstream iss(transformOrigin);
std::string part;
for (int i = 0; std::getline(iss, part, ' ') && i < 3; i++) {
auto percentPos = part.find('%');
bool isPercent = percentPos != std::string::npos;
Copy link
Member

Choose a reason for hiding this comment

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

TODO: this still needs to move away from getline, and look at adopting YGValue to avoid parsing this on every layout change

@github-actions github-actions bot added the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 19, 2023
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 20, 2023
@intergalacticspacehighway
Copy link
Contributor Author

intergalacticspacehighway commented Jul 23, 2023

@javache Thank you for reviewing. I have made separate PRs and incorporated the changes you suggested. We can discuss it there.
iOS old arch - #38626
Android - #38558
iOS fabric arch - #38559

@javache
Copy link
Member

javache commented Aug 21, 2023

Sorry for the delayed reviews, I was on parental leave. I'll get to this this week!

@javache
Copy link
Member

javache commented Aug 23, 2023

Closing this PR as reviews are happening on the split PR's.

@javache javache closed this Aug 23, 2023
javache pushed a commit to javache/react-native that referenced this pull request Sep 1, 2023
Summary:
This PR adds transform-origin support for iOS fabric. This PR also incorporates feedback/changes suggested by javache in the original [PR.](facebook#37606)

## Changelog:
[IOS] [ADDED] - Fabric Transform origin
<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38559

Test Plan: Run iOS RNTester app in old architecture and test transform-origin example in `TransformExample.js`.

Differential Revision: D48528363

Pulled By: javache

fbshipit-source-id: 21cfe06db750c8cf55d43039f0189089d29fca6f
javache pushed a commit to javache/react-native that referenced this pull request Sep 1, 2023
Summary:
This PR adds transform-origin support for android to make it easier to review. facebook#37606 (review) by javache. I'll answer feedback from javache below.

## Changelog:
[Android] [ADDED] - Transform origin
<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: facebook#38558

Test Plan: Run iOS RNTester app in old architecture and test transform-origin example in `TransformExample.js`.

Differential Revision: D48528339

Pulled By: javache

fbshipit-source-id: 09e0c9ef569b7e9131da2f6efa9ba057aa98ff82
facebook-github-bot pushed a commit that referenced this pull request Sep 1, 2023
Summary:
This PR adds transform-origin support for iOS fabric. This PR also incorporates feedback/changes suggested by javache in the original [PR.](#37606)

## Changelog:
[IOS] [ADDED] - Fabric Transform origin
<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #38559

Test Plan: Run iOS RNTester app in old architecture and test transform-origin example in `TransformExample.js`.

Reviewed By: NickGerleman

Differential Revision: D48528363

Pulled By: javache

fbshipit-source-id: 347b7c5896ad19ad24278de81b0e055e4cb01016
facebook-github-bot pushed a commit that referenced this pull request Sep 1, 2023
Summary:
This PR adds transform-origin support for android to make it easier to review. #37606 (review) by javache. I'll answer feedback from javache below.

## Changelog:
[Android] [ADDED] - Transform origin
<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

[ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #38558

Test Plan: Run iOS RNTester app in old architecture and test transform-origin example in `TransformExample.js`.

Reviewed By: NickGerleman

Differential Revision: D48528339

Pulled By: javache

fbshipit-source-id: e255a7f364e57396dada60b2c69c724cec406f51
@intergalacticspacehighway intergalacticspacehighway deleted the feat/transform-origin-1 branch August 9, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants