alexk111 / ngImgCrop

Image Crop directive for AngularJS (THIS PROJECT IS NOT MAINTAINED ANYMORE)
MIT License
1.12k stars 518 forks source link

Add support for rectangle area crop #6

Open sanketsaurav opened 9 years ago

sanketsaurav commented 9 years ago

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

akempes commented 9 years ago

+1

chirgwin commented 9 years ago

This is implemented in pull request #1

paulflo150 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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

paulflo150 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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!

sanketsaurav commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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

divan commented 9 years ago

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

chirgwin commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@monad98

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

monad98 commented 9 years ago

@iceye awesome! :)

iceye commented 9 years ago

@monad98 Thx :)

@alexk111 Are you planning to merge this mod?

chirgwin commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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

chirgwin commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

engineerchirag commented 9 years ago

@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 commented 9 years ago

You may find more recent developments here:

https://github.com/alexk111/ngImgCrop/pull/1

CrackerakiUA commented 8 years ago

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

Sajgoniarz commented 8 years ago

How is it going in official release?

CrackerakiUA commented 8 years ago

Official release don't look to need rectangle )

humbertbeltrao commented 8 years ago

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

MishaVolinets commented 7 years ago

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

khoramshahy commented 7 years ago

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 commented 6 years ago

@khoramshahy

am having the same issue

CrackerakiUA commented 6 years ago

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