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

Create non-@MainActor-bound methods for rounding to pixel at scale #140

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

dfed
Copy link
Collaborator

@dfed dfed commented Oct 30, 2024

In #132 we made it difficult to work with aspect ratios when the scale factor is known a-priori and does not need to be derived on the main actor. Here we make it easy again through the magic of copy and paste.

@dfed dfed requested a review from NickEntin October 30, 2024 06:12
@dfed dfed self-assigned this Oct 30, 2024
@@ -27,7 +27,7 @@ public protocol ScaleFactorProviding {
extension UIScreen: ScaleFactorProviding {

public var pixelsPerPoint: CGFloat {
return scale
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did some stylizing in this file while I was here 😅

@dfed dfed force-pushed the dfed--main-actor-less branch 2 times, most recently from 35ddc34 to 1d74c6b Compare October 30, 2024 06:15
@NickEntin
Copy link
Collaborator

NickEntin commented Oct 30, 2024

A (much more breaking) alternative that might be worth considering here is removing pixel rounding from AspectRatio. I'd argue we're being a little too magical today with the rounding, since it's not clear whether your resulting size will be wider/taller than the intended aspect ratio with rounding applied. It would be nice to make any applied rounding more explicit, e.g.

aspectRatio.size(forWidth: ...).ceiledToPixel()

or

aspectRatio.size(forWidth: ...).flooredToPixel()

Not tonight though. Let's open an issue for this.

Comment on lines -61 to -67
extension Int: ScaleFactorProviding {

public var pixelsPerPoint: CGFloat {
return CGFloat(self)
}

}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

big ol' breaking change. but... I doubt this is used much.

@dfed dfed force-pushed the dfed--main-actor-less branch from 5446950 to 0267011 Compare October 30, 2024 06:48
Paralayout/PixelRounding.swift Outdated Show resolved Hide resolved
@dfed dfed enabled auto-merge October 30, 2024 08:11
@dfed dfed merged commit abd41b4 into master Oct 30, 2024
5 checks passed
@dfed dfed deleted the dfed--main-actor-less branch October 30, 2024 19:43
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.

2 participants