ValentinH / react-easy-crop

A React component to crop images/videos with easy interactions
https://valentinh.github.io/react-easy-crop/
MIT License
2.32k stars 166 forks source link

Request for rotation props #47

Closed ayepRahman closed 5 years ago

ayepRahman commented 5 years ago

Are you making a feature request?

ValentinH commented 5 years ago

I've never thought about this but I don't think so. On ricardo.ch, we have a simple button to rotate the image by 90°.

junbong commented 5 years ago

I like this idea.

ValentinH commented 5 years ago

How would you see this feature? What would be the UI?

For mobile, it would be quite intuitive by simply using 2 fingers. However, the biggest question is how to provide a simple interaction on desktop?

ayepRahman commented 5 years ago

Probably pass props of integer in degree 90 etc

On Fri, Jul 12, 2019 at 1:37 PM Valentin Hervieu notifications@github.com wrote:

How would you see this feature? What would be the UI?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ricardo-ch/react-easy-crop/issues/47?email_source=notifications&email_token=AGJJD5H5CXPTUR7TXZJL77LP7AKARA5CNFSM4H7L6YM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZYXMUI#issuecomment-510752337, or mute the thread https://github.com/notifications/unsubscribe-auth/AGJJD5BW7ONEXRU55Z5QYHDP7AKARANCNFSM4H7L6YMQ .

ValentinH commented 5 years ago

I edited my previous answer.

The API would be quite simple indeed: rotation: number and onRotationChange: number => void.

Any thoughts regarding UI?

ValentinH commented 5 years ago

Actually it could be started with only an external UI for desktop (like a slider), and handle the rotation on mobile within the library.

ayepRahman commented 5 years ago

I like using components from Mui. But anyone can use any JSX element with onClick function to handle the roration. Even slider from Mui which previosuly you shown. Maybe you can ref the edit photo in LinkedIn?

ValentinH commented 5 years ago

OK, for now, I'll add support for the rotation prop, this is easy. I'll see later if we should add mobile gesture to handle this within the component 🙂

ValentinH commented 5 years ago

Wow, I didn't see this LinkedIn feature, the cropper element looks exactly like this one! I will do a demo to show how to build this UI with react-easy-crop😎

image

ValentinH commented 5 years ago

Support added for rotation prop in v1.14.0: https://github.com/ricardo-ch/react-easy-crop/releases/tag/v1.14.0 🎉 I've updated the website demo: https://ricardo-ch.github.io/react-easy-crop/ 🙂 (I still have to add support for the rotation in the "Show results" popup).

ayepRahman commented 5 years ago

Woah that was fast. Great job Valentin!

On Fri, Jul 12, 2019 at 2:33 PM Valentin Hervieu notifications@github.com wrote:

Support added for rotation prop in v1.14.0: https://github.com/ricardo-ch/react-easy-crop/releases/tag/v1.14.0 🎉 I've updated the website demo: https://ricardo-ch.github.io/react-easy-crop/ 🙂

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ricardo-ch/react-easy-crop/issues/47?email_source=notifications&email_token=AGJJD5HLKN6R5MPGSI7SEHTP7AQVPA5CNFSM4H7L6YM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZY2RMQ#issuecomment-510765234, or mute the thread https://github.com/notifications/unsubscribe-auth/AGJJD5EMU7QVHKQEQ3ELA2TP7AQVPANCNFSM4H7L6YMQ .

ValentinH commented 5 years ago

@ayepRahman supporting the rotation in the "Show results" popup is more tricky than expected. For now, I've removed it from the demo and marked the rotation as "experimental" in the ReadMe. Did you actually managed to use this new rotation prop? If yes, do you then generate the output image within the browser (canvas)?

rafaelsorto commented 5 years ago

This is amazing, I was about to fork the repo and see if I could get something like rotation rolling. I'll test the one that you added and come back with comments. Thank you for adding something like this. It's quite expected nowadays to just do pinch and zoom/rotate on mobile.

ValentinH commented 5 years ago

For now, we don't support rotating the image with gestures. I'd like to first ensure that the way rotation is handled now is correct:

Any help is welcome here because as I don't use the rotation myself, I might not be the right person to define those requirements.

Once all the points above are addressed, it should not be a big deal to support the gesture.

MattyBalaam commented 5 years ago

I’ve been having a little play with the rotation, but I’m hitting a wall trying to convert the co-ordinates to something that canvas will translate accurately.

If the crop is dead-center then using this kind of technique appears to work fine:

     context.translate(
        croppedAreaPixels.width / 2,
        croppedAreaPixels.height / 2,
      );

      context.rotate(getRadianAngle(rotation));

      context.translate(
        -croppedAreaPixels.width / 2,
        -croppedAreaPixels.height / 2,
      );

    context.drawImage(
      image,
      croppedAreaPixels.x,
      croppedAreaPixels.y,
      image.width,
      image.height,
      0,
      0,
      image.width, 
      image.height,
    );

However once the image is moved away from a central position then the image is moved in the wrong position and I just don’t have the knowledge about how to translating these x/y values.

ValentinH commented 5 years ago

@MattyBalaam Indeed, I faced the same issue. Everything goes wrong once the image is moved from the center. Also the edge detection is broken.

MattyBalaam commented 5 years ago

I’ll invest some time next week investigating this a bit further.

MattyBalaam commented 5 years ago

Thinking out loud... I wonder if a way to do it might be for having a canvas the size of the original image, the context to the rotated, then the image applied without cropping. The initial context should then be restored and then the result of this can then be cropped.

ValentinH commented 5 years ago

When do you translate the image if needed?

MattyBalaam commented 5 years ago

So after the rotation is applied the image is drawn with scaling, but not cropped. The cropping would be done afterwards. I'm thinking out loud without a laptop so this might not work how I imagine. But my thinking is that doing it this way will simulate more what is happening in the css transformation.

MattyBalaam commented 5 years ago

There is no doubt a more mathematical algorithm approach to doing this of course.

ValentinH commented 5 years ago

This is my main issue, I'm too lazy to do proper maths. Most of this library mathematical computations have been implemented by bruteforcing some formulas! 😃

mbalaam commented 5 years ago

So, I can confirm that this approach creates a crop that matches the preview. However at the moment as you mention the problem is that the edge detection is broken.

I’ll for the code sandbox with some working code later today.

ValentinH commented 5 years ago

This is awesome, it would be really nice to have a working example with the image export when rotation! We can deal with the edge detection later.

mbalaam commented 5 years ago

Here is a forked demo with rotation preview: https://codesandbox.io/s/react-easy-crop-demo-with-cropped-output-mlwif?fontsize=14

ValentinH commented 5 years ago

MAN!

genius

This is so cool! Would you like to make a PR on the demo website to showcase this? Basically, this block should be uncommented, and you rotateImage() should be added.

Just one question, do you think there is a way to do the rotation and the crop/translate on the same canvas so that we only generate one image?

mbalaam commented 5 years ago

I’ve had a rethink, and here is an improved v2: https://codesandbox.io/s/react-easy-crop-demo-with-cropped-output-pd4v5

ValentinH commented 5 years ago

The code looks great! However, something might have been wrong because the output modal doesn't open anymore.

mbalaam commented 5 years ago

In the PR? I should have run it, I’ll check

ValentinH commented 5 years ago

No I meant in the sandbox

mbalaam commented 5 years ago

It’s working here. I’ve resaved it just in case.

ValentinH commented 5 years ago

Yes nevermind, it was due to my browser (Brave) tracking prevention.

ValentinH commented 5 years ago

Thanks to @mbalaam, the demo now supports the rotation props! 🎉

This mean that the only last remaining thing in the initial todo list is the edge detection. I think we can already remove the "experimental" flag from the readme as this is not that bad.

ValentinH commented 5 years ago

I'm closing this issue now. I have opened #52 for the edge detection.

marvwhere commented 5 years ago

Question: The "Show Image" is still not showing the right position, if you rotate and leave the center, right?

Or let it phrase the other way around: The croppedAreaPixels are totally off? Because when a landscape image and then it fits in the total width, x should always be 0 ?

I tested with the above v2 codesandbox

Is there any solution for it?

ValentinH commented 5 years ago

Indeed, the sandbox was not updated, only the demo on the website: https://ricardo-ch.github.io/react-easy-crop/ (source: https://github.com/ricardo-ch/react-easy-crop/tree/master/docs/src/components/Demo) I need to add another sandbox that uses the right code. 🙂

marvwhere commented 5 years ago

The Demo is giving out the wrong cutout too.

Reproduce: Rotate image by 270deg, move crop to total bottom. Click Show Result (Chrome OSX)

mbalaam commented 5 years ago

OK, seems to be wrong in certain cases… Could this be perhaps down to the changes I made with resizing the crop area?

@marvwhere can you confirm that at angles like 45 degrees the crop is correct?

ValentinH commented 5 years ago

Hmm this is weird, I was convinced that we had all the cases covered in the demo but this is clearly not case. It seems to be wrong when the image is translated to an edge with any rotation.

See: Screenshot_20190912-052850

@mbalaam do you think the demo was not updated to your latest example? I thought this was working in your example.

mbalaam commented 5 years ago

I could be that a change I made for 1.15.0 has broken the preview?

I’ll take another look over the next day or so.

ValentinH commented 5 years ago

Yes it might be. Maybe the way the output croppedArea has changed 🤔

I'm also coming back from holiday over this weekend, so I'll also investigate if needed.

ValentinH commented 5 years ago

I've found and fixed the issue locally. I'm adding some tests and publishing a fix soon 🎉

MattyBalaam commented 5 years ago

That's awesome news. Been really busy over the last couple of weeks and unable to find the time to investigate.

ValentinH commented 5 years ago

If you want to have a look: #63

ValentinH commented 5 years ago

A fix has been published under 1.16.1. I've also updated the demo website and this sandbox: https://codesandbox.io/s/q8q1mnr01w.

mbalaam commented 5 years ago

I’ve found some time to look at this as the demo was still showing the issue.

Good news is I think I know how to fix the canvas code and will open a PR. The clipping is happening because even though I am rotating on a canvas twice the size of the original image, once the crop is rotated around 90 degrees and then moved to an extreme edge the image is still clipped.

ValentinH commented 5 years ago

Indeed, in some really edge cases, a small part of the image can be clipped. 🙂

mbalaam commented 5 years ago

OK, I’m now generating the 'safe area' for the rotation based around the maximum width or height, so as far as I can tell the clipping issue is fixed now. Does anyone fancy giving it a battle test?

https://o87hi.csb.app/

ValentinH commented 5 years ago

I only see a black output when clicking the "Show results" button 😄

mbalaam commented 5 years ago

Hmmm, code sandbox doesn’t seem to have the right version saved going back to it… I’m going to need to work it out again