Skip to content

Commit

Permalink
New round-to-pixel-grid algorithm that fixes possible subpixel gaps b…
Browse files Browse the repository at this point in the history
…etween sibling nodes

Summary:
This diff introduces new, little bit sophisticated round-to-pixel-grid algorithm.

**Motivation:**

Previous simple and straightforward solution works in most cases but sometimes produce the not-so-great result. A while ago Nick Lockwood described this problem and proposed the solution in RN's RCTShadowView class:

For example, say you have the following structure:

  // +--------+---------+--------+
  // |        |+-------+|        |
  // |        ||       ||        |
  // |        |+-------+|        |
  // +--------+---------+--------+

Say the screen width is 320 pts so the three big views will get the following x bounds from our layout system:
{0, 106.667}, {106.667, 213.333}, {213.333, 320}
Assuming screen scale is 2, these numbers must be rounded to the nearest 0.5 to fit the pixel grid:
{0, 106.5}, {106.5, 213.5}, {213.5, 320}
You'll notice that the three widths are 106.5, 107, 106.5.

This is great for the parent views but it gets trickier when we consider rounding for the subview. When we go to round the bounds for the subview in the middle, it's relative bounds are {0, 106.667} which gets rounded to {0, 106.5}. This will cause the subview to be one pixel smaller than it should be. This is why we need to pass in the absolute position in order to do the rounding relative to the screen's grid rather than the view's grid. After passing in the absolutePosition of {106.667, y}, we do the following calculations:
absoluteLeft = round(absolutePosition.x + viewPosition.left) = round(106.667 + 0) = 106.5
absoluteRight = round(absolutePosition.x + viewPosition.left + viewSize.width) + round(106.667 + 0 + 106.667) = 213.5
width = 213.5 - 106.5 = 107

You'll notice that this is the same width we calculated for the parent view because we've taken its position into account.

I believe this is awesome. I also believe that we have to decouple this logic from RN and put it into awesome Yoga. So I did it in this diff.

**Fun fact:**
The original implementation of this algorithm in RN had (and still have) a bug, which was found by Dustin dshahidehpour and fixed in D4133643. Therefore that diff was unlanded because it broke something unrelated inside RN text engine. I will fix that problem in RN later.

**Why do we need to change test methodology?**
Because the way we receive layout metrics from Chrome browser actually directly related to rounding problem. Previously we used `offsetHeight` and `offsetWidth` properties of the DOM node, which contain naively rounded values from `computedStyle` or `getBoundingClientRect`. (Which is we are trying to fix!) So, I added the new function that computes node size using two-step-rounding approach, conceptually similar to one that implemented in Yoga. Note: Chrome browser performs rounding layout as part of rendering process and actual values that can ve computed by counting actual pixel are different from these natively rounded ones.

**Why do some tests now have different desired values?**
These changes actually prove that my approach is correct and more useful for actual view rendering goals. So, let's take a look at test with changed values `rounding_fractial_input_3`:
Previously: 64+25+24=114 (Incorrect!)
Now: 65+24+25=114 (Correct!)
Previously: 64+25+24=114 (Incorrect!)
Now: 65+24+25=114 (Correct!)

Reviewed By: emilsjolander

Differential Revision: D4941266

fbshipit-source-id: 07500f5cc93c628219500e9e07291438e9d5d36c
  • Loading branch information
shergin authored and facebook-github-bot committed Apr 26, 2017
1 parent f2b5d0f commit aa5b296
Show file tree
Hide file tree
Showing 7 changed files with 1,323 additions and 60 deletions.
315 changes: 309 additions & 6 deletions csharp/tests/Facebook.Yoga/YGRoundingTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -684,17 +684,17 @@ public void Test_rounding_fractial_input_3()
Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(100f, root_child0.LayoutWidth);
Assert.AreEqual(64f, root_child0.LayoutHeight);
Assert.AreEqual(65f, root_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1.LayoutX);
Assert.AreEqual(64f, root_child1.LayoutY);
Assert.AreEqual(100f, root_child1.LayoutWidth);
Assert.AreEqual(25f, root_child1.LayoutHeight);
Assert.AreEqual(24f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(89f, root_child2.LayoutY);
Assert.AreEqual(100f, root_child2.LayoutWidth);
Assert.AreEqual(24f, root_child2.LayoutHeight);
Assert.AreEqual(25f, root_child2.LayoutHeight);

root.StyleDirection = YogaDirection.RTL;
root.CalculateLayout();
Expand All @@ -707,17 +707,17 @@ public void Test_rounding_fractial_input_3()
Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(100f, root_child0.LayoutWidth);
Assert.AreEqual(64f, root_child0.LayoutHeight);
Assert.AreEqual(65f, root_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1.LayoutX);
Assert.AreEqual(64f, root_child1.LayoutY);
Assert.AreEqual(100f, root_child1.LayoutWidth);
Assert.AreEqual(25f, root_child1.LayoutHeight);
Assert.AreEqual(24f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(89f, root_child2.LayoutY);
Assert.AreEqual(100f, root_child2.LayoutWidth);
Assert.AreEqual(24f, root_child2.LayoutHeight);
Assert.AreEqual(25f, root_child2.LayoutHeight);
}

[Test]
Expand Down Expand Up @@ -793,5 +793,308 @@ public void Test_rounding_fractial_input_4()
Assert.AreEqual(24f, root_child2.LayoutHeight);
}

[Test]
public void Test_rounding_inner_node_controversy_horizontal()
{
YogaConfig config = new YogaConfig();
config.SetExperimentalFeatureEnabled(YogaExperimentalFeature.Rounding, true);

YogaNode root = new YogaNode(config);
root.FlexDirection = YogaFlexDirection.Row;
root.Width = 320;

YogaNode root_child0 = new YogaNode(config);
root_child0.FlexGrow = 1;
root_child0.Height = 10;
root.Insert(0, root_child0);

YogaNode root_child1 = new YogaNode(config);
root_child1.FlexGrow = 1;
root_child1.Height = 10;
root.Insert(1, root_child1);

YogaNode root_child1_child0 = new YogaNode(config);
root_child1_child0.FlexGrow = 1;
root_child1_child0.Height = 10;
root_child1.Insert(0, root_child1_child0);

YogaNode root_child2 = new YogaNode(config);
root_child2.FlexGrow = 1;
root_child2.Height = 10;
root.Insert(2, root_child2);
root.StyleDirection = YogaDirection.LTR;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(320f, root.LayoutWidth);
Assert.AreEqual(10f, root.LayoutHeight);

Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(107f, root_child0.LayoutWidth);
Assert.AreEqual(10f, root_child0.LayoutHeight);

Assert.AreEqual(107f, root_child1.LayoutX);
Assert.AreEqual(0f, root_child1.LayoutY);
Assert.AreEqual(106f, root_child1.LayoutWidth);
Assert.AreEqual(10f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(106f, root_child1_child0.LayoutWidth);
Assert.AreEqual(10f, root_child1_child0.LayoutHeight);

Assert.AreEqual(213f, root_child2.LayoutX);
Assert.AreEqual(0f, root_child2.LayoutY);
Assert.AreEqual(107f, root_child2.LayoutWidth);
Assert.AreEqual(10f, root_child2.LayoutHeight);

root.StyleDirection = YogaDirection.RTL;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(320f, root.LayoutWidth);
Assert.AreEqual(10f, root.LayoutHeight);

Assert.AreEqual(213f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(107f, root_child0.LayoutWidth);
Assert.AreEqual(10f, root_child0.LayoutHeight);

Assert.AreEqual(107f, root_child1.LayoutX);
Assert.AreEqual(0f, root_child1.LayoutY);
Assert.AreEqual(106f, root_child1.LayoutWidth);
Assert.AreEqual(10f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(106f, root_child1_child0.LayoutWidth);
Assert.AreEqual(10f, root_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(0f, root_child2.LayoutY);
Assert.AreEqual(107f, root_child2.LayoutWidth);
Assert.AreEqual(10f, root_child2.LayoutHeight);
}

[Test]
public void Test_rounding_inner_node_controversy_vertical()
{
YogaConfig config = new YogaConfig();
config.SetExperimentalFeatureEnabled(YogaExperimentalFeature.Rounding, true);

YogaNode root = new YogaNode(config);
root.Height = 320;

YogaNode root_child0 = new YogaNode(config);
root_child0.FlexGrow = 1;
root_child0.Width = 10;
root.Insert(0, root_child0);

YogaNode root_child1 = new YogaNode(config);
root_child1.FlexGrow = 1;
root_child1.Width = 10;
root.Insert(1, root_child1);

YogaNode root_child1_child0 = new YogaNode(config);
root_child1_child0.FlexGrow = 1;
root_child1_child0.Width = 10;
root_child1.Insert(0, root_child1_child0);

YogaNode root_child2 = new YogaNode(config);
root_child2.FlexGrow = 1;
root_child2.Width = 10;
root.Insert(2, root_child2);
root.StyleDirection = YogaDirection.LTR;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(10f, root.LayoutWidth);
Assert.AreEqual(320f, root.LayoutHeight);

Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(10f, root_child0.LayoutWidth);
Assert.AreEqual(107f, root_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1.LayoutX);
Assert.AreEqual(107f, root_child1.LayoutY);
Assert.AreEqual(10f, root_child1.LayoutWidth);
Assert.AreEqual(106f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(10f, root_child1_child0.LayoutWidth);
Assert.AreEqual(106f, root_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(213f, root_child2.LayoutY);
Assert.AreEqual(10f, root_child2.LayoutWidth);
Assert.AreEqual(107f, root_child2.LayoutHeight);

root.StyleDirection = YogaDirection.RTL;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(10f, root.LayoutWidth);
Assert.AreEqual(320f, root.LayoutHeight);

Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(10f, root_child0.LayoutWidth);
Assert.AreEqual(107f, root_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1.LayoutX);
Assert.AreEqual(107f, root_child1.LayoutY);
Assert.AreEqual(10f, root_child1.LayoutWidth);
Assert.AreEqual(106f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(10f, root_child1_child0.LayoutWidth);
Assert.AreEqual(106f, root_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(213f, root_child2.LayoutY);
Assert.AreEqual(10f, root_child2.LayoutWidth);
Assert.AreEqual(107f, root_child2.LayoutHeight);
}

[Test]
public void Test_rounding_inner_node_controversy_combined()
{
YogaConfig config = new YogaConfig();
config.SetExperimentalFeatureEnabled(YogaExperimentalFeature.Rounding, true);

YogaNode root = new YogaNode(config);
root.FlexDirection = YogaFlexDirection.Row;
root.Width = 640;
root.Height = 320;

YogaNode root_child0 = new YogaNode(config);
root_child0.FlexGrow = 1;
root_child0.Height = 100.Percent();
root.Insert(0, root_child0);

YogaNode root_child1 = new YogaNode(config);
root_child1.FlexGrow = 1;
root_child1.Height = 100.Percent();
root.Insert(1, root_child1);

YogaNode root_child1_child0 = new YogaNode(config);
root_child1_child0.FlexGrow = 1;
root_child1_child0.Width = 100.Percent();
root_child1.Insert(0, root_child1_child0);

YogaNode root_child1_child1 = new YogaNode(config);
root_child1_child1.FlexGrow = 1;
root_child1_child1.Width = 100.Percent();
root_child1.Insert(1, root_child1_child1);

YogaNode root_child1_child1_child0 = new YogaNode(config);
root_child1_child1_child0.FlexGrow = 1;
root_child1_child1_child0.Width = 100.Percent();
root_child1_child1.Insert(0, root_child1_child1_child0);

YogaNode root_child1_child2 = new YogaNode(config);
root_child1_child2.FlexGrow = 1;
root_child1_child2.Width = 100.Percent();
root_child1.Insert(2, root_child1_child2);

YogaNode root_child2 = new YogaNode(config);
root_child2.FlexGrow = 1;
root_child2.Height = 100.Percent();
root.Insert(2, root_child2);
root.StyleDirection = YogaDirection.LTR;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(640f, root.LayoutWidth);
Assert.AreEqual(320f, root.LayoutHeight);

Assert.AreEqual(0f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(213f, root_child0.LayoutWidth);
Assert.AreEqual(320f, root_child0.LayoutHeight);

Assert.AreEqual(213f, root_child1.LayoutX);
Assert.AreEqual(0f, root_child1.LayoutY);
Assert.AreEqual(214f, root_child1.LayoutWidth);
Assert.AreEqual(320f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(214f, root_child1_child0.LayoutWidth);
Assert.AreEqual(107f, root_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1_child1.LayoutX);
Assert.AreEqual(107f, root_child1_child1.LayoutY);
Assert.AreEqual(214f, root_child1_child1.LayoutWidth);
Assert.AreEqual(106f, root_child1_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child1_child0.LayoutY);
Assert.AreEqual(214f, root_child1_child1_child0.LayoutWidth);
Assert.AreEqual(106f, root_child1_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1_child2.LayoutX);
Assert.AreEqual(213f, root_child1_child2.LayoutY);
Assert.AreEqual(214f, root_child1_child2.LayoutWidth);
Assert.AreEqual(107f, root_child1_child2.LayoutHeight);

Assert.AreEqual(427f, root_child2.LayoutX);
Assert.AreEqual(0f, root_child2.LayoutY);
Assert.AreEqual(213f, root_child2.LayoutWidth);
Assert.AreEqual(320f, root_child2.LayoutHeight);

root.StyleDirection = YogaDirection.RTL;
root.CalculateLayout();

Assert.AreEqual(0f, root.LayoutX);
Assert.AreEqual(0f, root.LayoutY);
Assert.AreEqual(640f, root.LayoutWidth);
Assert.AreEqual(320f, root.LayoutHeight);

Assert.AreEqual(427f, root_child0.LayoutX);
Assert.AreEqual(0f, root_child0.LayoutY);
Assert.AreEqual(213f, root_child0.LayoutWidth);
Assert.AreEqual(320f, root_child0.LayoutHeight);

Assert.AreEqual(213f, root_child1.LayoutX);
Assert.AreEqual(0f, root_child1.LayoutY);
Assert.AreEqual(214f, root_child1.LayoutWidth);
Assert.AreEqual(320f, root_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child0.LayoutY);
Assert.AreEqual(214f, root_child1_child0.LayoutWidth);
Assert.AreEqual(107f, root_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1_child1.LayoutX);
Assert.AreEqual(107f, root_child1_child1.LayoutY);
Assert.AreEqual(214f, root_child1_child1.LayoutWidth);
Assert.AreEqual(106f, root_child1_child1.LayoutHeight);

Assert.AreEqual(0f, root_child1_child1_child0.LayoutX);
Assert.AreEqual(0f, root_child1_child1_child0.LayoutY);
Assert.AreEqual(214f, root_child1_child1_child0.LayoutWidth);
Assert.AreEqual(106f, root_child1_child1_child0.LayoutHeight);

Assert.AreEqual(0f, root_child1_child2.LayoutX);
Assert.AreEqual(213f, root_child1_child2.LayoutY);
Assert.AreEqual(214f, root_child1_child2.LayoutWidth);
Assert.AreEqual(107f, root_child1_child2.LayoutHeight);

Assert.AreEqual(0f, root_child2.LayoutX);
Assert.AreEqual(0f, root_child2.LayoutY);
Assert.AreEqual(213f, root_child2.LayoutWidth);
Assert.AreEqual(320f, root_child2.LayoutHeight);
}

}
}
31 changes: 31 additions & 0 deletions gentest/fixtures/YGRoundingTest.html
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,34 @@
<div style="height: 10px; flex-grow:1;"></div>
<div style="height: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_inner_node_controversy_horizontal" experiments="Rounding" style="width: 320px; flex-direction: row;">
<div style="height: 10px; flex-grow:1;"></div>
<div style="height: 10px; flex-grow:1;">
<div style="height: 10px; flex-grow:1;">
</div>
</div>
<div style="height: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_inner_node_controversy_vertical" experiments="Rounding" style="height: 320px;">
<div style="width: 10px; flex-grow:1;"></div>
<div style="width: 10px; flex-grow:1;">
<div style="width: 10px; flex-grow:1;">
</div>
</div>
<div style="width: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_inner_node_controversy_combined" experiments="Rounding" style="width: 640px; height: 320px; flex-direction: row;">
<div style="height: 100%; flex-grow:1;"></div>
<div style="height: 100%; flex-grow:1; flex-direction: column;">
<div style="width: 100%; flex-grow:1;"></div>
<div style="width: 100%; flex-grow:1;">
<div style="flex-grow:1; width: 100%;">
</div>
</div>
<div style="width: 100%; flex-grow:1;"></div>
</div>
<div style="height: 100%; flex-grow:1;"></div>
</div>
Loading

0 comments on commit aa5b296

Please sign in to comment.