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

[Issue 838] Update ASCeilPixelValue and ASRoundPixelValue #864

Merged
merged 2 commits into from
Mar 31, 2018

Conversation

rcancro
Copy link
Contributor

@rcancro rcancro commented 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

#838

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
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

If you have time, please update ASFloorPixelValue as well. Thanks!

Copy link
Member

@wiseoldduck wiseoldduck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense for ceil() but I don't see the same reasoning applying to round()

@wiseoldduck
Copy link
Member

Yes I think ASFloorPixelValue would want +FLT_EPSILON, and ASRoundPixelValue would want to be left alone.

@Adlai-Holler
Copy link
Member

I am not a smart man… is it worth using ASCeilPixelValue? It seems like it's asking for this kind of trouble, and subpixel clipping of content isn't a serious risk.

Anyway, I support landing this change. I can't think of how it could produce bad results in practice.

@wiseoldduck
Copy link
Member

Yes fair warning I am quite naive as to how this is used in practice!

But am pretty sure if you apply this to ASRoundPixelValue and can come up with examples where it mattered it would be incorrect about half the time.

@wiseoldduck
Copy link
Member

OK I see the example in the comments cites 100.666666666669, not what I was expecting to see like 100.666666666667, where we were addressing just the inability to capture a nonterminating rational in a CGFloat. There is apparently some other source of inaccuracy than that! I will take this conversation somewhere else as it is now more about my enlightenment than constructive review. 😄

@rcancro
Copy link
Contributor Author

rcancro commented Mar 30, 2018

@wiseoldduck I think you are right -- We probably shouldn't do anything to round. I'll remove that.

@Adlai-Holler When creating an ASLayout, it uses ASCeilPixelValue on the given size. I don't know the performance implications, but in the past making sure you staying on pixel bounds was important for keeping text and images crisp.

I don't love this solution, but it with 3x devices in particular floats/doubles will try to represent a value they can't leading to garbage at some point. If we treat that garbage as true then we will see weird issues like these.

@Adlai-Holler Adlai-Holler merged commit 1e8c6f0 into TextureGroup:master Mar 31, 2018
@Adlai-Holler
Copy link
Member

Good change.

bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request 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

Successfully merging this pull request may close these issues.

None yet

4 participants