This effect provides the distorting of the proportions and shape of the image in visual perception.

Effect tries to support both the GD toolkit from Drupal core and the ImageMagick toolkit. However, please note that there may be effect that are not supported by all toolkits, or that provide different results with different toolkits.

We have developed a module that demonstrate the Perspective effect https://www.drupal.org/sandbox/lebster/2758383.

Could you review module and effect, then we add a patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lebster created an issue. See original summary.

lebster’s picture

dman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
318.73 KB

In visual review, the code looks high-class.
It looks like it took a lot of work to get the maths/algorithm correct there - but it's very readable. I do favour clarity over cleverness, and this is a winner.

Patch applied clean.

I know it's hard to describe or explain in words what is happening here, but the new UI and help text does a great job. The suggestions of 'good' numbers are especially helpful.

I tried it against both toolkits in my testsuite, and got identical results, so whatever is happening- works.

I didn't imagine a use/need for this at first, but seeing it in action... - I do guess someone will want it some time, and the code contrib is of such high quality, I'm happy to encourage it. I feel it can go right in!

I did find a glitch with the colorizing of the background fill (I tried a 'define canvas' after the perspective - worked great in gd (!) but not imagemagick), but that can be a follow up.

lebster’s picture

I attached some demo.

Before and after
non-perspective
perspective

Adjust the selected effect
settings

In order to avoid a black background, we apply effect "Change file format: convert to png".

fietserwin’s picture

Status: Reviewed & tested by the community » Needs work

Code is indeed high quality, nevertheless I found some remarks.

  1. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +  $data = array_merge($defaults, (array) $data);
    

    Cast to array is not necessary due to type checking.

  2. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +    '#size' => 2,
    +    '#maxlength' => 2,
    

    Why can't (shouldn't) I use a percentage like 16,66667 or 33,33333?

  3. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +    '#allow_negative' => FALSE,
    

    This is not used (appears 3 times).

  4. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +function image_gd_perspective(stdClass $image, array $data = array()) {
    

    Leave out the default value for the 2nd parameter: it must be (and will be) passed by toolkit invoke (Also for imagemagick function). (Furthermore: you assume that all keys will be available, which is correct, but that doesn't play nicely together with an empty array as default argument.)

  5. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +    case "bottom":
    

    Empty line above case (Drupal coding standard).

  6. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +      for ($y = $height; $y >= 0; $y--) {
    

    This loop is executed height + 1 times (while the loop in case 'top' is executed height times. Same for case 'right'.

  7. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +  $image->ops[] = '-matte -virtual-pixel transparent -distort Perspective \'' . $perspective_arg . '\'';
    

    On windows I get:

    Debug: ImageMagick command:
    
    start "ImageMagick" /D "D:\Projecten\Drupal7\test" /B "C:\Program Files\ImageMagick-6.8.9-Q16\convert.exe" "D:\Projecten\Drupal7\test\modules\image\sample.png" -matte -virtual-pixel transparent -distort Perspective '0,0,0,0 0,600,0,600 800,0,800,84 800,600,800,516' -quality 75 "png:...\sites\default\files\styles\perspective\public\modules\image/sample.png"
    
    in _imagemagick_convert_exec() (regel 495 van ...\sites\all\modules\imagemagick\imagemagick.module).
    Debug: ImageMagick error:
    
    convert.exe: invalid argument for option Affine : 'require at least 1 CPs' @ error/distort.c/GenerateCoefficients/530.
    convert.exe: unable to open image `0,600,0,600': No such file or directory @ error/blob.c/OpenBlob/2658.
    convert.exe: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501.
    convert.exe: unable to open image `800,0,800,84': No such file or directory @ error/blob.c/OpenBlob/2658.
    convert.exe: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501.
    convert.exe: unable to open image `800,600,800,516'': No such file or directory @ error/blob.c/OpenBlob/2658.
    convert.exe: no decode delegate for this image format `' @ error/constitute.c/ReadImage/501.
    
    in _imagemagick_convert_exec() (regel 500 van ...\sites\all\modules\imagemagick\imagemagick.module).

    Use escapeshellarg()

  8. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    +  $row = array();
    

    Warning: unused variable $row.

  9. +++ b/canvasactions/canvasactions.inc
    @@ -1247,3 +1247,424 @@ function image_imagemagick_interlace($image/*, array $data*/) {
    \ No newline at end of file
    diff --git a/canvasactions/imagecache_canvasactions.module b/canvasactions/imagecache_canvasactions.module
    

    Add a new line at end of file

lebster’s picture

Status: Needs work » Needs review
FileSize
8.68 KB
16.37 KB

Hi fietserwin,

We have resolved most of the comments listed above.

'#allow_negative' => FALSE,
It is used to avoid the possibility to set a negative value for the distortion parameters (distortion, opposite_distortion) and the conversion factor (aspect_coefficient).

Why can't (shouldn't) I use a percentage like 16,66667 or 33,33333?

By using these coefficients, there will be no visual difference. If necessary, we can add this.

fietserwin’s picture

Status: Needs review » Needs work

Thanks for updating the patch. We're almost there now, just the validation: #allow_negative is a property used by the function image_effect_integer_validate() (file image.admin.inc). As your validate function does not use it, nor call image_effect_integer_validate(), it is of no use. Moreover, as your validate functions already check for specific ranges (0-1 resp. 0-50) it wouldn't make sense to also call image_effect_integer_validate(), so we better remove the #allow_negative property.

With that change I will commit.

Regarding the accepted values for the distortion percentage: it indeed probably will not give you visible changes, but percentages are used at a lot of places in image effects and I prefer to keep the settings for all those fields the same as much as possible. But this point won't keep me from committing.

fietserwin’s picture

BTW: contrary to @dman, I did get (slightly) different results when using GD versus Imagemagick (both on Windows), but nothing serious and we do have other effects that give different results, so this is not something new or unique.

lebster’s picture

Status: Needs work » Needs review
FileSize
16.2 KB
1.92 KB

Fixed the range validation and percentages format.

fietserwin’s picture

Status: Needs review » Needs work

I was doing a last review before committing when I discovered that 'aspect_coefficient' is not used when applying the effect ??? Use it or remove it.

And some more minor points:
- When an effect canot determine the new dimensions it should return NULL for width and height in the Image dimensions callback function. So other callbacks should be prepared to handle that situation.
- The Image dimensions callback does not have to check whether all data values are there (e.g. the theme function also doesn't check). If a value is missing we are talking about corrupted data: it should produce warnings. (You may find checks like this in the existing code, but that should be because of additional effect values added in a new release where it thus may be expected that existing effect data in the database does not hold those values).

BTW: I saw that you also posted this to the D8 version of this module: good job! I will review over there as well when we have this code committed.

lebster’s picture

Fixed mentioned issues and attached the latest patch.

lebster’s picture

Status: Needs work » Needs review
lebster’s picture

We swapped the two variables for GD, when the vanishing point on the left. This error was noticed in related effect for Drupal 8 - https://www.drupal.org/node/2762627

fietserwin’s picture

Status: Needs review » Needs work
FileSize
16.75 KB

I still don't get how the aspect coefficient is used. You use it to set the 'width" and 'height' parameters, but merely setting these won't change the size of the image: it is the toolkit operations that actually change the dimensions. If you want to change the dimensions, you will have to use this parameter in the image_gd_perspective() and image_imagemagick_perspective() functions so that the toolkit will actually change the image dimensions.

I also:
- tried to improve the help texts. But as I am not a native English speaker either, maybe @dman can have a look at that when the code gets finalized.
- changed the order of the parameters to what seems more logical to me.

if you want to keep those changes, please continue with this patch.

lebster’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
17.04 KB

We have taken patch from the comment https://www.drupal.org/node/2760121#comment-11440451

Added image resizing for both toolkits.

We agree with the comment about the logical order of fields in the settings, and in accordance with the selected order, we have rearranged the variables in the list of values by default.

fietserwin’s picture

Status: Needs review » Needs work
FileSize
294.72 KB
382.08 KB
390.47 KB

Thanks for picking my changes up.

OK, now I see how the aspect coefficient is used. However, this is just a resize at the end of the effect and it doesn't influence the effect otherwise. To keep effects simple and "atomic" (they should do 1 thing and do that well), I propose to drop this parameter. Users can add a resize effect after this effect, like they can add a background canvas effect to define the background color or image of the unused part of the image.

If you want to keep it: the IM command extend does just set the canvas size, it does not resize but crop the image. So that would be a real error.

BTW: did you play with the multiplier, because 25 times the surface/memory usage of a picture from a recent portable phone will crash most configurations. I do see differences between 1 and 5 but hardly any between 3 or 5 (and 3 is "only" 9 times the memory). Perhaps make it a variable (without any UI for it).

Below a multiplier of resp. 1, 3 and 5:

lebster’s picture

Status: Needs work » Needs review
FileSize
40.74 KB
39.83 KB
38.45 KB
109.8 KB
126.22 KB
82.29 KB
38.92 KB
2.12 KB
17.27 KB

We have tested the multiplier parameter within GD toolkit.

We use the follow parameters:

Scale 220x220
Perspective transform asymmetrical. Vanishing point position: left. Top distort: 20%, bottom distort: 7%. Aspect coefficient: 0.9

test multiplier 1
test multiplier 3
test multiplier 5

In general, I agreed that 3 and 5 are not very different. Let's leave 3. We used a higher coefficient based on our requirements.

OK, now I see how the aspect coefficient is used. However, this is just a resize at the end of the effect and it doesn't influence the effect otherwise. To keep effects simple and "atomic" (they should do 1 thing and do that well), I propose to drop this parameter.

We should not abandon to use this parameter, or replace it using the resize effect after the perspective effect, because aspect_coefficient option is directly related to the transformation. In fact, when we look at the image at an angle, its apparent size smaller than actually (depending on the angle of view of any width or height). It is necessary to create a realistic effect, after transformation, we must set the option - as far as the dimensions of the image will be changed after this effect, so we adjust the angle from which the observer looks at the image.

Everything happens just as we see the opening door:

test aspect

Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.9
test aspect

Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.5
test aspect

Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.2
test aspect

fietserwin’s picture

FileSize
29.98 KB
23.81 KB

I decided to push this issue towards finalization, but based on my preferences:
- I removed the final resize part: imo this is better done in a separate subsequent effect
- I changed the background color to transparent white (remark @dman in comment #3)
- I changed a lot of the help texts. For me it is clearer, but this may not be so for everybody.
- Some code and documentation changes (e.g. 'string' instead of "string" where possible).
- I changed the IM command, -matte is kind of deprecated and I wanted more control then just setting the alpha channel (i.e: transparent white, for when the image type does not support transparency).
- Some more code and documentation changes not related to this patch but in the same file to get rid of all warnings in PhpStorm. This makes the patch and interdiff more difficult to read but it is to much trouble for me now to split it into 2 separate patches.

I intend to commit the patch as posted, if you cannot accept these changes and don't want to be credited anymore, please let me know. Otherwise you can let me know what name, organization and client I may mention on our project page.

fietserwin’s picture

lebster’s picture

Hi @fietserwin, sorry for late respond, I was not able to contribute a long time. Is it still actual request?

My name is Anton Lebedev, my company is CimpleO.

Should I do anything?

  • fietserwin committed abe2bfe on 7.x-1.x authored by lebster
    [#2760121] by lebster, fietserwin: Add a "Perspective" effect.
    
fietserwin’s picture

Title: Add an "Perspective" effect » Add a "Perspective" effect
Status: Needs review » Fixed

Thanks. I committed the version from #18 with some additional help notes about the missing resize.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.