non-square selections

[chirgwin]

@alexk111 For your consideration, I've made some changes to allow for (non-square) rectangular selections (which I plan on implementing soon, along with some tests). This mostly involved moving geometry getters/setters away from being center-point centric. Behavior of current circle and square selections should be identical.

I've also added a property for binding the image data (RGBa pixel array), since we have a need to further process pixels from the crop selection. The alternative would be to externally convert the existing imageURI back into an RGBa pixel array, which we could live with.

[leveler]

Just curious about this pull request. It would be really helpful.

[mladenplavsic]

I'm planning to use ngImgCrop and allowing selection other than square/circle would be great!

[alexk111]

Sorry for the delayed response. Thank you for the proposed feature! Tested and it didn't work for me. It produces the error on my side:

Error: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The source width is 0.
    at Error (native)
    at getResultImage (http://localhost:9000/compile/unminified/ng-img-crop.js:1008:57)
    at updateResultImage (http://localhost:9000/compile/unminified/ng-img-crop.js:1271:37)
    at http://localhost:9000/compile/unminified/ng-img-crop.js:1314:11
    at http://localhost:9000/compile/unminified/ng-img-crop.js:1291:15
    at k.$eval (http://localhost:9000/bower_components/angular/angular.min.js:112:319)
    at k.$apply (http://localhost:9000/bower_components/angular/angular.min.js:113:48)
    at http://localhost:9000/compile/unminified/ng-img-crop.js:1290:19
    at http://localhost:9000/bower_components/angular/angular.min.js:123:78
    at e (http://localhost:9000/bower_components/angular/angular.min.js:37:497) 

It shows the crop area, but doesn't allow to get the cropped image (produces that error each time I try to access it).

[chirgwin]

@alexk111 Looks like this bug is related to adding support for getting pixel array data (commit b919ac94e5), which should've been a separate branch/pull request. My mistake for conflating this with rectangular selections.

I wasn't able to reproduce locally the error you're seeing, so I've simply reverted that change. I'll re-apply this change on a new branch and issue a separate pull request for it.

I've also committed a test html page in 62614d5f. This is referenced in the gulp configuration, but the file itself seemed to be missing. This is copied from your example in the README.md file, with area-type changed to "rectangle".

[alexk111]

@chirgwin Thank you! I'll give it a try now.

[alexk111]

@chirgwin Why doesn't the result image have the same aspect ratio as the crop area? Why did you decide to move from the center-based area calculations to the corners' based ones?

Here are the bugs I've found:

• When I dynamically change the crop area type to rectangular, it does set type = square. Probably the aspect ratio is not applied for some reason.
• Every time I dynamically update the aspect ratio it does move the crop area to the top-left corner. The crop area center should always stay at the same place.
• Also getting 'GET unsafe:data:, net::ERR_UNKNOWN_URL_SCHEME ' errors on my test page. I'll investigate this later.

Thank you for letting me know about the missing test html page. Forgot to include it in the initial commit :) Will add it soon. A PR should only have one purpose. In this case - non-square areas (rectangular crop area). Please exclude the test page things from it.

[chirgwin]

Why doesn't the result image have the same aspect ratio as the crop area?

This requires getting the result dimensions-- covered separately in issue #4 and pull request #7-- and binding them to the width/height properties of the result image tag. See this fiddle for a working example.

Why did you decide to move from the center-based area calculations to the corners' based ones?

While center point calculations worked well for circles/squares, using the corners simplifies the geometry and rules when generalizing among rectangles, circles, and squares. If more detail on this would help, we could review specific commits.

Here are the bugs I've found:

I'll investigate these, too. Having a test suite would help get us on the same page with debugging (see more below). Also, maybe @sderungs wants to chime in on bugs related aspect ratio?

A PR should only have one purpose. In this case - non-square areas (rectangular crop area). Please exclude the test page things from it.

Understood and agreed. I've removed the test page. But, a rectangle-specific test case is relevant to this pull request. If you approve, I would like to include a simple one based on your tests. Alternatively, I could at least add an example for clarity (this would have answered your first question above). A proper automated test suite would help a great deal and save time. I'd gladly contribute to this (see #27).

[alexk111]

@chirgwin Thank you! I've just pushed the test page I'm using to master. Pull it to your fork/branch and play with the options to catch the bugs.

[alexk111]

@chirgwin The test page also has the rectangle type / aspect ratio options. Just uncomment the commented lines in the html.

[mtree]

@chirgwin @alexk111 hey! What's going on with rectangular selections? I REALLY need this feature in current project and I see working solutions here and there. Why still not merged into manline? Is there anything I can help you with guys?

[levydev]

Hello, I just came across this component and found it so intuitive and easy to use that I decided to replace my current solution. I'm almost there and a am now stuck on the rectangular pattern. Is this change easy to find in the pull requests? Thanks

[rubenjimenez]

Hi,

I'm using the ngImgCrop from the last fiddle published here and It seems to work fine excepting the image result, which is completely a square instead of a rectangle.

When do you think this could be fixed in some available version? I'm using ngImgCrop in a project and rectangle selected area seems to be a must for the client.

Thanks!

[alexk111]

@chirgwin Are there any updates on the bugs?

[mflatt23]

We would also appreciate any updates! Thanks for doing this.

[Capellaro]

+1

[axelav]

+1

[alexk111]

@chirgwin Any news? I'd like to add rectangular areas to the master branch till the end of the week. If there are any problems with it, please let me know - I'll solve it.

[chirgwin]

@alexk111 OK, I'll make some time for this soon. Please expect an update within 24 hours or so.

[mike-suggitt]

FYI I've discovered another bug in this branch when using aspect ratio. As it's driven by the user resizing X then programmatically adjusting Y accordingly, if the user drags Y out of the canvas it blows up. I've fixed it in my local repo but will paste it here for ease of inclusion:

File: crop-area

else if (areaType === "rectangle" && this._aspectRatio !== null) {
            var canvasH = this._ctx.canvas.height;
            var heightWithRatio = newSize.w / this._aspectRatio;

            if (heightWithRatio < canvasH && se.y < canvasH) {
                newSize.h = newSize.w / this._aspectRatio;
            }
            else {
                if((newSize.h * this._aspectRatio) <= canvasW) {
                    newSize.w = newSize.h * this._aspectRatio;
                }
                else {
                    newSize.h = newSize.w / this._aspectRatio;
                }
            }
        }

[levydev]

Hello, Sorry to pile on.
My site is due to go live in beta on Monday and I was wondering if there is a way to turn off the cropping as I have a set of users who will be uploading rectangular banners "as is' and with no need to crop. I know this sounds like a silly question but the banner upload control is sitting next to an avatar upload control that is working brilliantly with the circular cropping. So rather than removing the ngImgCrop for the banner and installing a different file upload mechanism, I'd like to simply turn off cropping and take the rectangular image as is.

Thanks

[mike-suggitt]

@levydev i'm a bit confused, why do you need to turn off cropping? surely there are a number of ways of "faking" the no crop option? Such as placing the image over the top of the hidden crop area? or setting the crop area to display: none? or simply not using the scope variable related to the outputted crop image?

[levydev]

Yes thank you. putting a watch on the selected file and then rendering the selected image instead of ngImgCrop widget - seems to do the trick.

[chirgwin]

@alexk111 I've spent some time running your test page against these changes. I'm committed to spending as much time as needed this week towards resolving all issues to your satisfaction. Specific responses follow.

A PR should only have one purpose. In this case - non-square areas (rectangular crop area). Please exclude the test page things from it.

I've taken your request on this one step further by reverting the downstream pull request for aspect ratio. This seems to be the source of a handful of bugs (see recent comments above), so I'm proposing that gets its own discrete pull request. It's my mistake for not testing thoroughly enough before merging the aspect ratio pull request.

When I dynamically change the crop area type to rectangular, it does set type = square. Probably the aspect ratio is not applied for some reason.

That's correct, and I think that's the expected behavior for a scalar value with a 1:1 aspect ratio. I've implemented this setter method(s) to be backwards compatible by either taking a scaler value (assumes 1:1 aspect ratio) or an object with both width and height properties. See also above comment on aspect ratio.

Every time I dynamically update the aspect ratio it does move the crop area to the top-left corner. The crop area center should always stay at the same place.

See my comment above on aspect ratio. I think this needs more work and testing. I've created a branch on my for fixed aspect ratios, where I'll review changes from @sderungs. Unless someone else beats me to it, once that's in a reasonable state, I'll issue another pull request specific to arbitrary fixed aspect ratios (#17).

Also getting 'GET unsafe:data:, net::ERR_UNKNOWN_URL_SCHEME ' errors on my test page. I'll investigate this later.

I think is is most likely the canvas being tainted by cross-origin data from a source image loaded by URL. Can you confirm this is not a problem when loading an image from the local filesystem?

[mike-suggitt]

Just FYI @chirgwin i'm looking at a bunch of "quirks" with the aspect ratio in my version. I'm not sure how easy/clean it's going to be placing the logic in the boundarycollision function so just looking at other possibilities.

[chirgwin]

@mike-suggitt Thanks. I've removed all remnants of setting or constraining an arbitrary aspect ratio from this pull request, now always assuming either a 1:1 aspect ratio or aspect ratio is implied by width/height value. Actually, it was your previous comment that prompted this. Once this pull request is closed, I'll be working on incorporating arbitrary aspect ratios (#17) separately, and I'll gladly accept some help working out the bugs.

[mike-suggitt]

@chirgwin Yea definitely, no problem. Ultimately I've looked to make the processMouseMove function in crop-area-rectangle do all the work. Almost all the problems I encountered were because the crop area was being set to new dimensions/positions regardless and then being "reverted/changed" where necessary. this just doesn't work as we have to backwards engineer x,y coordinates etc.

It's also allowed me to remove the crop-area resizing that was being done when dragging into the boundary (which was WAS REALLY annoying me! lol).

Happy to discuss stuff further in #17 if that's the best place for it.

[mike-suggitt]

oh but worth mentioning that rectangle crop area without aspect ratio works great! good work mate

[chirgwin]

@mike-suggitt Thanks. Yes, I made some similar changes towards making dimension constraints checking closer to user input, but sounds like you've gone a step further. And, yes, let's continue with aspect ratio in #17.

@alexk111 Any other bugs or problems you've identified that I should troubleshoot? I know you're looking to merge this pull request this week, and it's soon coming to an end.

[Smolations]

Out of curiosity, is there a separate implementation in the codebase for both squares and rectangles in this PR? I saw a comment earlier about type = square and thought, "There is no reason for a square type. A square is a rectangle." It seems to me that there should only be two types: circle and rectangle. To get a square crop area, the aspect ratio should just be 1:1. If this is already the way it is being implemented, then ignore my belated thoughts. =]

Thanks to both @alexk111 and @chirgwin for stepping up the velocity on this one. We all really appreciate it!

[mike-suggitt]

@Smolations If you're interested in the codebase I'd suggest forking the code and browsing through :)