fix: Snap computed sizes to whole pixels - #144
Merged
Merged
Conversation
constrain_both and contain are candidates for removal in vector2d 3.0. Both have equivalents in 2.3, so dropping them now keeps the eventual version bump a gemspec-only change. constrain_both is a plain alias for fit. contain is inlined as a private helper, which also removes a readability trap: the old size.fit(fit_size) and size.contain(fit_size) shared a receiver but meant structurally opposite things. No behavior change.
resize was documented as only scaling down, but it fits the image to max_size in both directions: a 320x200 image resized to "640x640" is rewritten at 640x400. Also document the string form of max_size, which was already accepted but undocumented.
ImageSizing#fit scales in floating point, so an axis that should land exactly on a whole pixel can come out a few ulps below it. Callers floor the result, turning that into a lost pixel. A 320x200 image requested at "322x" rendered at 319x200, and at "323x" it rendered at 320x199, instead of the image's own size in both cases. Plain downscaling drifts the same way: on a 3024x4032 image, 204 of the 3024 possible width-only requests came out a pixel short. The crop path loses a row or column for the same reason. Snap any axis that scaling leaves within a rounding error of a whole pixel onto it.
ImageSizing#fit conflicted: both sides added a step after the contain call. Sizes are now snapped to whole pixels first and the result validated afterwards, so a size that rounds up to a pixel isn't rejected for landing a rounding error below one. #snap leaves non-finite values alone, so require_pixels! keeps reporting those as InvalidSizeOptions.
It has a single use site.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ImageSizing#fitscales in floating point, so an axis that should land exactly on a whole pixel can come out a few ulps below it, and since every caller floors the result, that pixel is lost. A 320x200 image requested at322xrendered at 319x200 and at323xat 320x199 instead of its own size in both cases; plain downscaling drifts the same way, with 204 of the 3024 possible width-only requests on a 3024x4032 image coming out short, and the crop path losing a row or column for the same reason. Snapping any axis that scaling leaves within a rounding error of a whole pixel onto it covers all three sites at once.The snap runs before #142's
require_pixels!so that a size rounding up to one pixel isn't rejected for landing a rounding error below it, and#snappasses non-finite values through untouched so that guard still reports them asInvalidSizeOptions.This dates back to 8da8729 (2014), which both swapped the clamp to run after the fit and changed the final quantization from
roundtofloor— neither half produces the bug alone.