-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
feat: transform-origin #37606
Conversation
packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm
Outdated
Show resolved
Hide resolved
Base commit: c803a5b |
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.
Thanks for contributing this! I have some concerns about how this works when you just change the transformOrigin prop and the transform remains stable.
packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
Outdated
Show resolved
Hide resolved
packages/react-native/React/Fabric/Mounting/ComponentViews/View/RCTViewComponentView.mm
Outdated
Show resolved
Hide resolved
packages/react-native/React/Fabric/Mounting/RCTMountingManager.mm
Outdated
Show resolved
Hide resolved
...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java
Outdated
Show resolved
Hide resolved
...es/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java
Outdated
Show resolved
Hide resolved
High-level thought on the approach here: Could we apply An example of what's going to be incorrect here now is any of the measure callbacks which respect transforms: react-native/packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp Line 101 in 0e83c53
|
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? |
ShadowNode / ShadowView have the full layout information available, so ideally Please also add a test that shows this works correctly when animations of the transform property are used. |
@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) |
@intergalacticspacehighway That looks very promising! Can you add it here, and I'll add some comments? |
6a43736
to
f39ba67
Compare
@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. |
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; |
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.
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.
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.
Instead of parsing this on every transform calculation, consider parsing it as and storing it as a YGValue, using YGUnitPercent
, YGUnitPoint
and YGResolveValue
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.
TODO: this still needs to move away from getline, and look at adopting YGValue
to avoid parsing this on every layout change
} 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); |
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.
This will currently accept the input "top left", which is not spec complaint. Do browsers accept?
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.
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! 😅
@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. |
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.
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; |
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.
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 |
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.
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)]; |
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.
Can we return nil
in this case, and use that to avoid a call to CATransform3DTranslate
?
CGFloat translateX = [offsets[0] floatValue]; | ||
CGFloat translateY = [offsets[1] floatValue]; | ||
CGFloat translateZ = [offsets[2] floatValue]; | ||
transform = CATransform3DTranslate(transform, translateX, translateY, translateZ); |
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.
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); |
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.
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) { |
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.
private
@@ -60,6 +60,10 @@ public static void processTransform(ReadableArray transforms, double[] result) { | |||
return; |
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.
Is the transformOrigin already applied in the codepath above?
MatrixMathHelper.applyTranslate3D(helperMatrix, offsets[0], offsets[1], offsets[2]); | ||
MatrixMathHelper.multiplyInto(result, result, helperMatrix); |
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.
result
is the identity matrix at this point, no multiplication should be required to do this.
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; |
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.
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; |
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; |
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.
TODO: this still needs to move away from getline, and look at adopting YGValue
to avoid parsing this on every layout change
packages/react-native/Libraries/Components/View/ReactNativeStyleAttributes.js
Show resolved
Hide resolved
Sorry for the delayed reviews, I was on parental leave. I'll get to this this week! |
Closing this PR as reviews are happening on the split PR's. |
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
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
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
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
Checklist
Summary
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:
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: