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

Pixel aligned CGFloat equality fix #128

Merged
merged 1 commit into from
Mar 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions MagazineLayout/LayoutCore/Types/ScreenPixelAlignment.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ extension CGFloat {
(self * scale).rounded() / scale
}

/// Tests `self` for approximate equality using the threshold value. For example, 1.48 equals 1.52 if the threshold is 0.05.
/// `threshold` will be treated as a positive value by taking its absolute value.
func isEqual(to rhs: CGFloat, threshold: CGFloat) -> Bool {
abs(self - rhs) <= abs(threshold)
/// Tests `self` for approximate equality, first rounding the operands to be pixel-aligned for a screen with the given
/// `screenScale`. For example, 1.48 equals 1.52 if the `screenScale` is `2`.
func isEqual(to rhs: CGFloat, screenScale: CGFloat) -> Bool {
let lhs = alignedToPixel(forScreenWithScale: screenScale)
let rhs = rhs.alignedToPixel(forScreenWithScale: screenScale)
return lhs == rhs
Comment on lines +27 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the actual fix - the rest of the PR is just some minor refactoring

}

}
27 changes: 13 additions & 14 deletions MagazineLayout/Public/MagazineLayout.swift
Original file line number Diff line number Diff line change
Expand Up @@ -584,22 +584,21 @@ public final class MagazineLayout: UICollectionViewLayout {
}

override public func shouldInvalidateLayout(forBoundsChange newBounds: CGRect) -> Bool {
// When using the topToBottom layout direction, we only want to invalidate the layout when the
// widths differ. When using the bottomToTop layout direction, we want to invalidate on any
// size change due to the requirement of needing to preserve scroll position from the bottom
let isSameWidth = currentCollectionView.bounds.size.width.isEqual(
to: newBounds.size.width,
screenScale: scale)
let shouldInvalidateDueToSize: Bool
switch verticalLayoutDirection {
case .topToBottom:
shouldInvalidateDueToSize = !currentCollectionView.bounds.size.width.isEqual(
to: newBounds.size.width,
threshold: 1 / scale)
shouldInvalidateDueToSize = !isSameWidth
case .bottomToTop:
shouldInvalidateDueToSize = !(currentCollectionView.bounds.size.width.isEqual(
to: newBounds.size.width,
threshold: 1 / scale) &&
currentCollectionView.bounds.size.height.isEqual(
// When using the topToBottom layout direction, we only want to invalidate the layout when the
// widths differ. When using the bottomToTop layout direction, we want to invalidate on any
// size change due to the requirement of needing to preserve scroll position from the bottom
let isSameHeight = currentCollectionView.bounds.size.height.isEqual(
to: newBounds.size.height,
threshold: 1 / scale))
screenScale: scale)
shouldInvalidateDueToSize = !isSameWidth || !isSameHeight
}

return shouldInvalidateDueToSize || hasPinnedHeaderOrFooter
Expand Down Expand Up @@ -638,7 +637,7 @@ public final class MagazineLayout: UICollectionViewLayout {

let isSameHeight = preferredAttributes.size.height.isEqual(
to: originalAttributes.size.height,
threshold: 1 / scale)
screenScale: scale)
let hasNewPreferredHeight = !isSameHeight

switch (preferredAttributes.representedElementCategory, preferredAttributes.representedElementKind) {
Expand All @@ -655,7 +654,7 @@ public final class MagazineLayout: UICollectionViewLayout {
at: preferredAttributes.indexPath)
let isSameHeight = preferredAttributes.size.height.isEqual(
to: currentPreferredHeight ?? -.greatestFiniteMagnitude,
threshold: 1 / scale)
screenScale: scale)
return hasNewPreferredHeight && !isSameHeight
case nil:
return false
Expand Down Expand Up @@ -789,7 +788,7 @@ public final class MagazineLayout: UICollectionViewLayout {
// because the collection view's width can change without a `contentSizeAdjustment` occurring.
let isSameWidth = collectionView?.bounds.size.width.isEqual(
to: cachedCollectionViewWidth ?? -.greatestFiniteMagnitude,
threshold: 1 / scale)
screenScale: scale)
?? false
if !isSameWidth {
prepareActions.formUnion([.updateLayoutMetrics, .cachePreviousWidth])
Expand Down
16 changes: 5 additions & 11 deletions Tests/ScreenPixelAlignmentTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,11 @@ final class ScreenPixelAlignmentTests: XCTestCase {
// MARK: Approximate equality tests

func testApproximateEquality() {
XCTAssert(CGFloat(1.48).isEqual(to: 1.52, threshold: 0.05))
XCTAssert(!CGFloat(1.48).isEqual(to: 1.53, threshold: 0.05))

XCTAssert(CGFloat(1).isEqual(to: 10, threshold: 9))
XCTAssert(!CGFloat(1).isEqual(to: 11, threshold: 9))

XCTAssert(CGFloat(1).isEqual(to: 10, threshold: 9))
XCTAssert(!CGFloat(1).isEqual(to: 11, threshold: 9))

XCTAssert(CGFloat(1.333).isEqual(to: 1.666, threshold: 1 / 3))
XCTAssert(!CGFloat(1.332).isEqual(to: 1.666, threshold: 1 / 3))
XCTAssert(CGFloat(1.48).isEqual(to: 1.52, screenScale: 2))
XCTAssert(!CGFloat(1).isEqual(to: 10, screenScale: 9))
XCTAssert(!CGFloat(1).isEqual(to: 10, screenScale: 9))
XCTAssert(!CGFloat(1).isEqual(to: 9, screenScale: 9))
XCTAssert(!CGFloat(1.333).isEqual(to: 1.666, screenScale: 3))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what was I thinking - of course these aren't equal. They're literally 1px apart on a 3x screen! 🤦‍♂️

}

}
Expand Down
Loading