Skip to content

[Issue 838] Update ASCeilPixelValue and ASRoundPixelValue#864

Merged
Adlai-Holler merged 2 commits into
TextureGroup:masterfrom
rcancro:ASCeilPixelValue
Mar 31, 2018
Merged

[Issue 838] Update ASCeilPixelValue and ASRoundPixelValue#864
Adlai-Holler merged 2 commits into
TextureGroup:masterfrom
rcancro:ASCeilPixelValue

Conversation

@rcancro

@rcancro rcancro commented Mar 29, 2018

Copy link
Copy Markdown
Contributor

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

@nguyenhuy nguyenhuy left a comment

Copy link
Copy Markdown
Member

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!

@wiseoldduck wiseoldduck left a comment

Copy link
Copy Markdown
Member

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

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

@Adlai-Holler

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

rcancro commented Mar 30, 2018

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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.

4 participants