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

Only pass crop parameter when ratio is specified #2

Open
hirasso opened this issue Oct 12, 2023 · 8 comments
Open

Only pass crop parameter when ratio is specified #2

hirasso opened this issue Oct 12, 2023 · 8 comments
Labels
type: enhancement ✨ Suggests an enhancement; improves the plugin

Comments

@hirasso
Copy link

hirasso commented Oct 12, 2023

Hi @tobimori ,

thanks so much for this beautiful plugin!

Today I suddenly ran into memory issues. While source-diving and playing around, I noticed that in my case, the $file->thumb() doesn't actually do any downsizing when cropping is set to true, leading to Memory allocation errors.

'crop' => true,

Setting that to false magically fixed it. While this of course doesn't really make sense and seems to be pointing towards an underlying issue in my Kirby install, I'm still wondering what the reasoning is behind setting cropping to true?

@tobimori
Copy link
Owner

When you crop an image, you get a FileVersion class, which doesn't support file methods and hence you can't create a thumbhash out of a cropped image.

In order to still support cropped images, you can supply the ratio to the original file method call like so: https://github.com/tobimori/kirby-thumbhash#cropped-images

This is what the plugin uses cropping for.

@hirasso
Copy link
Author

hirasso commented Oct 12, 2023

Whoaa that was quick! 🚀😄

Sorry, I had an error in my original question. I'm asking if crop should not be set to false (not true!) by the plugin.

@tobimori
Copy link
Owner

You could try modifying the line like this and still have ratio/crop support:
'crop' => isset($options['ratio']),

@hirasso
Copy link
Author

hirasso commented Oct 12, 2023

I don't need crop support 😄 I want it off by default

@tobimori
Copy link
Owner

Then just set it to false.

@hirasso
Copy link
Author

hirasso commented Oct 12, 2023

Sure, that's what I did. Do you accept PRs?

@tobimori
Copy link
Owner

Try replacing the script with the following:
https://gist.github.com/tobimori/78eb776ad7fe5e1a608b2d1121fb9d66

Does the error still occur with that?

@hirasso
Copy link
Author

hirasso commented Oct 12, 2023

Yes it's working with the code above!

@tobimori tobimori changed the title Memory Issues with crop: true Only pass crop parameter when ratio is specified Oct 13, 2023
@tobimori tobimori added the type: enhancement ✨ Suggests an enhancement; improves the plugin label Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves the plugin
Projects
None yet
Development

No branches or pull requests

2 participants