[Yoga] Initial commit for supporting Yoga-powered layout calculation. by appleguy · Pull Request #2982 · facebookarchive/AsyncDisplayKit

appleguy · 2017-02-03T23:53:24Z

Because this results in ASLayout objects, it preserve support for automaticallyManagesSubnodes as well as the animated transition system. More work remains to vet performance of the new mode,
and it will remain in +Beta for the forseeable future.

I'm not sure that this should ever be used as the primary ASDK layout system, but it should remain an option for some apps to experiment with if they require an implementation that more strictly mirrors W3C standard Flexbox.

…ayout calculation. Because this results in ASLayout objects, it preserve support for automaticallyManagesSubnodes as well as the animated transition system. More work remains to vet performance of the new mode, and it will remain in +Beta for the forseeable future. I'm not sure that this should ever be used as the primary ASDK layout system, but it should remain an option for some apps to experiment with if they require an implementation that more strictly mirrors W3C standard Flexbox.

appleguy · 2017-02-03T23:55:44Z

 @property (nonatomic, assign, readwrite) ASDimension maxWidth;

+#if YOGA
+@property (nonatomic, assign, readwrite) ASStackLayoutDirection direction;


@maicki I decided to use compile-gated extensions in the base style object for a few reasons:

They are compiled out by default

Apps that use them will find them to be very performance-sensitive due to very widespread use

They aren't particularly feature-specific, but rather standard expressions of CSS Flexbox

The Extensibility PR had not landed by the time I started this, and isn't merging (also see my comment about ASEnvironment).

All that said, I would like to find a path to landing this diff soon that is "acceptable" and then I can actively improve it from there, so that the time-consuming merge conflicts we've all been dealing with are reduced. For example, we could create a new style object like .yogaStyle, or a category .h / .mm for these new properties.

garrettmoon · 2017-02-03T23:58:49Z

Is there any way we can separate this out so that it can be cleanly included / excluded? I.e. all separate files with non-yoga specific changes in the core classes which would support a generally pluggable system?

appleguy · 2017-02-04T00:00:30Z

@garrettmoon unless I missed one, every single change is gated by just one compiler flag, which is off by default (#if YOGA).

Because of the high rate of change recently, I think it would be better to split out into separate files when ASDisplayNode is broken up (IIRC this is on the docket for very soon?).

For an example of this pain, recently there was a ~3,100 line change to ASDisplayNode that took almost a day for me to fix past :(.

appleguy · 2017-02-04T00:02:54Z

OK, I think this is technically merge-conflict-free for the moment. However we should definitely strategize how to better separate this out.

If we don't want to land it until then, I'm open to suggestions — let's just agree on how to separate it and try to coordinate on changing these files too much until that point?

garrettmoon · 2017-02-04T00:06:44Z

@appleguy yeah, hopefully we have fewer changes like that in the future; they're specifically attempting to prevent it :D

Sounds good, we can try and hold off on landing big changes until we've made some decisions around this one.

appleguy · 2017-02-04T02:03:46Z

@garrettmoon - awesome, thanks! Since we're continuing to change this, I want to try to stay in sync as much as possible (even with this PR there's a risk of internal divergence). The best way to do that would be to land in the next few days, so I'm willing to put effort in over the weekend to gate it to whatever degree is needed to be a good citizen when landing something experimental like this.

Any thoughts on how to compartmentalize it further than the compile-out switch? Can I use the +Beta.h headers, and then you'd prefer to see +Yoga.mm implementation files -- or did you have a different setup in mind?

…layouts can't happen.

garrettmoon · 2017-02-06T21:18:49Z

My impression is that you, @Adlai-Holler and @maicki came up with a direction in your meeting, will defer to them :)

appleguy · 2017-02-09T03:43:04Z


 #if ASEVENTLOG_ENABLE
-#define ASDisplayNodeLogEvent(node, ...) [node.eventLog logEventWithBacktrace:(AS_SAVE_EVENT_BACKTRACES ? [NSThread callStackSymbols] : nil) format:__VA_ARGS__]
+  #define ASDisplayNodeLogEvent(node, ...) [node.eventLog logEventWithBacktrace:(AS_SAVE_EVENT_BACKTRACES ? [NSThread callStackSymbols] : nil) format:__VA_ARGS__]


I have seen this indentation style used in a few places in ASDK, and I think it increases readability quite a bit. So only in the files I touched, I made sure the #defines matched.

appleguy · 2017-02-09T03:45:03Z

+
+@interface ASLayoutElementStyle (Yoga)
+
+@property (nonatomic, assign, readwrite) ASStackLayoutDirection direction;


@maicki I decided to put these on the style object directly for a few reasons:

They are able to use std::atomic, which is great for perf.

This is the only way to achieve thread safety (extensibility API is new, and doesn't yet have a solution for locking)

They can be nicely hidden in +Beta

The impact in the .mm is very small

I'm believe these changes won't add any work during refactoring of the layout system, as they are tucked into the style object — without business logic integrations.

appleguy · 2017-02-09T03:45:24Z

 /**
-  * Declare <ASInterfaceState> methods as requiring super calls (this can't be required in the protocol).
-  * For descriptions, see <ASInterfaceState> definition.
+  * Declare <ASInterfaceStateDelegate> methods as requiring super calls (this can't be required in the protocol).


This was a spot that hadn't been renamed after there was a request to put Delegate in the name.

appleguy · 2017-02-09T03:46:17Z

+
+- (ASDisplayNode *)yogaParent
+{
+  return _yogaParent;


I realize these methods don't currently lock. This is something I have in mind and will follow up on. For now, there is a risk of deadlocking if I add locks, and so I would like to approach this with a careful analysis of the correct locking before dropping them in.

appleguy · 2017-02-09T03:47:55Z

+
+  // Apply the constrainedSize as a base, known frame of reference.
+  // If the root node also has style.*Size set, these will be overridden below.
+  YGNodeStyleSetMinWidth (rootYogaNode, yogaFloatForCGFloat(rootConstrainedSize.min.width));


I have planned changes to how this copying of style information should work. However, I'd like to get the initial version in before changing this much more. The reason is that I can't test against a large Yoga layout without doing a repository sync, and syncing is tough right now because of divergence from master. In any case, consider it my responsibility to make sure this logic becomes cleaner, even though it is hidden in the +Yoga file.

appleguy · 2017-02-09T03:49:59Z

  _subnodes = nil;

+#if YOGA
+  if (_yogaNode != NULL) {


For the three changes in ASDisplayNode.mm, I couldn't figure out a reasonable way to move them out. However, I thought pretty carefully about each of these, and don't think they will create any complexity problems during refactoring.

appleguy · 2017-02-09T03:50:17Z

                 relativeToParentSize:(CGSize)parentSize
 {
-  const ASSizeRange resolvedRange = ASSizeRangeIntersect(constrainedSize, ASLayoutElementSizeResolve(self.style.size, parentSize));
+  ASSizeRange styleAndParentSize = ASLayoutElementSizeResolve(self.style.size, parentSize);


This is here because I found it is very useful during debugging to have this intermediate variable.

appleguy · 2017-02-09T03:51:39Z

  ASDN::MutexLocker l(__instanceLock__);

+#if YOGA /* YOGA */
+  if (ASHierarchyStateIncludesYogaLayoutEnabled(_hierarchyState) == YES &&


This integration in the layout method is just an early-bail condition that has no other ties into the behavior of the method. So, total refactoring of the rest of the method should be OK.

If any issues are encountered when refactoring, I am very supportive of deleting this integration entirely, and just ask me to figure out how to re-add it with the new code. The intention is not to add more officially-supported paths.

appleguy · 2017-02-09T03:52:38Z

+  ASDimension left;
+  ASDimension bottom;
+  ASDimension right;
+} ASEdgeInsets;


I think this type could be generally useful for many things, so proposing we keep it in ASDimension. I can move it if needed.

appleguy · 2017-02-09T03:54:05Z

+
+#if YOGA
+
+- (ASStackLayoutDirection)direction           { return _direction.load(); }


These and especially the ivars had to be in the main class instead of a category. I think the footprint is minimized enough and not linked into the other code that it should be OK. This integration is very beneficial due to the std::atomic performance in very key code (these values are read much more frequently even than the ASLayoutSpec style properties, because of how the YGNodeRef style copying works currently).

appleguy · 2017-02-09T03:54:36Z

      Any layout calculated during this state should not be applied immediately, but pending until later. */
  ASHierarchyStateLayoutPending           = 1 << 3,
-  ASHierarchyStateVisualizeLayout         = 1 << 4
+  ASHierarchyStateYogaLayoutEnabled       = 1 << 4,


Likewise, I could #define these values and "Illegally" OR them into the state value. However since it doesn't cause tangling, it seemed best to add these directly.

…ng code. Created new file ASDisplayNode+Yoga.mm, reduced size and number of integration points in core code.

…llocating _yogaNode in property accessor, and changing all accesses to use the property.

Adlai-Holler

I've given this a complete review and I'm glad to approve it. I'm excited to see this up and running sometime. Thanks for taking the time to minimize impact while this bold project gets developed. High hopes!

appleguy added the Layout label Feb 3, 2017

appleguy self-assigned this Feb 3, 2017

appleguy requested review from Adlai-Holler, garrettmoon, maicki and nguyenhuy February 3, 2017 23:53

facebook-github-bot added the CLA Signed label Feb 3, 2017

appleguy commented Feb 3, 2017

View reviewed changes

Merge branch 'master' into YogaUpstream

8f27889

Merge branch 'master' into YogaUpstream

c47a173

appleguy force-pushed the YogaUpstream branch from 31b244e to c47a173 Compare February 5, 2017 00:46

appleguy added 4 commits February 4, 2017 17:15

Merge branch 'master' into YogaUpstream

2dd0677

Merge branch 'master' into YogaUpstream

eccae80

Merge branch 'master' into YogaUpstream

9015fe6

[Yoga] Improve usage of ASHierarchyState to ensure simultaneous yoga …

2d90595

…layouts can't happen.

Merge branch 'master' into YogaUpstream

8587be0

appleguy commented Feb 9, 2017

View reviewed changes

[Yoga] Strictly minimize the impact of the Yoga integration on existi…

a7daf50

…ng code. Created new file ASDisplayNode+Yoga.mm, reduced size and number of integration points in core code.

appleguy force-pushed the YogaUpstream branch from 25c39f1 to a7daf50 Compare February 9, 2017 03:55

appleguy added 2 commits February 8, 2017 20:31

Merge branch 'master' into YogaUpstream

0954e21

[Yoga] Figured out how to further reduce ASDisplayNode.mm impact by a…

9abe420

…llocating _yogaNode in property accessor, and changing all accesses to use the property.

Adlai-Holler approved these changes Feb 10, 2017

View reviewed changes

Adlai-Holler merged commit f912657 into master Feb 10, 2017

Adlai-Holler deleted the YogaUpstream branch February 10, 2017 00:10


		@interface ASLayoutElementStyle (Yoga)

		@property (nonatomic, assign, readwrite) ASStackLayoutDirection direction;


		#if YOGA

		- (ASStackLayoutDirection)direction { return _direction.load(); }

Conversation

appleguy commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garrettmoon commented Feb 3, 2017

Uh oh!

appleguy commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

appleguy commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garrettmoon commented Feb 4, 2017

Uh oh!

appleguy commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garrettmoon commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

appleguy Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

appleguy Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

appleguy commented Feb 3, 2017 •

edited

Loading

appleguy commented Feb 4, 2017 •

edited

Loading

appleguy commented Feb 4, 2017 •

edited

Loading

appleguy commented Feb 4, 2017 •

edited

Loading

appleguy Feb 9, 2017 •

edited

Loading

appleguy Feb 9, 2017 •

edited

Loading