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

Rounding error while auto scaling dimensions #238

Closed
rapka opened this issue Jul 1, 2015 · 3 comments
Closed

Rounding error while auto scaling dimensions #238

rapka opened this issue Jul 1, 2015 · 3 comments
Labels
Milestone

Comments

@rapka
Copy link

rapka commented Jul 1, 2015

It appears as though there are rounding errors that occur in certain scenarios when auto scaling the height of images during resizing. In my specific case, calling resize(300, null) on a 165x220 JPG results in an image that's 300x399 rather than 300x400.

@lovell
Copy link
Owner

lovell commented Jul 1, 2015

Hi Richard, thanks for reporting this problem. It looks like the cause is a (lack of) floating point precision.

The closest double-precision floating point value to the integer value of 400 is 399.999999999999943156581139191985130310058593750, which is converted to 399 by floor.

For the auto-scaling width and height calculations, I think we may have to instead use round (I need to remember why floor was used here in the first place) or add a very small number e.g. 0.000000001 before floor. Leave it with me.

@lovell lovell added the bug label Jul 1, 2015
@lovell lovell added this to the v0.11.0 milestone Jul 3, 2015
@lovell
Copy link
Owner

lovell commented Jul 3, 2015

Commit b8885c1 on the knife branch switches from the use of floor to round, which improves the "safety" of converting from float to integer, but also makes the resultant dimensions more closely match users' expectations.

This will be part of the forthcoming v0.11.0 release, but you can test now using:

npm install lovell/sharp#knife

(I'd prefer to avoid a patch-release for this as it introduces a slight change to existing behaviour.)

Thanks again for reporting this - I'm amazed it has taken this long to come up.

@lovell
Copy link
Owner

lovell commented Jul 13, 2015

This is now in master awaiting release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants