search

claviska / SimpleImage

A PHP class that makes working with images and GD as simple as possible.
MIT License
1.38k stars 382 forks source link

Add compression for BMP #286

Closed fflnvb closed 1 year ago

fflnvb commented 2 years ago

Hi, while using SimpleImage for creating BMP Files for my E-Paper Display I noticed that all BMP Files are being compressed by default with this class. While most file formats are offering a quality param, imagebmp() does not have that param but instead a boolean for compression. I guess adding a bool inside the param Quality would work regarding the result, but it would not really stick to the documentation and described data type, being an int.

Therefore I suggest making the following change for the SimpleImage Class seen below. Feel free to accept, modify wording etc. or reject it. I already work with that modified class locally :smile:

Here's the doc for imagebmp(): https://www.php.net/manual/de/function.imagebmp.php

fflnvb commented 2 years ago

Following up on my answer to your comment: I guess my commit for the class can be purged. Instead we will need to update the README.md on lines 149, 159, 168, 177 as well as 186. As of now they all have the following description:

-$quality(int) - Image quality as a percentage (default 100). This argument has no effect on PNG images, since the format is lossless.

I would suggest the following addition at the end of these lines: Set to 0 for disabling compression on BMP images 0 instead of false because this is how we could keep the data type.

Would that be a good idea?

Edit: just saw your comment up there, so there needs to be also a change for describing the data type like: (int|bool) and then suggesting with Set to false...

claviska commented 1 year ago

I should have replied here instead of using reactions since those don't send out notifications. Yes, the updated proposal seems like a good idea to me. Thanks!

bnomei commented 1 year ago

@claviska this PR can be closed and not merged since its solved in v4

claviska commented 1 year ago

Thanks, @bnomei. Indeed, it looks like this has been resolved in 4.0.