Added feature to use rounded values #256

woehrl01 · 2016-11-19T14:00:20Z

Added an experimental feature to allow to use only rounded values. See #184. It's not a perfect solution and definitely can be further improved. I'm looking forward to your ideas.

emilsjolander · 2016-11-19T14:43:27Z

Thanks! Pretty hard to review whether this does what we want though. Could you add a test (CSSLayoutPixelRoundingTest.cpp) which tests this thoroughly? I'm specifically interested how this works together with flex-grow and flex-shrink.

woehrl01 · 2016-11-19T21:47:56Z

I just tried to add some test, but I can't get it to work to modify gentest.js to output the values as float. For a simple case like the following:

<div id="pixel_rounding_flex_basis_flex_grow_row" style="width: 100px; height: 100px; flex-direction: row;">
  <div style="flex-grow: 1;"></div>
  <div style="flex-grow: 1;"></div>
  <div style="flex-grow: 1;"></div>
</div>

it returns 33, 34, 33 as width. But it should return 33.3333, 33.3333, 33.3333.

Did I miss something?

emilsjolander · 2016-11-19T21:50:18Z

I suggest for this that you right a test manually which tests this specific
feature and don't use gentest. Gentest is used for testing style features
and make sure they render the same as browsers. We have manually written
tests as well for example the CSSLayoutMeasureCacheTest.cpp as well as
others.
On Sat, 19 Nov 2016 at 21:47, Lukas Wöhrl [email protected] wrote:

I just tried to add some test, but I can't get it to work to modify
gentest.js to output the values as float. For a simple case like the
following:

it returns 33, 34, 33 as width. But it should return 33.3333, 33.3333,
33.3333.

[image: image]
https://cloud.githubusercontent.com/assets/2535856/20458607/f517696c-aea9-11e6-90b9-3a594da2fa0d.png

Did I miss something?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#256 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdIpC4JYUjnRoS30bDOp-L5fOmp4TZzks5q_26OgaJpZM4K3Qp3
.

woehrl01 · 2016-11-19T22:44:18Z

Jep, I think this is the way to go. I just wanted to use gentest.js as a pre-step of the manually written tests.
I just figured out, that if I run this test (took me a little to setup gtest first), they fail as expected: 👍

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from CSSLayoutTest
[ RUN      ] CSSLayoutTest.pixel_rounding_flex_basis_flex_grow_row
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(51): error: Value of: CSSNodeLayoutGetWidth(root_child0)
  Actual: 33.3333
Expected: 33
[  FAILED  ] CSSLayoutTest.pixel_rounding_flex_basis_flex_grow_row (1 ms)

I had the wrong assumption that there are tests which return numbers with decimal places already. As there where some in the older code base. Had some wtf moments while figuring that out 😅.

facebook-github-bot · 2016-11-19T23:48:30Z

@woehrl01 updated the pull request - view changes

woehrl01 · 2016-11-19T23:51:18Z

I added some tests and changed the distribution logic of the remaining pixels. You now can even use the output from gentest.js directly. You only have to replace ASSERT_EQ with ASSERT_FLOAT_EQ and reference the "feature" test suite with TEST_F.

facebook-github-bot · 2016-11-20T00:16:40Z

@woehrl01 updated the pull request - view changes

facebook-github-bot · 2016-11-20T00:23:51Z

@woehrl01 updated the pull request - view changes

emilsjolander · 2016-11-20T12:11:10Z

Because browsers do rounding of pixel values automatically I would argue that this perhaps should not be an experimental feature but instead just part of the core library. Also the tests you committed don't seem to be generated via gentest.rb from a fixture provided in /gentext/fixtures. Also generated tests should not be hand modified in any way as they are not able to be re-generated.

woehrl01 · 2016-11-20T12:27:46Z

I haven't added the .html file for the generated test as it would be overriden on the next gen. I would suggest to change the generated tests to use ASSERT_FLOAT_EQ and be able to change the test fixture via data-testfixture=SomeClassName on the main div. So we can generate them more easily in future.

emilsjolander · 2016-11-20T12:32:00Z

To date we haven't needed any test fixtures so it has not been a problem really, once we do I think that might be a good idea. I think using ASSERT_FLOAT_EQ would be fine, please submit that as a separate pull request. As I said I don't think the fixture is needed here though as I'm not sure this should be an experimental feature. What do you think?

woehrl01 · 2016-11-20T13:18:38Z

I'll provide a PR for the gentest change.

I'm not sure if this should be a core feature. There are a few pros/cons for this.

Pro:

Result behaves graphically like the browser
Won't clutter the code with lot's of IFs and different behaviour according to having the feature enabled or not

Con:

Could be worse for the performance because of lots of rounding calculation.
Some end users maybe want to have fractial numbers as an endresult, maybe if you want to scale the output.

I did some tests with nodes like top: 0.3px; height: 113.4px;. They aren't calculated as in chrome, I found it pretty hard to adapt to the expected result, especially correctly distribution of the "missing pixels" over the children. I think this needs a bigger change, which aren't that easy to opt-out.

emilsjolander · 2016-11-20T13:26:22Z

Could be worse for the performance because of lots of rounding calculation

I'm not all too worried about this. We can test this and currently a lot of the higher level frameworks do their own rounding of values anyways. May be worth keeping this as experimental to be able to run these tests first though.

Some end users maybe want to have fractial numbers as an endresult, maybe if you want to scale the output.

Let's not speculate in what people may want. We have seen many people have the need for rounded output so let's only tackle this in the case we see a need for it.

I did some tests with nodes like top: 0.3px; height: 113.4px;. They aren't calculated as in chrome.

Could you elaborate?

woehrl01 · 2016-11-20T13:37:19Z

I agree that we shouldn't speculate.

Sure, I tried the following tests:

<div id="rounding_feature_fractial_1" style="height: 113.4px; width: 100px;">
  <div style="height: 20px; flex-grow:1; flex-basis:50px;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_feature_fractial_2" style="height: 113.6px; width: 100px;">
  <div style="height: 20px; flex-grow:1; flex-basis:50px;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_feature_fractial_3" style="top: 0.3px; height: 113.4px; width: 100px;">
  <div style="height: 20px; flex-grow:1; flex-basis:50px;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
</div>

<div id="rounding_feature_fractial_4" style="top: 0.7px; height: 113.4px; width: 100px;">
  <div style="height: 20px; flex-grow:1; flex-basis:50px;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
  <div style="height: 10px; flex-grow:1;"></div>
</div>

I got the code so that it almost works except for one test:

[ RUN      ] CSSLayoutFeatureRoundingTest.rounding_feature_fractial_2
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(444): error: Value of: CSSNodeLayoutGetHeight(root_child0)
  Actual: 64
Expected: 65
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(447): error: Value of: CSSNodeLayoutGetTop(root_child1)
  Actual: 64
Expected: 65
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(449): error: Value of: CSSNodeLayoutGetHeight(root_child1)
  Actual: 25
Expected: 24
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(454): error: Value of: CSSNodeLayoutGetHeight(root_child2)
  Actual: 24
Expected: 25
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(466): error: Value of: CSSNodeLayoutGetHeight(root_child0)
  Actual: 64
Expected: 65
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(469): error: Value of: CSSNodeLayoutGetTop(root_child1)
  Actual: 64
Expected: 65
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(471): error: Value of: CSSNodeLayoutGetHeight(root_child1)
  Actual: 25
Expected: 24
c:\users\lukas\dev\css-layout\tests\csslayoutpixelroundingtest.cpp(476): error: Value of: CSSNodeLayoutGetHeight(root_child2)
  Actual: 24
Expected: 25
[  FAILED  ] CSSLayoutFeatureRoundingTest.rounding_feature_fractial_2 (16 ms)

I'm not sure how "bad" those differences are, and if this is even relevant? (In my use case it isn't ;) )

But I only had an hour to look into this, yet. I just wanted to try out how it behaves with fractial input.

emilsjolander · 2016-11-20T15:12:50Z

Yeah I was going to mention that. We should expand the tests to handle when a user explicitly sets a fractional input value. We could round the number in the setter perhaps?

woehrl01 · 2016-11-20T19:05:27Z

Rounding the numbers via setter could be a possibility to lower the calculation overhead. But you won't get your input back via the getter.

My current implementation is rounding inside computedEdgeValue and round availableWidth and availableHeight at the top of layoutImpl. If you want I'll publish them, but it isn't perfect yet.

Do we have the requirement to have the distribution of the values over the children to be pixel perfect with chrome?

emilsjolander · 2016-11-20T19:13:03Z

Rounding the numbers via setter could be a possibility to lower the calculation overhead. But you won't get your input back via the getter.

This is fine as long as we document it.

Do we have the requirement to have the distribution of the values over the children to be pixel perfect with chrome?

We try to where possible. This makes it much easier to test our implementation.

…yout into rounding-feature

woehrl01 · 2016-11-21T17:11:13Z

I reworked the algorithm, now its more opt-out/-in and the tests are passing as expected. It's also more clean imho.

emilsjolander · 2016-11-21T17:14:38Z

CSSLayout/CSSLayout.c

@@ -2462,6 +2462,23 @@ bool layoutNodeInternal(const CSSNodeRef node,
  return (needToVisitNode || cachedResults == NULL);
 }

+
+void roundToPixelGrid(const CSSNodeRef node){


facebook-github-bot · 2016-11-21T17:15:11Z

@woehrl01 updated the pull request - view changes

emilsjolander · 2016-11-21T17:17:51Z

This is starting to look very good. Please also add tests for when input is a decimal value (width, height, flex-basis, left, top, right, bottom) as well as for when a measure function returns a decimal value.

facebook-github-bot · 2016-11-21T17:43:14Z

@woehrl01 updated the pull request - view changes

woehrl01 · 2016-11-21T17:44:26Z

made the method static and added some extended tests cases. Not sure how I should test the measure function returnment. Should I simply assume the correct result? As those can't be verified via chrome.

…t, as they are manually edited afterwards

facebook-github-bot · 2016-11-21T17:49:57Z

@woehrl01 updated the pull request - view changes

facebook-github-bot · 2016-11-21T17:54:03Z

@woehrl01 updated the pull request - view changes

emilsjolander · 2016-11-21T17:54:54Z

CSSLayout/CSSLayout.c

@@ -2549,7 +2549,7 @@ void CSSLog(CSSLogLevel level, const char *format, ...) {
  va_end(args);
 }

-static bool experimentalFeatures[CSSExperimentalFeatureCount + 1];


Let's keep this in here as it is very little memory and changing this back and forth when adding/removing experiments is just annoying.

emilsjolander · 2016-11-21T17:56:24Z

For measure function test just return something like {10.2, 10.2} and assert that the output width/height is what you expect {10, 10} I would assume.

facebook-github-bot · 2016-11-21T18:10:59Z

@woehrl01 updated the pull request - view changes

woehrl01 · 2016-11-21T18:12:05Z

Added two tests one for rounding down and one for rounding up.

facebook-github-bot · 2016-11-21T18:13:03Z

@woehrl01 updated the pull request - view changes

emilsjolander · 2016-11-21T18:18:18Z

Awesome! I'll import and test this. Once we have run this in production i'll probably remove the experimental flag. Sound good?

woehrl01 · 2016-11-21T18:20:16Z

Sounds pretty good to me! Looking forward to your results. 😄

facebook-github-bot · 2016-11-21T18:39:57Z

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

Summary: Added an experimental feature to allow to use only rounded values. See #184. It's not a perfect solution and definitely can be further improved. I'm looking forward to your ideas. Closes facebook/yoga#256 Reviewed By: splhack Differential Revision: D4214168 Pulled By: emilsjolander fbshipit-source-id: 6293352d479b7b4dad258eb3f9e0afaa11cf7236

vjeux · 2016-11-22T16:54:46Z

FYI, the code that does the same thing in React Native is: https://github.com/facebook/react-native/blob/master/React/Views/RCTShadowView.m#L107-L182

The way the algorithm works is to first get the global absolute position by summing up your position + the position of all your parents unrounded and then round it.

There are two benefits:

This ensures that 1px borders don't vanish. If you operate locally, your 1px border may be rounded down to 0 and disappear. But if you operate in a global space, you are guaranteed that they are all going to remain.
This ensures that you don't have rounding errors that propagate, this is especially important for weird aspect ratios like 1/3 which don't cleanly map to floating points.

It also not only rounds to the nearest integer but to the nearest pixel density. For example on a 2x retina you can have a value of 0.5 and on 3x you can have 0.33333.

woehrl01 · 2016-11-22T18:01:17Z

Thanks @vjeux . I'm not sure if RCTShadowView is doing what you think it does:

Frame.X and Frame.Y are Node.Left and Node.Right simply rounded.

Frame.Width is also simply rounding of Node.Width: (same for Frame.Height)

absoluteBottomRight.x - absoluteTopLeft.x 
= (absolutePosition.x + CSSNodeLayoutGetLeft(node) + CSSNodeLayoutGetWidth(node)) - (absolutePosition.x + CSSNodeLayoutGetLeft(node))
= absolutePosition.x + CSSNodeLayoutGetLeft(node) + CSSNodeLayoutGetWidth(node) - absolutePosition.x - CSSNodeLayoutGetLeft(node)
= CSSNodeLayoutGetWidth(node)

Please don't get me wrong but absolutePosition seems to have no relevance for me in that calculation.

I tried to match the calculation, as it is done in chrome, I took the absolute position into account and the result differed from the chrome output.

Thanks for the hint with the boarder, those values aren't rounded yet, this should definitely be done.

True that the pixel density is not taken into account, maybe density should be configurable?

Summary: Added an experimental feature to allow to use only rounded values. See facebook#184. It's not a perfect solution and definitely can be further improved. I'm looking forward to your ideas. Closes facebook/yoga#256 Reviewed By: splhack Differential Revision: D4214168 Pulled By: emilsjolander fbshipit-source-id: 6293352d479b7b4dad258eb3f9e0afaa11cf7236

added feature to use rounded values

69afffc

facebook-github-bot added the CLA Signed label Nov 19, 2016

added some tests and changed distribution logic to equal distribution

d27c661

fix include on non VS and no need to round when casting is enough

8f2c45e

Fix macro call

bcc524e

woehrl01 added 3 commits November 21, 2016 18:07

reworked the rounding algorithm to be more opt-in

c91e7e0

renamed method

4183d1b

Merge branch 'rounding-feature' of https://github.com/woehrl01/css-la…

b9d1219

…yout into rounding-feature

emilsjolander reviewed Nov 21, 2016

View reviewed changes

use static on roundToPixelGrid

c0a8746

added extended tests

438bee1

added the html layout of the test to the top, removed the gentest hin…

3baa632

…t, as they are manually edited afterwards

remove +1 as there is now a feature

8de20bc

emilsjolander reviewed Nov 21, 2016

View reviewed changes

read +1 and added tests for measure with fraction

1d72738

remove vs header

efd55e1

facebook-github-bot closed this in b90f6e2 Nov 22, 2016

Added feature to use rounded values #256

Added feature to use rounded values #256

Conversation

woehrl01 commented Nov 19, 2016

emilsjolander commented Nov 19, 2016

woehrl01 commented Nov 19, 2016

emilsjolander commented Nov 19, 2016

woehrl01 commented Nov 19, 2016

facebook-github-bot commented Nov 19, 2016

woehrl01 commented Nov 19, 2016

facebook-github-bot commented Nov 20, 2016

facebook-github-bot commented Nov 20, 2016

emilsjolander commented Nov 20, 2016

woehrl01 commented Nov 20, 2016

emilsjolander commented Nov 20, 2016

woehrl01 commented Nov 20, 2016

emilsjolander commented Nov 20, 2016

woehrl01 commented Nov 20, 2016

emilsjolander commented Nov 20, 2016

woehrl01 commented Nov 20, 2016

emilsjolander commented Nov 20, 2016

woehrl01 commented Nov 21, 2016

emilsjolander Nov 21, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Nov 21, 2016

emilsjolander commented Nov 21, 2016

facebook-github-bot commented Nov 21, 2016

woehrl01 commented Nov 21, 2016 • edited Loading

facebook-github-bot commented Nov 21, 2016

facebook-github-bot commented Nov 21, 2016

emilsjolander Nov 21, 2016

Choose a reason for hiding this comment

emilsjolander commented Nov 21, 2016

facebook-github-bot commented Nov 21, 2016

woehrl01 commented Nov 21, 2016

facebook-github-bot commented Nov 21, 2016

emilsjolander commented Nov 21, 2016

woehrl01 commented Nov 21, 2016

facebook-github-bot commented Nov 21, 2016

vjeux commented Nov 22, 2016

woehrl01 commented Nov 22, 2016

woehrl01 commented Nov 21, 2016 •

edited

Loading