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

Upgrade sharp to 0.33.4 #193

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

Upgrade sharp to 0.33.4 #193

wants to merge 2 commits into from

Conversation

joehoyle
Copy link
Member

No description provided.

@joehoyle
Copy link
Member Author

This appears to be an issue with lovell/sharp#4163 with pre-built binaries

@lovell
Copy link

lovell commented Jul 27, 2024

I think this might be due to the overly-restrictive peerDependencies of the smartcrop-sharp dependency - see jwagner/smartcrop-sharp#32

If you weren't already aware, sharp provides a smartcrop feature via a position of "entropy" or "attention".

await sharp(input).resize({ width, height, position: "entropy" })...

https://sharp.pixelplumbing.com/api-resize#resize

@rmccue
Copy link
Member

rmccue commented Aug 2, 2024

Thanks @lovell (and thanks for the great library!). Might be a good idea for us to reduce our dependencies here if the behaviour is equivalent.

I'm not sure to the extent we're using smart cropping, or what the impact of switching would be; the only mention of it in tachyon-plugin in the docs, albeit we appear to be using it in smart-media for resizes (but not crops).

@roborourke Any insight on this?

@roborourke
Copy link
Contributor

The behaviour isn’t equivalent unfortunately or I would agree on reducing dependencies. The smartcrop-sharp library is a wrapper around https://github.com/jwagner/smartcrop.js so we could perhaps switch to using that directly instead to avoid the extra dependency.

The smartcrop.js results are generally better in my testing than entropy or attention especially when it comes to faces and people which is why I implemented it in Tachyon and used it as the default in Altis.

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.

4 participants