BUGFIX: fix bug with pixelSnapping on certain aspectRatios and image dimensions

[patricekaufmann]

With 8.2 the pixelSnapping feature got introduced #3065. We started using it in all our installations and I noticed a rare bug happening with specific combinations of image sizes and aspect ratio.

Since only cropArea.width is used for calculating the dimensions, it might happen that the resulting naturalCropHeight was bigger than the image itself, leading to errors when trying to save the image. I think it happened due to rounding in react-crop component.

The solution is made this way to keep ensuring correct aspect ratio values, incrementally subtracting the normalized aspect ratio values until the area is valid.

[crydotsnake]

How can i reproduce the described issue?

[patricekaufmann]

First you would need an ImageInterace property with the following editorOptions:

editorOptions:
  crop:
    aspectRatio:
     pixelSnapping: TRUE

The issue appears when editing the image inside the inspector, so that would need to be configured aswell.
Since this issue only appears on certain image dimensions and aspect ratios, I attach the following sample image which leads to an error in 2:3 aspect ratio: Sample Gradient JPG

The issue now occurs when moving the croparea on the ImageCropper without ever adjusting its size as demonstrated in the following video. It does not occur when not moving the area or resizing the area in addition to moving it, even dragging it to its maximum size.

G5FTdrdHg5.mp4

To refer to the actual error message: the crop area has an y value of -1 because the area is larger than the actual image.

Is that of any use to you so that you are able to review / test it properly? Otherwise let me know if there is anything else I can prepare.

[grebaldi]

Hi @patricekaufmann,

thanks a lot for your fix :) I was able to verify that it works as described.

👍

[mhsdesign]

Hmm with your image i can reproduce the bug. And also when creating an image with the same dimensions.

same ratio as yours:

different ratio:

also the editor behaves weird when trying to crop the 341 one ... (edit: its not actually 340,9 only my photo program told me that this were the dimensions after downscaling)

(when pixel Snapping =1 ) there is a little gap below:

---

if you like we can have a call about this so we can move faster forward ^^

[patricekaufmann]

if you like we can have a call about this so we can move faster forward ^^

I reached out to @mhsdesign and we decided upon calling about this sometime next week. Until the problem is resolved, we will disable the snapping.

[mhsdesign]

we should be sure that this while loop never causes the ui to freeze (eg. are negative values at any time possible? - if so we need to guard against this ...)

[patricekaufmann]

The assumption being made in the following statements is that imageHeight will be positive or 0.

The loop then terminates when one of the following conditions is met:

normalizedAspectRatioHeight > 0 or naturalCropHeight < 0

If we actually substitute the variables in naturalCropHeight variable declaration and ignore the floor function since it wouldn't change the sign , you eventually get a minimized form like

let naturalCropHeightWithoutFloor = cropArea.width * imageWidth * (currentAspectRatioStrategy.height /( 100 * currentAspectRatioStrategy.width)) (see Wolframalpha)

From that we can see that if either passed variable in crop configuration is negative, naturalCropHeight will be smaller than zero and the loop will never start.

If both configuration dimension variables are positive the aspectRatioGcd will be positive and therefore normalizedAspectRatioHeight will be positive, hence the loop with exit eventually.

If both are negative, naturalCropHeight will be greater than 0 and normalizedAspectRatioHeight will be smaller than 0. In that case the loop will not exit. Math.abs() on aspectRatioGcd would cover that edge case (I mean who passes negative values in the aspectRatio configuration ?)