Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2760121.patch | 23.81 KB | fietserwin |
#18 | interdiff.txt | 29.98 KB | fietserwin |
Comments
Comment #2
lebster CreditAttribution: lebster at CimpleO commentedComment #3
dman CreditAttribution: dman as a volunteer commentedIn 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.
Comment #4
lebster CreditAttribution: lebster at CimpleO commentedI attached some demo.
Before and after
Adjust the selected effect
In order to avoid a black background, we apply effect "Change file format: convert to png".
Comment #5
fietserwinCode is indeed high quality, nevertheless I found some remarks.
Cast to array is not necessary due to type checking.
Why can't (shouldn't) I use a percentage like 16,66667 or 33,33333?
This is not used (appears 3 times).
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.)
Empty line above case (Drupal coding standard).
This loop is executed height + 1 times (while the loop in case 'top' is executed height times. Same for case 'right'.
On windows I get:
Use escapeshellarg()
Warning: unused variable $row.
Add a new line at end of file
Comment #6
lebster CreditAttribution: lebster at CimpleO commentedHi 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).
By using these coefficients, there will be no visual difference. If necessary, we can add this.
Comment #7
fietserwinThanks 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.
Comment #8
fietserwinBTW: 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.
Comment #9
lebster CreditAttribution: lebster at CimpleO commentedFixed the range validation and percentages format.
Comment #10
fietserwinI 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.
Comment #11
lebster CreditAttribution: lebster at CimpleO commentedFixed mentioned issues and attached the latest patch.
Comment #12
lebster CreditAttribution: lebster at CimpleO commentedComment #13
lebster CreditAttribution: lebster at CimpleO commentedWe 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
Comment #14
fietserwinI 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.
Comment #15
lebster CreditAttribution: lebster at CimpleO commentedWe 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.
Comment #16
fietserwinThanks 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:
Comment #17
lebster CreditAttribution: lebster at CimpleO commentedWe have tested the multiplier parameter within GD toolkit.
We use the follow parameters:
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.
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:
Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.9
Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.5
Perspective transform symmetrical. Vanishing point position: left. Distort: 10%. Aspect coefficient: 0.2
Comment #18
fietserwinI 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.
Comment #19
fietserwinComment #20
fietserwinComment #21
lebster CreditAttribution: lebster at CimpleO commentedHi @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?
Comment #23
fietserwinThanks. I committed the version from #18 with some additional help notes about the missing resize.