Skip to content
This repository has been archived by the owner on Aug 1, 2020. It is now read-only.

Crop zone exceeds natural image size #34

Closed
orangewarp opened this issue Jul 5, 2014 · 11 comments
Closed

Crop zone exceeds natural image size #34

orangewarp opened this issue Jul 5, 2014 · 11 comments

Comments

@orangewarp
Copy link

In certain scenarios, the crop data object is returning coordinates larger than the image itself. See image example:

screen shot 2014-07-05 at 4 56 24 pm

Thus, when passed to a server we receive an error like this:
{"code":142,"error":"["Crop height should be in the range [1, height - top].","Crop height should be in the range [1, height - top]."]"}

@fengyuanchen
Copy link
Owner

Thank you very much! I will fix it soon!

fengyuanchen pushed a commit that referenced this issue Jul 6, 2014
@orangewarp
Copy link
Author

It's a great tool. :-) Appreciate your work.

On Sunday, July 6, 2014, Fengyuan Chen notifications@github.com wrote:

Thank you very much! I will fix it soon!


Reply to this email directly or view it on GitHub
#34 (comment).

@fengyuanchen
Copy link
Owner

Thanks~ It's OK now.

@orangewarp
Copy link
Author

Nice. I've been testing it and so far no errors. Heh heh. Math.round vs Math.floor. ;-)

@orangewarp orangewarp reopened this Jul 7, 2014
@orangewarp
Copy link
Author

Guess I spoke too soon. :-)

There are still some edge cases where the height and y2 can exceed the size of the original image. The issue seems to be with the transformData() function. The cropper data passed in is correct and from first glance, the passed in ratio multiplier seems good too. But at the most extreme crop areas (cropper size is maxed out in one dimension) the end return of the transformData() function can slightly exceed the original image size. We're talking 1 pixel per ~ 900. I've only seen this happen on the y dimension although you might want to check the x as well.

This might be solved by having a validation sub-routine to make sure the boundaries are within the naturalHeight and naturalWidth. If a dimension (or both) exceed, data will reduce the exceeding crop dimension by the difference and reduce the other proportionally, taking the aspectRatio in account.

See console output with various console.log() markers from the source.

screen shot 2014-07-07 at 5 29 29 pm

@fengyuanchen
Copy link
Owner

Well, I'll check it carefully. Thanks again~

fengyuanchen pushed a commit that referenced this issue Jul 8, 2014
@nbibler
Copy link

nbibler commented Jul 10, 2014

It appears as though something since 14c20fe has negatively affected setData. It appears to be an internal rounding error causing the results of getData to be different from the given x1, y1, width, and height in setData. Sometimes it's off by 1px, sometimes 2-3px. It can be seen on the demo page, as well.

Here's a soundless video demo showing how changing any of the setData values fail to properly set the cropper location or size to those given values:

http://screencast.com/t/bP1FlBcq

@fengyuanchen
Copy link
Owner

@nbibler Thank you. I know, It's the reason of using Math.floor to transform the data. I will try to improve this as soon as possible.

@orangewarp
Copy link
Author

@fengyuanchen , Is the Math.floor or rounding etc. necessary at all? I was looking at the output results of using jcrop for comparison and they're sending pure decimal values as data crop output.

I went ahead and removed all rounding / flooring from the the cropper code and it seems to work just fine. Images and full height crops that were giving me errors before on my server seem to have disappeared.

Without any number adjusting cropper will output stuff like this...

Object {x1: 95.99999999999997, y1: 0, x2: 1920.0000000000002, y2: 1824.0000000000002, width: 1824.0000000000002…}

Which is being happily accepted by the back-end. I don't know if most server-side image processing takes decimal points as input but if this is indicative of the standard - maybe the number adjusting can be removed or at least, an option?

@fengyuanchen
Copy link
Owner

@orangewarp Fine! I will try to improve this in the next version.

@fengyuanchen
Copy link
Owner

@orangewarp I have improve this on v0.4.0, and the difference between input and output is less than 2px.

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

No branches or pull requests

3 participants