feat(iOS/fabric): percentage support in translate #43192

intergalacticspacehighway · 2024-02-25T14:17:47Z

Summary:

This PR adds percentage support in translate properties for new arch iOS. Isolating this PR for easier reviews.

The approach taken here introduces usage of ValueUnit struct for transform operations so it can support % in translates and delay the generation of actual transform matrix until view dimensions are known. I have tried to keep the changes minimal and reuse existing APIs, open to changes if there's an alternative approach.

Changelog:

[IOS] [ADDED] - Percentage support in translate in new arch.

Test Plan:

Checkout TransformExample.js -> Translate percentage example.
Added a simple test in processTransform-test.js. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!)

Related PRs - #43193, #43191

NickGerleman

Overall LGTM (left a couple of nits). I think @javache was wanting to take a closer look at this, who is more familiar with some of this code.

When we import this, we should add a quick screenshot test to the RNTester example added.

NickGerleman · 2024-02-26T22:08:22Z

packages/react-native/Libraries/StyleSheet/processTransform.js

+      invariant(
+        typeof value === 'number' ||
+          (typeof value === 'string' && value.endsWith('%')),
+        'Transform with key of "%s" must be number or a percentage. Passed value: %s.',
+        key,
+        stringifySafe(transformation),
+      );


It looks like this is an existing pattern, but for the future, we've been trying to move towards no-oping on invalid values, instead of creating a redbox, or something more disruptive.

Even longer into the future (not worth thinking about yet), on Fabric side, we want to just do all this parsing in native.

All of this parsing does happen in native. validateTransform is just an additional DEV-time only validation of the input to get better signal to the developer.

Yep, but we have though redbox during development on invalid style is too aggressive/disruptive.

NickGerleman · 2024-02-26T22:13:02Z

packages/react-native/ReactCommon/react/renderer/graphics/ValueUnit.cpp

+ValueUnit ValueUnit::getValueUnitFromRawValue(const RawValue& value) {
+  if (value.hasType<Float>()) {
+    auto number = (float)value;
+    return ValueUnit(number, UnitType::Point);
+  } else if (value.hasType<std::string>()) {
+    const auto stringValue = (std::string)value;
+    if (stringValue.back() == '%') {
+      auto tryValue = folly::tryTo<float>(
+          std::string_view(stringValue).substr(0, stringValue.length() - 1));
+      if (tryValue.hasValue()) {
+        return ValueUnit(tryValue.value(), UnitType::Percent);
+      }
+    }
+  }
+  return ValueUnit();
+};


I think I still need to hook this up to OSS build more generally, but this code will eventually be replaced with a couple bits already landed.

CSSValueVariant<CSSLength, CSSPercentage>

Along with the equivalent parseCSSProp or parseCSSComponentValue.

Feel free not to use those yet though, since it's not yet hooked up more broadly.

I don't think we'd want to use these here though @NickGerleman, since the code is fully layout independent? Or are you happy for these CSS utils to be exposed for non-layout purposes?

I am planning to move the CSS values to be used in View/Image code as well, but am wanting to start with layout (mostly because it can happen before a chunk of work to change how styles in view managers work).

packages/react-native/ReactCommon/react/renderer/graphics/Transform.h

NickGerleman · 2024-02-26T22:18:35Z

packages/react-native/ReactCommon/react/renderer/graphics/Transform.cpp

@@ -262,6 +279,14 @@ bool Transform::operator==(const Transform& rhs) const {
      return false;
    }
  }
+  if (this->operations.size() != rhs.operations.size()) {


nit: Since the operations and matrix are the only members, and we aren't doing any sort of clever comparison (e.g. fp error or NaN insensitive), I think we could just default this operator. vector and array already generate equality operators, so I'm not sure why the existing code was looping to do this.

NickGerleman · 2024-02-26T22:20:35Z

packages/react-native/ReactCommon/react/renderer/graphics/Transform.cpp

-            lhsOp.x + (rhsOp.x - lhsOp.x) * animationProgress,
-            lhsOp.y + (rhsOp.y - lhsOp.y) * animationProgress,
-            lhsOp.z + (rhsOp.z - lhsOp.z) * animationProgress});
+            ValueUnit(lhsOp.x.value + (rhsOp.x.value - lhsOp.x.value) * animationProgress, UnitType::Point),


Could you explain why we use UnitType::Point unconditionally here?

My bad, I assumed it'll be point always. I should be checking whether lhs and rhs have same unit and use that. wdyt of below checks above the result and use one of the op's unit?

react_native_assert(lhsOp.x.unit == rhsOp.x.unit); react_native_assert(lhsOp.y.unit == rhsOp.y.unit); react_native_assert(lhsOp.z.unit == rhsOp.z.unit);

Added the above checks. I think lhs and rhs would have same units during interpolate

NickGerleman · 2024-02-26T22:27:26Z

packages/react-native/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp

+      finalView.layoutMetrics.frame.size.width,
+      finalView.layoutMetrics.frame.size.height


I double check that the spec defaults to border-box for reference box by default, so this should be good for now.

C++ 20 lets you default this operator (does memberwise-comparison) C++ 20 will automatically define != if you define == Co-authored-by: Nick Gerleman <nick@nickgerleman.com>

javache

Can we have some C++ unit tests for the behaviours added here?

I have some concerns with the design of ValueUnit, as it makes it a bit too easy to have gaps in the implementation

javache · 2024-03-04T12:50:07Z

packages/react-native/Libraries/StyleSheet/processTransform.js

+      invariant(
+        typeof value === 'number' ||
+          (typeof value === 'string' && value.endsWith('%')),
+        'Transform with key of "%s" must be number or a percentage. Passed value: %s.',
+        key,
+        stringifySafe(transformation),
+      );


All of this parsing does happen in native. validateTransform is just an additional DEV-time only validation of the input to get better signal to the developer.

javache · 2024-03-04T12:51:01Z

packages/react-native/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp

+    Float viewWidth, 
+    Float viewHeight) const {


Can you use Size? https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/renderer/graphics/Size.h

Suggested change

Float viewWidth,

Float viewHeight) const {

const Size& size) const {

javache · 2024-03-04T12:51:47Z

packages/react-native/ReactCommon/react/renderer/components/view/BaseViewProps.cpp

+  auto transformMatrix = Transform{};
+
+  if (viewWidth == 0 && viewHeight == 0){
+    return transformMatrix;


Different from previous behaviour which returned the plain transform. Not sure it matters.

True. Added this as an optimisation to avoid the operations loop as FromTransformOperation has become size dependent (we can also prevent calling this method when size is 0). I am also not sure transformation with zero dimensions matter (with border-box). It's an assumption though.

At some point I would like to explore moving transform matrix generation to shadow view and native view can implement a method, similar to how updateLayoutMetrics works.

javache · 2024-03-04T12:52:13Z

packages/react-native/ReactCommon/react/renderer/components/view/BaseViewProps.cpp

+
+  // transform is matrix
+  if (transform.operations.size() == 1 && transform.operations[0].type == TransformOperationType::Arbitrary) {
+    transformMatrix = transformMatrix * transform;


Avoid the multiplication

Suggested change

transformMatrix = transformMatrix * transform;

transformMatrix = transform;

javache · 2024-03-04T12:53:15Z

packages/react-native/ReactCommon/react/renderer/components/view/BaseViewProps.cpp

+  if (transformOrigin.isSet()) {
+    std::array<float, 3> translateOffsets =
+          getTranslateForTransformOrigin(viewWidth, viewHeight, transformOrigin);
+    transformMatrix = 
      Transform::Translate(
-          -translateOffsets[0], -translateOffsets[1], -translateOffsets[2]);
-  return newTransform;
+            translateOffsets[0], translateOffsets[1], translateOffsets[2])
+      * transformMatrix
+      * Transform::Translate(-translateOffsets[0], -translateOffsets[1], -translateOffsets[2]);
+  }


Why not do this before the transform processing loop? That way we can avoid less intermediate objects.

We can do translateOffsets[0], translateOffsets[1], translateOffsets[2]) before the loop but -translateOffsets[0], -translateOffsets[1], -translateOffsets[2] needs to be done post processing for transform origin to work correctly. I am missing how it can lead to less intermediate objects. Let me know, happy to fix it! Also, we want to prevent additional transformOrigin.isSet() checks.

javache · 2024-03-04T12:54:41Z

packages/react-native/ReactCommon/react/renderer/graphics/Transform.cpp

  }
  if (transformOperation.type == TransformOperationType::Scale) {
    return Transform::Scale(
-        transformOperation.x, transformOperation.y, transformOperation.z);
+        transformOperation.x.value, transformOperation.y.value, transformOperation.z.value);


This is quite a type-unsafe API now, as it's very easy to "forget" to check whether it's a percentage of a point value. Yoga used to require you to call something like resolveValue, can we make something explicit?

javache · 2024-03-04T12:55:50Z

packages/react-native/ReactCommon/react/renderer/graphics/ValueUnit.cpp

+ValueUnit ValueUnit::getValueUnitFromRawValue(const RawValue& value) {
+  if (value.hasType<Float>()) {
+    auto number = (float)value;
+    return ValueUnit(number, UnitType::Point);
+  } else if (value.hasType<std::string>()) {
+    const auto stringValue = (std::string)value;
+    if (stringValue.back() == '%') {
+      auto tryValue = folly::tryTo<float>(
+          std::string_view(stringValue).substr(0, stringValue.length() - 1));
+      if (tryValue.hasValue()) {
+        return ValueUnit(tryValue.value(), UnitType::Percent);
+      }
+    }
+  }
+  return ValueUnit();
+};


I don't think we'd want to use these here though @NickGerleman, since the code is fully layout independent? Or are you happy for these CSS utils to be exposed for non-layout purposes?

NickGerleman · 2024-03-25T12:40:52Z

@javache @intergalacticspacehighway anything pending here?

intergalacticspacehighway · 2024-03-26T01:46:30Z

@NickGerleman i got occupied with stuff at work 😅. @javache has commented in other PRs too. I am gonna wrap it up this week.

NickGerleman · 2024-03-27T20:16:37Z

No worries! I will take a look again after next revision.

analysis-bot · 2024-03-30T11:41:58Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	19,427,379	+16,635
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	22,800,763	+16,493
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 03a51da
Branch: main

Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

intergalacticspacehighway · 2024-04-15T01:40:57Z

@NickGerleman @javache Any pointers or command how can i run these transform tests?

I'll add some tests here for translate percentage.

NickGerleman · 2024-04-15T19:12:01Z

@NickGerleman @javache Any pointers or command how can i run these transform tests?

I'll add some tests here for translate percentage.

I think @cortinico wired this up to Android CMake build. I think this is called by Gradle inc in OSS CI, but I haven't run myself.

react-native/packages/react-native/ReactAndroid/src/main/jni/CMakeLists.txt

Line 141 in 6dd83cb

add_executable(reactnative_unittest

intergalacticspacehighway · 2024-04-21T18:41:29Z

@NickGerleman @javache added a testcase. CI is fine, so hopefully the test is running? Seems to be ready to take another look at this and the other PRs 🙏

cortinico · 2024-04-22T09:49:43Z

I think @cortinico wired this up to Android CMake build. I think this is called by Gradle inc in OSS CI, but I haven't run myself.

@intergalacticspacehighway Sadly those tests don't run automatically in OSS as we don't have a running device on CI. They will execute when we import the PR internally though so it's good that you added one 👍

intergalacticspacehighway · 2024-04-22T17:45:40Z

Thanks @cortinico Any way i can run these tests locally, just to confirm? i can see it bundles the cpp test files in android local build. But i couldn't find a way to run them. Is it like a separate project/gradle-task?

cortinico · 2024-04-23T11:14:07Z

Thanks @cortinico Any way i can run these tests locally, just to confirm? i can see it bundles the cpp test files in android local build. But i couldn't find a way to run them. Is it like a separate project/gradle-task?

Instructions on how to run them are here:
https://github.com/ShiraOzeri/Android-NDK-with-Google-Test#android-ndk-with-google-test
Just note that we don't run those in OSS so they might just be broken, but once we import it internally we can tell if they're working or not

NickGerleman · 2024-05-01T00:39:28Z

I'll import this and see how it looks

facebook-github-bot · 2024-05-01T00:40:05Z

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

intergalacticspacehighway · 2024-05-02T04:15:21Z

Something seems off. FB internal tests are running indefinitely 😅

NickGerleman · 2024-05-02T18:21:54Z

Something seems off. FB internal tests are running indefinitely 😅

Needs some build fixes for Buck. Going to see if I can take a look at this today

NickGerleman · 2024-05-02T19:05:11Z

Tests pass!

Small changes I needed to make:

Formatting
A shadowing warning the OSS build didn't complain as loudly about
Remove getValueUnitFromRawValue() in favor of fromRawValue overload we added recently. For dependency organization purposes, ValueUnit should not depend on RawValue

NickGerleman

This was causing a test outside RN to fail, but I think I narrowed it down to an unrelated bug #44429

NickGerleman · 2024-05-06T23:04:01Z

packages/react-native/ReactCommon/react/renderer/components/view/conversions.h

@@ -513,48 +515,52 @@ inline void fromRawValue(
        transformMatrix.matrix[i++] = number;
      }
      transformMatrix.operations.push_back(
-          TransformOperation{TransformOperationType::Arbitrary, 0, 0, 0});
+          TransformOperation{TransformOperationType::Arbitrary, Zero, Zero, Zero});
    } else if (operation == "perspective") {


I missed on my initial look-through that we are using ValueUnit (Point, or Percentage) for operations which take radians, which is a little confusing.

Don't want to make this PR too big, but we would replace ValueUnit will CSSValueVariant after the dust on that settles.

radians

You mean operations like this right? transformMatrix.operations.push_back( TransformOperation{TransformOperationType::Rotate, ValueUnit(toRadians(parameters, 0.0f), UnitType::Point), Zero, Zero})

Yeah, I considered UnitType::Point like a regular Float as ValueUnit eventually will get replaced and to keep the PR small but we can add Angle units in CSSValueVariant.

Replacing ValueUnit with CSSValueVariant

For sure, i can see it is in renderer/css, can we import and start using it? We can also replace ValueUnit in transformOrigin, i guess a separate PR to do that would be better.

facebook-github-bot · 2024-05-17T02:39:07Z

@NickGerleman merged this pull request in f997b81.

github-actions · 2024-05-17T02:39:08Z

This pull request was successfully merged by @intergalacticspacehighway in f997b81.

^{When will my fix make it into a release? | How to file a pick request?}

intergalacticspacehighway · 2024-05-20T03:39:23Z

cc - @NickGerleman Thanks for reviewing. fyi two more PRs are there - #43193 #43191 🙏

NickGerleman · 2024-05-20T10:39:26Z

cc - @NickGerleman Thanks for reviewing. fyi two more PRs are there - #43193 #43191 🙏

I will try to take a closer look at Android one soon. In general we have been recently wanting to avoid non-trivial Paper-specific changes (though for Android it is kinda shared), so the iOS one might make less sense.

Summary: This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews. ## Changelog: [Android] [ADDED] - Percentage support in translate <!-- 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: #43193 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - #43191, #43192 Reviewed By: joevilches Differential Revision: D57723216 Pulled By: NickGerleman fbshipit-source-id: c9da007678341b62745df858f043821bcc662a98

Summary: This PR adds percentage support in translate properties for new arch iOS. Isolating this PR for easier reviews. The approach taken here introduces usage of `ValueUnit` struct for transform operations so it can support `%` in translates and delay the generation of actual transform matrix until view dimensions are known. I have tried to keep the changes minimal and reuse existing APIs, open to changes if there's an alternative approach. ## Changelog: [IOS] [ADDED] - Percentage support in translate in new arch. <!-- 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#43192 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - facebook#43193, facebook#43191 Reviewed By: javache Differential Revision: D56802425 Pulled By: NickGerleman fbshipit-source-id: 978cbbdde004afe1e68ffee9a3c7eb7d16336b46

Summary: This PR adds percentage support in translate properties for android. Isolating this PR for easier reviews. ## Changelog: [Android] [ADDED] - Percentage support in translate <!-- 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#43193 Test Plan: - Checkout TransformExample.js -> Translate percentage example. - Added a simple test in `processTransform-test.js`. The regex is not perfect (values like 20px%, 20%px will pass, can be improved, let me know!) Related PRs - facebook#43191, facebook#43192 Reviewed By: joevilches Differential Revision: D57723216 Pulled By: NickGerleman fbshipit-source-id: c9da007678341b62745df858f043821bcc662a98

intergalacticspacehighway added 4 commits February 25, 2024 15:27

value unit in transform operations

14a4dde

translate with percentage

6829aa2

translate regex to support %

8879930

translate percentage test

50fd7fb

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 25, 2024

This was referenced Feb 25, 2024

feat(android): percentage support in translate #43193

Closed

feat(iOS/paper): percentage support in translate #43191

Open

NickGerleman reviewed Feb 26, 2024

View reviewed changes

default in equality operator for transform operation

411c84d

C++ 20 lets you default this operator (does memberwise-comparison) C++ 20 will automatically define != if you define == Co-authored-by: Nick Gerleman <nick@nickgerleman.com>

javache requested changes Mar 4, 2024

View reviewed changes

pr feedback: Size struct

5a6f86e

intergalacticspacehighway added 2 commits March 30, 2024 16:39

unit assert in interpolate

9ac7106

Merge branch 'main' into feat/new-arch-translate-percentage

98a597f

intergalacticspacehighway and others added 6 commits March 31, 2024 01:29

Update packages/react-native/Libraries/StyleSheet/processTransform.js

1ed7559

Co-authored-by: Pieter De Baets <pieter.debaets@gmail.com>

update transform when bounds change in updateLayoutMetrics

371a816

flow check fix

5e63c8a

prettier fix

b84a003

fix flow check

ea3b0af

resolve method in value unit struct

783056a

intergalacticspacehighway added 3 commits April 21, 2024 19:23

Merge branch 'main' into feat/new-arch-translate-percentage

3d9b569

test translate percentage

776d67d

translate y resolve height

e677dd7

NickGerleman mentioned this pull request Apr 22, 2024

Fix inverted flat list #44168

Open

NickGerleman reviewed May 6, 2024

View reviewed changes

facebook-github-bot closed this in f997b81 May 17, 2024

facebook-github-bot added the Merged This PR has been merged. label May 17, 2024

		finalView.layoutMetrics.frame.size.width,
		finalView.layoutMetrics.frame.size.height

	Float viewWidth,
	Float viewHeight) const {
	const Size& size) const {

	transformMatrix = transformMatrix * transform;
	transformMatrix = transform;

feat(iOS/fabric): percentage support in translate #43192

feat(iOS/fabric): percentage support in translate #43192

Conversation

intergalacticspacehighway commented Feb 25, 2024 • edited

Summary:

Changelog:

Test Plan:

NickGerleman left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intergalacticspacehighway Mar 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman commented Mar 25, 2024

intergalacticspacehighway commented Mar 26, 2024

NickGerleman commented Mar 27, 2024

analysis-bot commented Mar 30, 2024 • edited

intergalacticspacehighway commented Apr 15, 2024 • edited

NickGerleman commented Apr 15, 2024

intergalacticspacehighway commented Apr 21, 2024

cortinico commented Apr 22, 2024

intergalacticspacehighway commented Apr 22, 2024

cortinico commented Apr 23, 2024

NickGerleman commented May 1, 2024

facebook-github-bot commented May 1, 2024

intergalacticspacehighway commented May 2, 2024

NickGerleman commented May 2, 2024

NickGerleman commented May 2, 2024

NickGerleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 17, 2024

github-actions bot commented May 17, 2024

intergalacticspacehighway commented May 20, 2024

NickGerleman commented May 20, 2024

intergalacticspacehighway commented Feb 25, 2024 •

edited

NickGerleman left a comment •

edited

intergalacticspacehighway Mar 5, 2024 •

edited

analysis-bot commented Mar 30, 2024 •

edited

intergalacticspacehighway commented Apr 15, 2024 •

edited