Skip to content

[5.4] Ratio validation - allow precision to be based on size of image#19542

Merged
taylorotwell merged 4 commits into
laravel:5.4from
charliekassel:5.4
Jun 13, 2017
Merged

[5.4] Ratio validation - allow precision to be based on size of image#19542
taylorotwell merged 4 commits into
laravel:5.4from
charliekassel:5.4

Conversation

@charliekassel

@charliekassel charliekassel commented Jun 9, 2017

Copy link
Copy Markdown
Contributor

The precision required to pass ratio validation is quite strict.

Images must have full pixels dimensions when cropping or resizing an image to a specific ratio, decimals must be rounded either up or down to the nearest pixel. This creates an inaccuracy of the required ratio and the inaccuracy is amplified the smaller the image is.

This PR changes the precision required based on the size of the image.

Precision based on max pixel dimension of image

>>> 100/(100*100)
=> 0.01
>>> 1000/(1000*1000)
=> 0.001
>>> 10000/(10000*10000)
=> 0.0001

Example inaccuracy of ratio based on above image sizes

>>> abs(2/3 - 66/100)
=> 0.0066666666666666
>>> abs(2/3 - 666/1000)
=> 0.00066666666666659
>>> abs(2/3 - 6666/10000)
=> 6.6666666666659E-5

So this change allows these proportions to pass the validation, whereas they would all have failed given a 0.000001 precision factor.

@KKSzymanowski

Copy link
Copy Markdown
Contributor

I don't get this line:

$precision = $maxDimension / $maxDimension ** 2;

Isn't that equivalent to:

$precision = 1 / $maxDimension;

@themsaid

Copy link
Copy Markdown
Member

@charliekassel why don't we just change the 0.000001?

@themsaid themsaid closed this Jun 10, 2017
@themsaid themsaid reopened this Jun 10, 2017
@charliekassel

charliekassel commented Jun 11, 2017

Copy link
Copy Markdown
Contributor Author

@KKSzymanowski - it is indeed sir. Your math is better than mine.
Adjusted in d3b784d

@themsaid

Copy link
Copy Markdown
Member

@charliekassel can you please share a real world example of the problem? What image size and what ration fails?

@charliekassel

Copy link
Copy Markdown
Contributor Author

Expanding on the examples above.
If we wanted an 2/3 ratio image 100px wide the closest we can get is 67px rounded up.
Using the calculation in failsRatioCheck that would be:

abs(2/3 - 67/100); // 67px x 100px - 0.0033333333333334 which is greater that 0.000001 so fails.
abs(2/3 - 66/100); // 66px x 100px - 0.0066666666666666 - fails
abs(2/3 - 667/1000); // 667px x 1000px - 0.00033333333333341 - fails
abs(2/3 - 6667/10000); // 6667px x 10000px - 0.00003333333333333 - fails
abs(2/3 - 66667/100000); // 66667px x 100000px - 0.0000033333333333552 - fails
abs(2/3 - 666667/1000000); // 666667px x 1000000px - 0.00000033333333337993 - passes

So you see for a 2/3 ratio image we need go up to 1 million pixels in width in order to pass current precision rules. That's a pretty big image.

Using a precision based on the size of the maximum dimension for above sizes we would get:

0.0033333333333334 > 0.01 === false // 67px x 100px - passes
0.0066666666666666 > 0.01 === false // 66px x 100px - passes
0.00033333333333341 > 0.001 === false // 667px x 1000px - passes
0.00003333333333333 > 0.0001 === false // 6667px x 10000px - passes
0.0000033333333333552 > 0.00001 === false // 66667px x 100000px - passes
0.00000033333333337993 > 0.000001 === false // 666667px x 1000000px - passes

@charliekassel

Copy link
Copy Markdown
Contributor Author

I have added a fixture and a test to assert this works.

@themsaid

Copy link
Copy Markdown
Member

@charliekassel I see now, thanks. I think this is ok

@taylorotwell
taylorotwell merged commit ca3955f into laravel:5.4 Jun 13, 2017
@OrkhanAlikhanov

This comment has been minimized.

@laravel laravel locked as off-topic and limited conversation to collaborators Aug 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants