Skip to content
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

Implement missing functions #14

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

Lustmored
Copy link

@Lustmored Lustmored commented Sep 18, 2020

This PR supersedes #13 and implements all commands.

I have also made some small improvements to previously implemented commands and added some typehint docblocks.

My intention is to implement everything there is in the end and maybe help with upstreaming this driver into intervention if that's OK with you.

@Lustmored Lustmored changed the title Imp[lement most of the missing functions Implement most of the missing functions Sep 18, 2020
@Lustmored Lustmored changed the title Implement most of the missing functions Implement missing functions Sep 18, 2020
@Lustmored
Copy link
Author

@bonzai how do you feel about attempting to merge VIPS driver into Intervention? I would gladly do the effort of preparing PR and handling it, of course leaving credit for the work you did here. Intervention community would probably help with testing and reviewing everything done in more efficient way than separate repository ever could.

I'm no expert in licensing, but seeing this lib licensed "as is" I think I just need an agreement from you to pursue such goal?

@bonzai
Copy link
Member

bonzai commented Sep 23, 2020

@Lustmored I'm not a fan of this because it looks like Intervention is "dead", and I don't want to be in a situation where I'm waiting for my PR to be merged.

@bonzai
Copy link
Member

bonzai commented Sep 23, 2020

  • Vips headers version: 8.10.0
  • Vips library version: 8.10.0
  • Vips ABI version: 54.3.12

I've tested it, and there are problems with the following commands:

Pixelate

It doesn't pixelate the image - it looks like blur - and sometimes it changes the image dimensions.

Image::make('a.jpg')->pixelate(12);
Before (487×511) After (492×516)
a a2

Text

phpfpm/nginx is dying when I'm using this command:

upstream prematurely closed connection while reading response header from upstream

I'll try to test it later on other PC.

@Lustmored
Copy link
Author

Thanks for the feedback! Indeed I missed important step in PixelateCommand and have just pushed the fix.

About the text command - I have tested it on a few different fonts and source images and it works here (I have 8.10 as well, under Gentoo Linux). Could you please share the script (with font file and source image if any) you are using? I will happily dive into it if it's something within driver :)

@bonzai
Copy link
Member

bonzai commented Sep 24, 2020

Here you can find my test image and font: text.zip

Image::make('a.jpg')->text('The quick brown fox jumps over the lazy dog.', 250, 250, function ($font) {
    $font->file('b.ttf');
    $font->size(32);
    $font->color('#fdf6e3');
    $font->align('center');
    $font->valign('top');
    $font->angle(45);
});

intervention

If you can confirm it looks similar, I'll merge your PR.

@Lustmored
Copy link
Author

Indeed there were 2 problems:

  • if font file had different name than font itself, VIPS falled back to some system font. This could be the reason your nginx was dying. I had to add a new dependency to a lib that reads actual font names from TTF to overcome that. I hope that's a reasonable solution.
  • there were positioning issues. In fact GD and Imagick are not very consistent in this matter so I took GD approach and took their math to be as consistent as possible.

The result:
b

GD version for comparison:
b

Font rendering is just as VIPS does that, I have tried to make it look better but failed at a few attempts.

@tsmgeek
Copy link

tsmgeek commented Aug 26, 2021

Would be be possible to get this pulled in, lots of useful draw functions.

@dansleboby
Copy link

Hi, this is a nice lib and this will be a really great update could you merge it?

@forsdahl
Copy link
Contributor

@bonzai could we have this merged, or are there still blocking issues here? At least the ResizeCanvasCommand is needed for using this library with Silverstripe.

@christopherdarling
Copy link

@forsdahl do you have a package for the silverstripe integration? It's on my list to write, so would be a huge help if you were happy to share

@bonzai
Copy link
Member

bonzai commented Jan 23, 2023

@bonzai could we have this merged, or are there still blocking issues here? At least the ResizeCanvasCommand is needed for using this library with Silverstripe.

I'll add ResizeCanvasCommand from this PR later today.

@bonzai
Copy link
Member

bonzai commented Jan 23, 2023

@forsdahl: I've merged ResizeCanvas command.

Can you test it and if something is wrong, send PR with fixes? Thanks!

@forsdahl
Copy link
Contributor

@christopherdarling it doesn't really need any other packages to work with Silverstripe except a yaml config to use it as the image backend, and an ongoing PR (silverstripe/silverstripe-assets#539) I made for silverstripe/assets.
@bonzai tested version 0.10.0, works great now with ResizeCanvas, thanks!

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.

6 participants