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

Request for rotation props #47

Closed
ayepRahman opened this issue Jul 10, 2019 · 59 comments
Closed

Request for rotation props #47

ayepRahman opened this issue Jul 10, 2019 · 59 comments
Labels
enhancement New feature or request

Comments

@ayepRahman
Copy link

Are you making a feature request?

  • Is there a plan on making the image rotatable?
@ValentinH
Copy link
Owner

ValentinH commented Jul 10, 2019

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
Copy link

junbong commented Jul 10, 2019

I like this idea.

@ValentinH
Copy link
Owner

ValentinH commented Jul 12, 2019

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
Copy link
Author

ayepRahman commented Jul 12, 2019 via email

@ValentinH
Copy link
Owner

I edited my previous answer.

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

Any thoughts regarding UI?

@ValentinH
Copy link
Owner

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
Copy link
Author

ayepRahman commented Jul 12, 2019 via email

@ValentinH
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

ValentinH commented Jul 12, 2019

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).

@ValentinH ValentinH added the enhancement New feature or request label Jul 12, 2019
@ayepRahman
Copy link
Author

ayepRahman commented Jul 12, 2019 via email

@ValentinH
Copy link
Owner

@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
Copy link

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
Copy link
Owner

ValentinH commented Jul 19, 2019

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:

  • is the data exported by the lib enough to actually crop/rotate the original image? The croppedAreaPixels data might be wrong when rotation is different than 0.
  • the "show output" demo should handled the rotation.
  • what about edges detection? Right now, it's acting like the rotation is 0 which might not be the right approach.

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
Copy link
Contributor

MattyBalaam commented Aug 16, 2019

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
Copy link
Owner

@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
Copy link
Contributor

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

@MattyBalaam
Copy link
Contributor

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
Copy link
Owner

When do you translate the image if needed?

@MattyBalaam
Copy link
Contributor

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
Copy link
Contributor

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

@ValentinH
Copy link
Owner

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
Copy link

mbalaam commented Aug 19, 2019

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
Copy link
Owner

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
Copy link

mbalaam commented Aug 19, 2019

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

@ValentinH
Copy link
Owner

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
Copy link

mbalaam commented Aug 19, 2019

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

@ValentinH
Copy link
Owner

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

@mbalaam
Copy link

mbalaam commented Aug 19, 2019

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

@ValentinH
Copy link
Owner

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
Copy link
Owner

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

@marvwhere
Copy link

marvwhere commented Sep 12, 2019

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
Copy link
Owner

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
Copy link

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
Copy link

mbalaam commented Sep 12, 2019

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
Copy link
Owner

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
Copy link

mbalaam commented Sep 12, 2019

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
Copy link
Owner

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
Copy link
Owner

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

@MattyBalaam
Copy link
Contributor

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

@ValentinH
Copy link
Owner

If you want to have a look: #63

@ValentinH
Copy link
Owner

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
Copy link

mbalaam commented Oct 7, 2019

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
Copy link
Owner

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

@mbalaam
Copy link

mbalaam commented Oct 7, 2019

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
Copy link
Owner

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

@mbalaam
Copy link

mbalaam commented Oct 7, 2019

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

@mbalaam
Copy link

mbalaam commented Oct 7, 2019

Fingers crossed it will work this time…

https://codesandbox.io/embed/react-easy-crop-demo-with-cropped-output-o87hi

just in case though…


  const safeArea = Math.max(image.width, image.height) * 2

  // set each dimensions to double largest dimension to allow for a safe area for the
  // image to rotate in without being clipped by canvas context
  canvas.width = safeArea
  canvas.height = safeArea

  // translate canvas context to a central location on image to allow rotating around the center.
  ctx.translate(safeArea / 2, safeArea / 2)
  ctx.rotate(getRadianAngle(rotation))
  ctx.translate(-safeArea / 2, -safeArea / 2)

  // draw rotated image and store data.
  ctx.drawImage(
    image,
    safeArea / 2 - image.width * 0.5,
    safeArea / 2 - image.height * 0.5
  )
  const data = ctx.getImageData(0, 0, safeArea, safeArea)

  // set canvas width to final desired crop size - this will clear existing context
  canvas.width = pixelCrop.width
  canvas.height = pixelCrop.height

  // paste generated rotate image with correct offsets for x,y crop values.
  ctx.putImageData(
    data,
    0 - safeArea / 2 + image.width * 0.5 - pixelCrop.x,
    0 - safeArea / 2 + image.height * 0.5 - pixelCrop.y
  )

@ValentinH
Copy link
Owner

Indeed, it looks much better now! Would you like to update the website demo code? I'll update the sandboxes :)

@ValentinH
Copy link
Owner

@all-contributors please add @mbalaam for doc

@allcontributors
Copy link
Contributor

@ValentinH

I've put up a pull request to add @mbalaam! 🎉

@StefanZivkovic
Copy link

Hi guys,

I see you have rotation slider feature. Do you have built-in button-icon for left/right 90 degrees rotation? If not, any known suggestions how to implement it?

@ValentinH
Copy link
Owner

ValentinH commented Apr 29, 2020

Hi,

No there is no built-in feature for this but you can achieve it quite easily. Here's an example: https://codesandbox.io/s/react-easy-crop-demo-with-cropped-output-6skpi

@StefanZivkovic
Copy link

Ok,

Good, i implemented same way, but was curious if there are some other Cropper related props to use for this purposes.

Thank you @ValentinH for effort and quick response.

@shiva15102012
Copy link

Hi,

I went through the entire code and it's working fine in chrome browser but in the safari browser it's showing only a small area of the image even if I manually updated with initialCroppedAreaPixels.

And one more issue if we open this code in ios device safari and showing the black screen always if we upload the large size of image so I fixed the size issue with compress code but initialCroppedAreaPixels is always small when I opened in the ios safari browser.

Can some help me to fix the issue with initialCroppedAreaPixels in ios safari browser

@ValentinH
Copy link
Owner

Please open a new issue with a reproduction example. I don't see what's the link with this issue.

Repository owner locked as resolved and limited conversation to collaborators Sep 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants