Skip to content

Conversation

@trurl-master
Copy link
Contributor

@trurl-master trurl-master commented Nov 20, 2021

Fixes #240

Motivation

  • Currently, the croppedArea returned by onCropComplete when used together with non-zero rotation is not very useful, the values are more like temporary variables necessary for subsequent calculation of croppedAreaPixels and do not make much sense on their own. I wanted to change that and unify the output of croppedArea and croppedAreaPixels when used with rotation and without
  • If we want to restore previously stored cropArea we have to use initialCroppedAreaPixels. The problem with pixels is that the process that generates them also post-processes them (recalculates based on passed aspect and rounds to the nearest integer), which results in slowly drifting crop zone when saved -> restored multiple times

This solution

  • From now on the croppedArea return values from 0 to 100 (unless restrictPosition is false). It behaves the same as before when rotation is 0˚, but when it's not 0˚ the percentages are calculated within the bounding box size of the rotated image. This approach greatly simplifies crop operations, for sharp for example, it's extremely easy to just plug the values into rotate(), then extract() to get the correct result. (Breaking change)
  • The croppedAreaPixels is also changed, it also returns values within the bounding box of the rotated image with improved accuracy and rounded to the nearest integer (as before) (Breaking change)
  • Added new prop initialCroppedAreaPercentages that uses the exact inverse algorithm that generates croppedArea. It minimizes the losses of save/restore operations. They're still present on some occasions as the JS floating-point operations accuracy is limited, but it allows to preserve the crop for much longer. initialCroppedAreaPixels still can be used, but it's a bit risky

A PoC of server-side crop using sharp - https://codesandbox.io/s/test-easy-crop-next-e51oi
Official example with client-side crop remade for this PR - https://codesandbox.io/s/react-easy-crop-demo-with-cropped-output-forked-z0cgp

Migration guide

The main impact this PR has is on the croppedAreaPixels when used together with the rotation. As it now generates bounding box relative values, you will need to remake the part that generates the final image, taking that into account.

Here's an example implementation (codesandbox):

First, calculate the bounding box size:

const image = await createImage(...)

const rotRad = getRadianAngle(rotation)
const bBoxWidth =
  Math.abs(Math.cos(rotRad) * image.width) +
  Math.abs(Math.sin(rotRad) * image.height)
const bBoxHeight =
  Math.abs(Math.sin(rotRad) * image.width) +
  Math.abs(Math.cos(rotRad) * image.height)

Second, create a canvas element with the size of the bounding box:

const canvas = document.createElement('canvas')

canvas.width = bBoxWidth
canvas.height = bBoxHeight

Then, rotate the canvas context around the center of the canvas:

const ctx = canvas.getContext('2d')

ctx.translate(bBoxWidth / 2, bBoxHeight / 2)
ctx.rotate(rotRad)

Set context to point to the top-left corner of the rotated image:

ctx.translate(-image.width / 2, -image.height / 2)

Draw the image onto the rotated context:

ctx.drawImage(image, 0, 0)

What you have in the canvas at this point is a rotated image inside its bounding box. Now, you just need to extract the result using croppedAreaPixels and repaint it on a canvas with the size of the final image:

const data = ctx.getImageData(
  croppedAreaPixels.x,
  croppedAreaPixels.y,
  croppedAreaPixels.width,
  croppedAreaPixels.height
)

// set the canvas size to the final image size - this will clear existing context
canvas.width = croppedAreaPixels.width
canvas.height = croppedAreaPixels.height

// paste the extracted image at the top left corner
ctx.putImageData(data, 0, 0)

croppedArea

If you somehow managed to use croppedArea with rotation before - you're smarter than me. If not - now is the time. The approach is basically the same as with croppedAreaPixels - you just need to convert percentages to pixels based on the image you're cropping

Here's how you can use percentages server-side and crop using sharp (codesandbox):

Rotate the image:

const image = sharp(...);

await image.rotate(rotation);

Get rotated image bounding box size (basically you need to create a new image out of the rotated one, otherwise the metadata won't change):

const meta = await sharp(await image.toBuffer()).metadata();

Alternatively, you can calculate the bounding box size using the same method we used for the client-side crop

Convert percentages to pixels:

const cropInfo = {
  top: Math.round((croppedArea.y / 100) * meta.height),
  left: Math.round((croppedArea.x / 100) * meta.width),
  width: Math.round((croppedArea.width / 100) * meta.width),
  height: Math.round((croppedArea.height / 100) * meta.height)
}

Extract the image:

const myCroppedImage = await image.extract(cropInfo)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 64f2d91:

Sandbox Source
react-easy-crop Configuration
test-easy-crop-next PR
react-easy-crop demo with cropped output (forked) PR
react-easy-crop demo with cropped output (forked) PR
test-easy-crop-next PR
wizardly-mccarthy-ynuie Issue #240

@trurl-master trurl-master force-pushed the rework-rotated-croparea branch 3 times, most recently from f5737c0 to 7ce1770 Compare November 20, 2021 22:47
@trurl-master trurl-master force-pushed the rework-rotated-croparea branch from 7ce1770 to ff21a02 Compare November 21, 2021 00:21
@ValentinH
Copy link
Owner

Thanks a lot for this PR and the effort! 🙂

I quickly had a look to the changes from my phone but I'll do a more extensive review of the code this week.

I have a few remarks/suggestions already:

  • having to change most e2e tests doesn't sound right, especially for tests not involving rotation. What is the reason of these changes?
  • could you please update the cropping logic of the demo website (docs folder) in order to see the actual diff required by these changes? You will need to change the imported library from the latest NPM to a relative path pointing to the src folder.
  • in the description of the PR, could you please prepare migration instructions for the breaking changes that we will use when publishing the new major version?

@trurl-master
Copy link
Contributor Author

trurl-master commented Nov 21, 2021

Thanks a lot for this PR and the effort! 🙂

Thank you (and the rest of contributors) for the lib :)

  • having to change most e2e tests doesn't sound right, especially for tests not involving rotation. What is the reason of these changes?

The reason for these changes is that to calculate rendered media size, the solution was to use offsetWidth and offsetHeight. The problem with them is that they report rounded values (contrary to the spec, both firefox and chrome tried to change that but decided not to because it breaks a lot of sites). I can't use getBoundingClientRect because it takes transforms into account (unless I render a separate image without rotation just to calculate the width/height). Instead, I'm calculating the media size manually using the container size (taken with getBoundingClientRect) and the fact that the image is placed inside using max-width: 100%; max-height: 100%. This move greatly improved the accuracy of crop/zoom restoration. But the side-effect was that the actual CSS values aren't pretty anymore, thus all these changes.

You are correct in saying that it doesn't sound right to change all the e2e tests, but the reason for this in my view is the fact that these tests test implementation details instead of actual results. What matters to the user is what the lib outputs - in our case it's croppedArea and croppedAreaPixels. This is what should be tested. In this case, the tests wouldn't have been affected (or at least affected much less). Otherwise, every time you make changes to math inside the project, you will end up with "having to change most e2e tests doesn't sound right" moments.

  • could you please update the cropping logic of the demo website (docs folder) in order to see the actual diff required by these changes? You will need to change the imported library from the latest NPM to a relative path pointing to the src folder.

Ah! Sorry, didn't notice it there, thought it was somewhere else. Updated!

  • in the description of the PR, could you please prepare migration instructions for the breaking changes that we will use when publishing the new major version?

Will do

@ValentinH
Copy link
Owner

I completely agree that current e2e tests are not very valuable. I think that we should replace most of them with visual regression tests as what matters is the visual output.
I will check if the output is actually still the same when testing from the computer.

Regarding using the container size, did you check that it works with all possible values of the objectFit property? As the cover ones don't use 100% for max width and height I'm scared it might bring issues.

@trurl-master
Copy link
Contributor Author

trurl-master commented Nov 21, 2021

Ah! I didn't, that will probably break, I'll fix it asap. Thank you

@trurl-master
Copy link
Contributor Author

trurl-master commented Nov 21, 2021

I completely agree that current e2e tests are not very valuable. I think that we should replace most of them with visual regression tests as what matters is the visual output

Yeah, maybe like Chromatic or something similar, but I think just to test croppedArea (to a reasonable accuracy) and croppedAreaPixels instead of CSS is a good start

@trurl-master
Copy link
Contributor Author

objectFit should be supported properly now

Copy link
Owner

@ValentinH ValentinH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a few suggestions and reported a few small issues.

Except for this, the PR looks great to me! To be honest, I think this is one of the most advanced and detailed PR I've received on this repository, congrats! 🎉 👏

Thank you very much for this PR and for adding a detailed migration guide + a nice example with sharp (this one should be added to the official list of example once this is merged)! 😍

@trurl-master trurl-master force-pushed the rework-rotated-croparea branch from 7cb6deb to 8a63c1e Compare November 29, 2021 09:41
@ValentinH
Copy link
Owner

I published an alpha version with your branch: 4.0.0-alpha.0. I'll do a few other tests before merging and releasing 🤞

@ValentinH ValentinH merged commit ff7c805 into ValentinH:main Nov 29, 2021
@ValentinH
Copy link
Owner

This was released as v4.0.0: https://github.com/ricardo-ch/react-easy-crop/releases/tag/v4.0.0

Thank you very much @trurl-master for the awesome work 👏

ValentinH added a commit that referenced this pull request Nov 29, 2021
Example taken  from  #312
@ValentinH
Copy link
Owner

@all-contributors please add @trurl-master for code and examples

@allcontributors
Copy link
Contributor

@ValentinH

I've put up a pull request to add @trurl-master! 🎉

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

Successfully merging this pull request may close these issues.

After rotating a rectangular image, croppedAreaPixel range can be negative

2 participants