-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add strokeWidth and strokeColor support. #1301
Conversation
Added Text Stroke support that I used on my project. Fully tested on PHP 8.1 Supports both GD and Imagick drivers. Stroke is on demand, not added by default. FontInterface updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Biggest thanks to you, for this great library - it's great help, very easy to use and fun to work with to generate images! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes committed and public method changed to stroke( $width, $color ) as suggested, please review.
Great. I have a few more thoughts. With GD no outline function is natively supported, so in PR the text is written with a kind of "compound function" which writes the outline with a 3x3 matrix behind the actual text. This is perfectly ok, as the result works very well. However, I see two things that need to be considered. 1. Driver differencesWith GD and the method described above, the text is given an outline which is outside the actual text. See example: With Imagick, however, the result (with exactly the same code) looks different and the outline is drawn in the middle of the text shape. Unfortunately, this cannot be changed. I have already looked to see if Imagick has an option to draw the outline outside. There doesn't seem to be an option here. I am thinking about not using the native Imagick function here, but also using the "compound method", which is already used in the GD driver. The results would be the same. What do you think? 2. Higher values for the stroke widthAs the GD driver draws the outline with a trick, higher values have the effect that no outline is drawn, but the 3x3 matrix becomes visible. Example with font-size This effect depends on the ratio of the font size and the stroke width. A quick test of mine showed that 1/5 of the font size produces an acceptable result. I'm thinking about adding a limit here and throwing an exception for stroke widths that exceed this value. What do you think? |
Agree, these are very valid concerns.
This is certainly a solution when goal is to achieve Imagick outline instead of inline. To be consistent with both drivers. In regards limiting the stroke width, it seems that it is necessary.. however, unfortunately in my tests not 1/5 (20%) but lower value (~13%) is producing acceptable result, please see the test case below. What do you think? I would make changes later today to limit max stroke width as well as Imagick similar compound function. Test Case:
|
The more I think about it, the more I think this is the right way to go to let both drivers work with the same method for the moment. It would be great if you could implement this.
I just did some more tests with different TTF fonts and realized that the character of the font also plays a role. With very bold fonts (I used "Impact" in my first example), a limit of 20% is sufficient. The narrower the silhouette of the font, the smaller the limit must be to avoid the unwanted effect. With Mooli Regular, for example, the stroke width may only be around 6-7% for a font size of It should be noted that very low outline values such as If you also consider that the "Mooli example" only works with 6% at
Great. I'm looking forward to it. |
I agree, probably percentage is not reliable due unknown font characters. Made the above mentioned changes as promised. Those are:
Please review and let me know your thoughts. |
Thank you for your ideas. @maximuslight. I understand that you want to keep things flexible. I think it is better to use the limit only internally in a fixed form and not make it changeable via the public API. I'm afraid that in its current form this could cause confusion and I don't like the idea of a parameter value whose range can be overridden by another parameter value. I believe that very large stroke widths only work under very specific circumstances anyway (Bold Font & Large Font Size), so I think it's okay to only support a certain range for this library. I even briefly considered supporting only a width of 1, as even a small value like 5 would have an unwanted effect (see example below). Not sure if this is a good idea though. |
I removed settable limit so method remains simple:
Have improved the compound method, basically I looked up that one what others did in such scenario, including Goldengide. They run X,Y in all directions as many as strokeWidth needs, I placed hardcoded limit to 10 out of resource/memory considerations. Please see test cases with Imagick Driver: Gd Driver: Please let me know your thoughts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works very well now! I think that this can basically be adopted.
Before the merge I have a few minor change requests.
- Implementation of the changes in the respective commented lines of code
- Overall the code standards must also be adhered to. These are basically PSR12 with some additional defined rules.
Please call the following command and correct the corresponding "errors".
php ./vendor/bin/phpstan.phar analyze --memory-limit=512M --level=4 ./src && php ./vendor/bin/phpcs
Thank you for your commitment and patience. 👊
Co-authored-by: Oliver Vogel <oliver@olivervogel.com>
Changes requested are made. Ran
|
Thank you very much.
Thats strange. I get 4 issues here. In the GitHub workflow action they are also visible.
This is even stranger, because your screenshot shows errors in files that you have not edited by you at all. The errors from your screenshot are also not reported in the GitHub action. Only the files that were edited in the PR are problematic here. These should be fixed, I will then take care of the errors of the static analyzer.
|
Thank you! Just to assist a little in understanding why there are different in messages: This is likely due my inexperience with pull requests/contribution and perhaps differences in our set ups. I have a live project (and dev clone of it, where all happens) on the Debian server that I use for battle-testing changes and when everything is good, committing to Github. Then to comply and run tests, I was doing the following separately (from project above):
then run then run then then |
I have fixed some bugs and the warnings from the pipeline. I also did some refactoring. Your changes from this PR are squash merged in: a673763 Thanks again @maximuslight for your collaboration. |
Added strokeWidth and strokeColor support to "develop" branch to avoid conflicts.
Tested on PHP 8.1 with both drivers.
Aware of Goldengide PR, this is similar.