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

Add support for rectangle area crop #6

Open
sanketsaurav opened this issue Aug 10, 2014 · 40 comments
Open

Add support for rectangle area crop #6

sanketsaurav opened this issue Aug 10, 2014 · 40 comments

Comments

@sanketsaurav
Copy link

Would love it if you could include support for rectangular area crop, in addition to the existing circle and square.

@akempes
Copy link

akempes commented Aug 15, 2014

+1

@chirgwin
Copy link

This is implemented in pull request #1

@paulflo150
Copy link

chirgwin, I am noticing a couple of issues with your build:

  1. when using circle/ square the cropped image will have an incorrect AR and display more than the cropped area, until you click on the handle bars, at which point the selection area is very jumpy.
  2. the area-min-size does not seem to be respected, if I use the circle selection, the min area seems to be the size of my cropped image.

Thanks,

@chirgwin
Copy link

@paulflo150 I've just merged some additional selection fixes from the next branch into master on my fork, but square/circle selections remain a little buggy. These changes are also available on the 0.2.1 branch, which includes the build products and can be referenced directly from bower like so:
"ngImgCrop": "PortChaw/ngImgCrop#0.2.1"

I've been able to reproduce both the aspect ratio and minimum size bugs you point out and will work on fixes for those. Automated test would be very helpful in avoiding regressions like these. I've been meaning to write some, but haven't gotten around to it yet.

@paulflo150
Copy link

Hey chirgwin, thanks for the great work, looking forward to the fixes.

I just tried the latest build, and I am now seeing an issue with the selection resizing on circle/ square are types. I do not see the resize cursor anymore, and the resizing does not seem to be animated anymore. It works perfectly with the rectangle.

Also, how would you go about maintaining the AR of the original image when using the rectangle area type. I would be perfectly fine if the selection would keep the same AR as the image, which in turn would ensure the cropped image also has the correct AR, however I am not sure if that would be correct or not. Suppose you have an original image that is 600 x 300 pixels, what I would like to do is for the cropped image to also have the same AR by specifying a max width and a max height. For instance I want this max width to be 150 and max height 100, and assuming I am selecting the whole image, my cropped image would become 150 (my max width) by 75 px. Vice versa, if the image was 300 by 400, the cropped image would be 75 by 100 px (my max height).

Any thoughts on how to achieve this?

Thanks,

@paulflo150
Copy link

Just quick follow up: I think that _dontDragOutside got left out of one of the files.

@paulflo150
Copy link

Ok, so it appears that when resetHost is called it does not know anything about the area type, and the initial crop box gets created based on both the width and height of the image, but in case of a square or circle these should be the same. I fixed it by passing the areaType to resetCropHost, although you may have a better way of doing this:

// Resets CropHost
var resetCropHost = function (areaType) {
if (image !== null) {
.............................................................................
var cw = ctx.canvas.width;
var ch = ctx.canvas.height;
if ((areaType === 'circle') || (areaType === 'square')) {
cw = ch = Math.min(cw, ch);
}

                theArea.setSize({
                    w: Math.min(200, cw / 2),
                    h: Math.min(200, ch / 2)
                });

@chirgwin
Copy link

@paulflo150 Thanks for that fix. Looks reasonable. If you want to create a pull request on my fork, I'll gladly take a closer look and merge it. Or, I can incorporate this change myself.

The drawing issue you're seeing should be fixed in commit be414c2 in master on my fork (and appended to pull request #1). I've just merged this fix into the 0.2.1 branch, which includes updated pre-built files. Square/circle selection bounds checking still needs a little work, but it's functioning better.

On your suggestions, I think what would allow the most flexibility would be to expose an arbitrary fixed aspect ratio (in addition to 1:1, as currently supported by square selection) for rectangle selections as an attribute in the directive. You could also add a parameter to automatically determine this aspect ratio of the selection from the dimensions of the loaded image, effectively pegging the selection aspect ratio to the original image aspect ratio. This, in combination with the maximum selection dimensions, should accomplish what you're describing.

I was hoping to keep my changes on rectangular selections minimal until @alexk111 has a chance to review pull request(s) and I'll defer to him on design changes/feature requests. That said, I will likely continue scratching my own itches on the next branch of my fork.

@paulflo150
Copy link

Thanks for getting these fixes in so quickly!

It may be easier if you implemented those changes as I am not sure which branch to fork :), here are all my changes for your review:

  1. // Sync CropHost with Directive's options
    scope.$watch('image', function () {
    cropHost.setNewImageSource(scope.image, scope.areaType);
    });
  2. this.setNewImageSource = function (imageSource, areaType) {
    resetCropHost(areaType);
    ..............................
    newImage.onload = function () {
    ..............................................
    resetCropHost(areaType);
  3. the snippet I pasted above.

Oh and I also removed _dontDragOutside from two places - you may have already fixed this as I have not looked at the latest build yet.

Thanks again!

chirgwin added a commit to PortChaw/ngImgCrop that referenced this issue Aug 17, 2014
chirgwin added a commit to PortChaw/ngImgCrop that referenced this issue Aug 17, 2014
…ing around the type; improvements to suggestion by @paulflo150 in issue alexk111#6
@sanketsaurav
Copy link
Author

Hey @chirgwin! Thanks for taking out the time to implement this. I tried to test it out, however, but I couldn't get it right somehow. Here's what happening. And this is the fiddle.

screenshot from 2014-08-18 18 55 25

@chirgwin
Copy link

@sanketsaurav The problem here is that the output image isn't being resized with the selection (see issue #4 and pull request #7). I've cherry-picked these changes from #7 into the 0.2.1 branch of my fork and tweaked them a little to get things working.

This fork of your fiddle puts it all together.

@ocombe
Copy link

ocombe commented Aug 26, 2014

Awesome, I look forward to this implementation so that I can start using this lib :)

@divan
Copy link

divan commented Sep 12, 2014

Guys, thanks for the fork. Is there a chance to implement fixed ratio rectangle option?

@chirgwin
Copy link

As requested at least a couple of times in this thread, @sderungs has implemented arbitrary fixed aspect ratios for rectangular selections. Thanks for that. This is now merged into master of my fork for this issue.

@gerardlopez
Copy link

Thanks for the fork implementing the fixed aspect ratio.I tried it and I've seen a problem, as doc says:

"The result image will always be a square for the both circle and square area types."

This shouldn't be for the rectangle areaType.

A quick fix for me has been to change the setResultImageSize method adding this before drawScene:

if (theArea._aspectRatio) {
size.h = size.h / theArea._aspectRatio;
}
resImgSize=size;

@iceye
Copy link

iceye commented Oct 15, 2014

Just added few lines to manage rectangle and aspect ratio on resultImage
http://jsfiddle.net/iceye/ryb31tj1/1/

Other useful infos printed out (original dimension and original image crop position and size)

@monad98
Copy link

monad98 commented Oct 16, 2014

@iceye
Thank you.
But there are some errors printed on the console.

Cannot read property 'w' of undefined
or
Error: [$compile:nonassign] http://errors.angularjs.org/1.2.26/$compile/nonassign?p0=undefined&p1=imgCrop

@iceye
Copy link

iceye commented Oct 16, 2014

I saw, I need to add some checks because the code is called also when there's no image uploaded.
Just fixed on my fork http://jsfiddle.net/iceye/ryb31tj1/

There's the other error
Error: [$compile:nonassign] Expression 'undefined' used with directive 'imgCrop' is non-assignable!
but I don't exactly know why and where it comes from

Thank you.

@monad98
Copy link

monad98 commented Oct 16, 2014

@iceye
While I was testing, I got this error.

Error: [$compile:nonassign] http://errors.angularjs.org/1.2.26/$compile/nonassign?p0=undefined&p1=imgCrop
    at Error (native)
    at https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:6:450
    at n (https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:53:142)
    at Object.<anonymous> (https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:53:234)
    at k.$digest (https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:109:289)
    at k.$apply (https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:112:398)
    at http://localhost:9000/scripts/ng-img-crop.js:1291:35
    at https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:122:399
    at e (https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:37:497)
    at https://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js:41:235 

line 1291 is like this. "scope.$apply ..."

var fnSafeApply=function(fn) {
    return function(){
        $timeout(function(){
            scope.$apply(function(scope){
            fn(scope);
            });
       });
    };
};

Do you have some ideas?

@iceye
Copy link

iceye commented Oct 16, 2014

@monad98

I don't get why this is happening, the problem is this:

Error: [$compile:nonassign] Expression 'undefined' used with directive 'imgCrop' is non-assignable!

probably it's because I added scope variables assignments in the wrong way. The strange things is that the demo is working 0_o
I will try to debug this but I think will not be simple to find out the problem

Hope you have better luck

@iceye
Copy link

iceye commented Oct 16, 2014

@monad98

Ok, fixed (problem was caused by 'areaCoords' scope variable declaration):
http://jsfiddle.net/iceye/ryb31tj1/

@monad98
Copy link

monad98 commented Oct 17, 2014

@iceye
awesome! :)

@iceye
Copy link

iceye commented Oct 17, 2014

@monad98
Thx :)

@alexk111
Are you planning to merge this mod?

@chirgwin
Copy link

@iceye I'll gladly merge your changes into my fork if you create a pull request. Ditto for other rectangle related changes.

@alexk111 What iceye said; would love to see this (pull request #1) merged into your mainline. The less desirable alternative would be for me (or others?) to maintain a rectangular fork indefinitely.

@zokipoki
Copy link

Nice work, regards to all of you.
But, cropped image will always be square, regardless what we are cropped, with rectangle, or with square (try to open cropped image in new page). How can fix, so result image be in proper shape (rectangle shape, as I cropped)? I see that there is an option to set "result-image-size", but it's limited to be square (if I could set two values for image size, it would be excellent).

@robianmcd
Copy link

@iceye I'm running into the same problem as @zokipoki when I try and use the ng-img-crop.js file that I compiled from your fork. Here is a plunker demonstrating the problem: http://plnkr.co/edit/GVtbzDiaCa1hi2ehCePs?p=preview

When rectangle is selected the size of the output image is staying a square. I also set an aspect-ratio but it doesn't appear to have an effect.

Any idea what I'm doing wrong?

Thanks!

EDIT: here is a plunker using the compiled ng-img-crop.js from @chirgwin 's fork: http://plnkr.co/edit/83NR06FNOno0N6qlp2GD?p=preview
I'm experiencing the same problem though (aspect-ratio doesn't work and output image is always square).

EDIT2: @chirgwin ah I see you have addressed this issue on your 0.2.1 branch. But there is no aspect ratio feature on that branch. Does anyone have a working demo that specifies an aspect-ratio?

@alexk111
Copy link
Owner

Hi guys. Sorry for the delayed response. Waiting for an update here #1

@chirgwin
Copy link

Thanks, @alexk111. #1 should be ready to merge.

@robianmcd Aspect ratio is covered by issue #17. See my comment about it there. Short answer is it will be lumped in with pull request #1.

To clarify: I've mainly been maintaining branches for release and bower dependency purposes, generating compiled versions and reference branch git repository from bower.

As of this writing and to my knowledge, all outstanding changes discussed in this thread are covered by pull requests.

@levydev
Copy link

levydev commented Nov 24, 2014

Just curious:
Why am I seeing examples given on this thread , like this one (http://jsfiddle.net/iceye/ryb31tj1/), seeming to support the proper aspect ratio on outpu? But when use the same technique, in my app, the outputed image is still square. I've replaced my ng-img-crop js and css with the jfiddle example. The only other difference is that I am using ang-122.

@engineerchirag
Copy link

@iceye
Nice Work,
Waiting for "Set aspect ratio for cropping" jsfiddle example. Can you please build a working example on jsfiddle.

@engineerchirag
Copy link

@iceye ,
Using (http://jsfiddle.net/iceye/ryb31tj1/) i'm facing quality issue, while with ngImgCrop (original) , quality of image is quit well. Can anyone provide me the solution.

@levydev
Copy link

levydev commented Jun 27, 2015

You may find more recent developments here:

#1

@CrackerakiUA
Copy link

Checkout my fork: https://github.com/CrackerakiUA/ngImgCropFullExtended

@Sajgoniarz
Copy link

How is it going in official release?

@CrackerakiUA
Copy link

Official release don't look to need rectangle )

@humbertbeltrao
Copy link

Is area-min-size property working on rectangle area-type? I've tested, but only square area-type seems to be resizable.

@MishaVolinets
Copy link

Can you do minified version of this ngImgCrop http://jsfiddle.net/iceye/ryb31tj1/ ?

@khoramshahy
Copy link

hey I want to use rectangle but when I set area-type to rectangle that dosn't work and shows circle instead

I used exactly http://jsfiddle.net/iceye/ryb31tj1/1/ code and version of ngImgCrop is 0.3.2
I dont know whats the problem :(

@Xsmael
Copy link

Xsmael commented Apr 12, 2018

@khoramshahy

am having the same issue

@CrackerakiUA
Copy link

CrackerakiUA commented Apr 12, 2018

@Xsmael
https://github.com/CrackerakiUA/ui-cropper

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

No branches or pull requests