[Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. #270

appleguy · 2017-05-14T03:10:01Z

[If this is merged, it should be in a disabled state - e.g. with the ASAvailability.h flag set to YOGA / 1, enabling the current behavior]

This approach allows us to avoid any ASDisplayNode.mm integration points.
However, it is not yet proven to be possible to achieve correctness with this approach.

The entry point (to start calculating), and the measurement function inputs, lack
the full expressiveness of ASSizeRange; we need to make sure that workarounds like
using style.minSize are successful in simulating the behavior of a full Yoga tree.

…l-tree integration. This approach allows us to avoid any ASDisplayNode.mm integration points. However, it is not yet proven to be possible to achieve correctness with this approach. The entry point (to start calculating), and the measurement function inputs, lack the full expressiveness of ASSizeRange; we need to make sure that workarounds like using style.minSize are successful in simulating the behavior of a full Yoga tree.

ghost · 2017-05-14T03:13:51Z

🚫 CI failed with log

nguyenhuy · 2017-05-16T10:36:46Z

~~@appleguy Very clean approach indeed. I think it's acceptable for a Yoga spec to ignore the min size since the internal Yoga algorithm doesn't understand this concept.~~

~~I might be missing something, but I imagine a full Yoga tree should behave correctly because every node in that tree drops the min constraint?~~

In case it's not a complete Yoga tree, meaning some nodes use Yoga while others don't, the nodes that use Yoga can still clamp their resulting size (returned by Yoga) against their min size to get a final size. This is essentially the same way we're doing backward compatibility for -calculateSizeThatFits:(CGSize)maxSize.

nguyenhuy · 2017-05-16T11:11:18Z

Ok I'm an ignorant :P YGStyle struct has minDimensions. Now I understand what you meant. Gonna comment on the style.minSize workarounds.

nguyenhuy

It seems to me that the style.mainSize workarounds are actually correct! I'm curious about cases that you encountered in which this integration doesn't work as expected.

I'll do another round of review before approving, whenever you feel like it's ready.

nguyenhuy · 2017-05-16T11:51:35Z

Source/Layout/ASYogaLayoutSpec.mm

+  // If the root node also has style.*Size set, these will be overridden below.
+  // YGNodeCalculateLayout currently doesn't offer the ability to pass a minimum size (max is passed there).
+  YGNodeStyleSetMinWidth (rootYogaNode, yogaFloatForCGFloat(rootConstrainedSize.min.width));
+  YGNodeStyleSetMinHeight(rootYogaNode, yogaFloatForCGFloat(rootConstrainedSize.min.height));


@nguyenhuy thanks for your comments here, it's very helpful to me! I just realized, what if this node has a min size already set on it - such as minWidth = 10% fraction? Do we have enough information here, maybe if we use the parentSize variant, to resolve that percentage and choose the more restrictive one?

hmm, this might be OK, because we perform these lines before calling calculateLayoutThatFits:. Still, interesting to think if we could make use of these extra values.
ASSizeRange styleAndParentSize = ASLayoutElementSizeResolve(self.style.size, parentSize);
const ASSizeRange resolvedRange = ASSizeRangeIntersect(constrainedSize, styleAndParentSize);

nguyenhuy · 2017-05-16T11:51:41Z

Source/Layout/ASYogaLayoutSpec.mm

+    YGNODE_STYLE_SET_DIMENSION(yogaNode, Height, style.height);
+
+    YGNODE_STYLE_SET_DIMENSION(yogaNode, MinWidth, style.minWidth);
+    YGNODE_STYLE_SET_DIMENSION(yogaNode, MinHeight, style.minHeight);


nguyenhuy · 2017-05-16T11:57:24Z

Source/Layout/ASYogaLayoutSpec.mm

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


Could you revisit this. It seems to be outdated because these min width and height aren't (and shouldn't be) overridden

nguyenhuy · 2017-05-16T12:02:07Z

Source/Layout/ASYogaUtilities.mm

+  if (heightMode == YGMeasureModeExactly) {
+    sizeRange.min.height = sizeRange.max.height;
+  }
+  ASDisplayNodeCAssert([layoutElement isKindOfClass:[ASDisplayNode class]], @"!");


This actually looks good to me! I briefly checked Yoga's source code and it looks like whatever width and height values given to this func were resolved against yogaNode->style.minDimensions (if needed). ~~Thus, we don't need to consider layoutElement.style.minSize here.~~

Thinking again, I believe we should do sizeRange.min.(width|height) = layoutElement.style.minSize.(width|height) when (width|height)Mode is either YGMeasureModeAtMost or YGMeasureModeUndefined.

The reason is that, even if width and height were resolved against their min values, in the case of these modes, a sub-element still needs a min constraint to measure against. And style.minSize will not be picked-up by the sub-element if it's not included in rootConstrainedSize.

nguyenhuy · 2017-05-16T12:06:35Z

Source/Layout/ASYogaLayoutSpec.h

+@property (nonatomic, strong, nullable) NSArray<ASDisplayNode *> *yogaChildren;
+@end
+
+#endif /* !YOGA_TREE_CONTIGUOUS */


It seems like this spec only accepts display nodes. Any particular reasons why it can't just work with generic ASLayoutElement instances? It would be nice if it can do that, and even has the same API with ASStackLayoutSpec, should we want to make it the default stack algorithm spec and to ensure the transition is seamless. Plus, we can leverage existing snapshot tests of ASStackLayoutSpec to test this Yoga spec.

ghost · 2017-05-20T03:54:53Z

🚫 CI failed with log

ghost · 2017-05-21T19:29:46Z

🚫 CI failed with log

appleguy · 2017-05-21T19:30:03Z

I made some updates here, although unfortunately there is still an issue that I can't figure out. I've spent so much time on it that I have to move on for a while, although my team's work is blocked until a path forward is figured out :(.

My understanding is that the Yoga import is not available in the unit test target. If we did have a way to run the ASStackLayoutSpec test suite against this, it would be a huge step.

appleguy · 2017-05-21T19:31:53Z

@garrettmoon FYI, it appears the CI failure is not related to this change:


❌  /Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Tests/ASBasicImageDownloaderContextTests.m:18:9: could not build module 'AsyncDisplayKit'

#import <AsyncDisplayKit/ASBasicImageDownloader.h>
^


▸ Compiling ASBasicImageDownloaderTests.m
2017-05-21 12:29:46.473 xcodebuild[67918:830691] Error Domain=IDETestOperationsObserverErrorDomain Code=3 "Test operation was canceled. If you believe this error represents a bug, please attach the log file at /Users/buildkite/Library/Developer/Xcode/DerivedData/AsyncDisplayKit-fmrnchqmcmesavemvqwpwrxctnim/Logs/Test/B3A12972-D7D1-4930-A891-890F15A3C558/Session-AsyncDisplayKitTests-2017-05-21_122945-VK9i0z.log" UserInfo={NSLocalizedDescription=Test operation was canceled. If you believe this error represents a bug, please attach the log file at /Users/buildkite/Library/Developer/Xcode/DerivedData/AsyncDisplayKit-fmrnchqmcmesavemvqwpwrxctnim/Logs/Test/B3A12972-D7D1-4930-A891-890F15A3C558/Session-AsyncDisplayKitTests-2017-05-21_122945-VK9i0z.log}
2017-05-21 12:29:46.474 xcodebuild[67918:830748] Connection peer refused channel request for "dtxproxy:XCTestManager_IDEInterface:XCTestManager_DaemonConnectionInterface"; channel canceled <DTXChannel: 0x7fc5cc340650>

Testing failed:
	Umbrella header for module 'AsyncDisplayKit' does not include header '/Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Source/AsyncDisplayKit+IGListKitMethods.h'
	Umbrella header for module 'AsyncDisplayKit' does not include header '/Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Source/IGListAdapter+AsyncDisplayKit.h'
	Could not build module 'AsyncDisplayKit'
** TEST FAILED **


The following build commands failed:
	CompileC /Users/buildkite/Library/Developer/Xcode/DerivedData/AsyncDisplayKit-fmrnchqmcmesavemvqwpwrxctnim/Build/Intermediates/AsyncDisplayKit.build/Debug-iphonesimulator/AsyncDisplayKitTests.build/Objects-normal/x86_64/ASBasicImageDownloaderContextTests.o Tests/ASBasicImageDownloaderContextTests.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

maicki · 2017-05-22T16:50:36Z

@appleguy Can you rebase your PR the fix for the CI error landed on master.

ghost · 2017-05-23T00:26:37Z

🚫 CI failed with log

ghost · 2017-05-26T06:41:35Z

🚫 CI failed with log

ghost · 2017-05-26T06:44:29Z

🚫 CI failed with log

ghost · 2017-05-26T06:47:58Z

🚫 CI failed with log

ghost · 2017-05-26T07:41:38Z

🚫 CI failed with log

appleguy · 2017-05-26T07:52:30Z

@garrettmoon this appears to be the only failure left...do you know if this is known or being seen on any other PRs?


ASCollectionViewTests
  testThatMultipleBatchFetchesDontHappenUnnecessarily, failed - Too many batch fetches!
  /Users/Shared/buildkite/builds/iosf-garrett-VMrmmvuqKsO4.dyn.pinadmin.com-1/pinterest/texture/Tests/ASCollectionViewTests.mm:891

if (batchFetchCount > 1) {
  XCTFail(@"Too many batch fetches!");
  return;

   Executed 446 tests, with 1 failure (0 unexpected) in 21.254 (21.682) seconds
Failing tests:
  -[ASCollectionViewTests testThatMultipleBatchFetchesDontHappenUnnecessarily]
** TEST FAILED **

appleguy · 2017-05-27T04:22:46Z

@nguyenhuy @maicki ready for review! It's building correctly on the CI, and I also fixed the remaining issue I had with a series of nested stacks not behaving the same as they did previously (at the moment there are no known issues in the computed layout geometry, although I'm expanding testing and hope to utilize some of the ASStackLayoutSpec tests eventually).

If you have any concrete ideas on how to integrate this with the Stack tests, let me know - this would be very valuable input.

A known problem is that this leaks the yoga nodes right now, although the memory footprint of those is quite small. I will clean this up before landing, but let's work through any questions you have first.

Adlai-Holler

@appleguy This is absolutely huge! Congratulations on getting this to completion man! Merge at will.

Adlai-Holler · 2017-05-27T20:37:21Z

Source/Layout/ASYogaLayoutSpec.mm

+{
+  ASLayoutElementStyle *style = element.style;
+
+  YGNodeSetContext(yogaNode, (__bridge void *)element);


Small issue, not worth blocking merge. Thinking about ownership of element.

Should this be something like (__bridge_retained void *)element then in the yoga context destructor (YGFree?), id val = (__bridge_transfer id)YGNodeGetContext(node)?

Again, this looks like a headache that we don't need to wrestle with unless we see crashes from deallocated elements in a particular version of the framework. But now it's written down 🙂 .

Review is very old, diff updated.

ghost · 2017-05-29T21:55:30Z

🚫 CI failed with log

…transfer, YGNodeFree)

ghost · 2017-05-29T22:15:47Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

…for Yoga. (TextureGroup#270) * [Yoga] Implement ASYogaLayoutSpec, an experimental alternative to full-tree integration. This approach allows us to avoid any ASDisplayNode.mm integration points. However, it is not yet proven to be possible to achieve correctness with this approach. The entry point (to start calculating), and the measurement function inputs, lack the full expressiveness of ASSizeRange; we need to make sure that workarounds like using style.minSize are successful in simulating the behavior of a full Yoga tree. * [Yoga] Fix file comments, move towards <ASLayoutElement> support. * [Yoga] Important fix for simplified, non-contiguous Yoga integration. * [Yoga] Complete implementation of manual memory management (__bridge_transfer, YGNodeFree)

appleguy self-assigned this May 14, 2017

appleguy requested review from maicki and Adlai-Holler May 14, 2017 03:10

nguyenhuy previously requested changes May 16, 2017

View reviewed changes

Merge branch 'master' into ASYogaLayoutSpec

4da8531

[Yoga] Fix file comments, move towards <ASLayoutElement> support.

3b8f305

Merge branch 'master' into ASYogaLayoutSpec

495f8b0

Merge branch 'master' into ASYogaLayoutSpec

7a9d29e

appleguy force-pushed the ASYogaLayoutSpec branch from 8dd924f to 7c82304 Compare May 26, 2017 06:45

appleguy changed the title ~~[WIP] [Yoga] Implement ASYogaLayoutSpec, an experimental alternative to full-tree integration.~~ [Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. May 26, 2017

[Yoga] Important fix for simplified, non-contiguous Yoga integration.

0892c27

appleguy force-pushed the ASYogaLayoutSpec branch from 7c82304 to 0892c27 Compare May 26, 2017 07:40

Adlai-Holler approved these changes May 27, 2017

View reviewed changes

Merge branch 'master' into ASYogaLayoutSpec

f2d473e

appleguy force-pushed the ASYogaLayoutSpec branch 2 times, most recently from 1d39845 to fdbfe52 Compare May 29, 2017 21:39

[Yoga] Complete implementation of manual memory management (__bridge_…

da055aa

…transfer, YGNodeFree)

appleguy force-pushed the ASYogaLayoutSpec branch from fdbfe52 to da055aa Compare May 29, 2017 22:15

appleguy merged commit b285ece into master May 29, 2017

appleguy deleted the ASYogaLayoutSpec branch May 29, 2017 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. #270

[Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. #270

appleguy commented May 14, 2017

ghost commented May 14, 2017

nguyenhuy commented May 16, 2017 •

edited

Loading

nguyenhuy commented May 16, 2017

nguyenhuy left a comment

nguyenhuy May 16, 2017

appleguy May 21, 2017 •

edited

Loading

nguyenhuy May 16, 2017

nguyenhuy May 16, 2017

nguyenhuy May 16, 2017 •

edited

Loading

nguyenhuy May 16, 2017

nguyenhuy May 16, 2017 •

edited

Loading

ghost commented May 20, 2017

ghost commented May 21, 2017

appleguy commented May 21, 2017

appleguy commented May 21, 2017

maicki commented May 22, 2017

ghost commented May 23, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

appleguy commented May 26, 2017

appleguy commented May 27, 2017 •

edited

Loading

Adlai-Holler left a comment

Adlai-Holler May 27, 2017

ghost commented May 29, 2017

ghost commented May 29, 2017

[Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. #270

[Yoga] Implement ASYogaLayoutSpec, a simplified integration strategy for Yoga. #270

Conversation

appleguy commented May 14, 2017

ghost commented May 14, 2017

nguyenhuy commented May 16, 2017 • edited Loading

nguyenhuy commented May 16, 2017

nguyenhuy left a comment

Choose a reason for hiding this comment

nguyenhuy May 16, 2017

Choose a reason for hiding this comment

appleguy May 21, 2017 • edited Loading

Choose a reason for hiding this comment

nguyenhuy May 16, 2017

Choose a reason for hiding this comment

nguyenhuy May 16, 2017

Choose a reason for hiding this comment

nguyenhuy May 16, 2017 • edited Loading

Choose a reason for hiding this comment

nguyenhuy May 16, 2017

Choose a reason for hiding this comment

nguyenhuy May 16, 2017 • edited Loading

Choose a reason for hiding this comment

ghost commented May 20, 2017

ghost commented May 21, 2017

appleguy commented May 21, 2017

appleguy commented May 21, 2017

maicki commented May 22, 2017

ghost commented May 23, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

ghost commented May 26, 2017

appleguy commented May 26, 2017

appleguy commented May 27, 2017 • edited Loading

Adlai-Holler left a comment

Choose a reason for hiding this comment

Adlai-Holler May 27, 2017

Choose a reason for hiding this comment

ghost commented May 29, 2017

ghost commented May 29, 2017

nguyenhuy commented May 16, 2017 •

edited

Loading

appleguy May 21, 2017 •

edited

Loading

nguyenhuy May 16, 2017 •

edited

Loading

nguyenhuy May 16, 2017 •

edited

Loading

appleguy commented May 27, 2017 •

edited

Loading