Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

width calculation error after setting flexBasis and flexWrap attribute #838

Open
0x30 opened this issue Mar 16, 2018 · 5 comments
Open

width calculation error after setting flexBasis and flexWrap attribute #838

0x30 opened this issue Mar 16, 2018 · 5 comments

Comments

@0x30
Copy link

0x30 commented Mar 16, 2018

AsyncDisplayKit some Waring

Width calculation error after setting flexBasis attribute

I don’t know if I’m dealing with this issue

When I set flexWrap using ASStackLayoutSpec I found this problem. In the end I found out that because of the calculation, the width was out of range. This main problem appears in the following code
ASInternalHelpers ASCeilPixelValue

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat scale = ASScreenScale();
  return ceil(f * scale) / scale;
}

For example
in iPhoneX, the width is 375 and flexBasis is set to 0.5. The scale is 3.
The result is 186.666666666666....
Caused the width to exceed 375

Reproduce this problem

AsyncCeilWidthError

@rcancro
Copy link
Contributor

rcancro commented Mar 19, 2018

I noticed something similar when creating an ASStackLayoutSpec. The stack's computed height comes back as 147.66666666666669. This value is then place in an ASLayout which rounds the size vial ASCeilPixelValue. On a 3x device does the the following:

147.666666669 * 3 = 443.0000000000000697
ceil(443.0000000000000697) = 444
444/3 = 148

If there wasn't any precision error on the original stack layout's height, this wouldn't happen:

147.66666666 * 3 = 442.999999997999 *this is the value the debugger gave me
ceil(442.999999997999) = 443
443/3 = 147.6666666666

Would this be a reasonable change to ASCeilPixelValue?

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat scale = ASScreenScale();
  ceil(f * scale - FLT_EPSILON) / scale;
}

Or is that too hacky and heavy handed?

@0x30
Copy link
Author

0x30 commented Mar 20, 2018

@rcancro Maybe I know how to solve this problem. Although he still sometimes has the problem mentioned above, it is much better. Maybe we shouldn't use percentages to set the width. It's probably a good idea to use constrainedSize: ASSizeRange to calculate the actual width.

@nguyenhuy
Copy link
Member

nguyenhuy commented Mar 21, 2018

I think we're definitely dealing with an issue caused by floating points precision on 3x devices. I think a solution along the line of @rcancro's is the best we can do.

@rcancro Any reason to disregard the trailing bits of f * scale instead of f itself? Like this:

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat scale = ASScreenScale();
  return ceil((f - FLT_EPSILON) * scale) / scale;
}

Edit: Another solution is to use ceilf which, on 64-bit devices, casts the double value to float (and trims trailing bits along the way, which is what we want):

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat scale = ASScreenScale();
  return ceilf(f * scale) / scale;
}

@rcancro
Copy link
Contributor

rcancro commented Mar 21, 2018

I agree this is better:

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat scale = ASScreenScale();
  return ceil((f - FLT_EPSILON) * scale) / scale;
}

I think we'd want to stay in double if possible. Converting from double to a float will still put us in a position where we are trying to put more precision into a float than it can handle. Look at these examples:

screen shot 2018-03-21 at 12 17 11 pm

The last digit of the float is still not what we want.

Another idea is we may be able to decide how many decimal points we think are significant and compute based on those. For example, if we want 5 digits of layout precision we could try:

CGFloat ASCeilPixelValue(CGFloat f)
{
  CGFloat preciseEnough = (NSInteger)(f * 100000)/100000.0;
  CGFloat scale = ASScreenScale();
  return ceil(preciseEnough * scale) / scale;
}

As long as we require less precision than double/float can give us, we should avoid the precision errors at the end (I hope?).

screen shot 2018-03-21 at 12 20 35 pm

@nguyenhuy
Copy link
Member

@rcancro I agree with either of your solutions, although for simplicity, I'd prefer the first one (ceil((f - FLT_EPSILON) * scale) / scale).

rcancro added a commit to rcancro/Texture that referenced this issue Mar 29, 2018
Layouts can come back for 3x devices as a repeating decimal. Floats/doubles have a hard time representing these values and often the last digit or two will be  garbage. This garbage can result in unexpected values from `ceil` or `round`. I try to fix this by subtracting `FLT_EPSILON` from the value before calling `ceil` or `round`

TextureGroup#838
Adlai-Holler pushed a commit that referenced this issue Mar 31, 2018
* [Issue 838] Update ASCeilPixelValue and ASRoundPixelValue

Layouts can come back for 3x devices as a repeating decimal. Floats/doubles have a hard time representing these values and often the last digit or two will be  garbage. This garbage can result in unexpected values from `ceil` or `round`. I try to fix this by subtracting `FLT_EPSILON` from the value before calling `ceil` or `round`

#838

* addressed comments on the pr
bernieperez pushed a commit to AtomTickets/Texture that referenced this issue Apr 25, 2018
…up#864)

* [Issue 838] Update ASCeilPixelValue and ASRoundPixelValue

Layouts can come back for 3x devices as a repeating decimal. Floats/doubles have a hard time representing these values and often the last digit or two will be  garbage. This garbage can result in unexpected values from `ceil` or `round`. I try to fix this by subtracting `FLT_EPSILON` from the value before calling `ceil` or `round`

TextureGroup#838

* addressed comments on the pr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants