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

Enhance GD::greyscale to allow darker or lighter results #5332

Merged
merged 1 commit into from
Apr 21, 2016

Conversation

JorisDebonnet
Copy link
Contributor

@JorisDebonnet JorisDebonnet commented Apr 18, 2016

Tweaking the first three parameters (R, G, B) allows you to get a different mix of colors into grey, but it does not allow you to make the result lighter or darker because their total is normalized. This is quite a common operation on greyscaled images though, so I added a fourth parameter: brightness. It defaults to 100% so that it does not have any effect when not used (making this backwards compatible). $brightness = 50 will make it darker, $brithgness = 150 will overlight it.

By the way, please note that the default R, G, B values for the current greyscale function are very wrong, but I didn't dare change that because that would cause images to come out differently than before... It just makes no sense though in any scheme to allocate more weight to red than to green! A better scheme would be this common one: 30, 59, 11 -- or more accurately (why not) 299, 587, 114. Just as a note; I'd be happy to add this as a change of course, but assume I shouldn't.

@sminnee
Copy link
Member

sminnee commented Apr 19, 2016

Hrm, if the defaults are wrong—that is, that they don't provide an accurate representation of luminance—I'd be okay with declaring that a bugfix and fixing in the 3 branch.

If we do change them, however, can we find an external source URL of the values and put it as a comment in the code, so people know where we got the numbers from?

@JorisDebonnet
Copy link
Contributor Author

Okay, I have updated the weights and added references!

@sminnee
Copy link
Member

sminnee commented Apr 21, 2016

Thanks, @JorisDebonnet, I notice that the GDTest::testGreyscale() is now failing. It may need some changes to work with your amended colourings. Can you please get it working again?

…color weights for greyscale

Tweaking the first three parameters (R, G, B) allows you to get a different mix of colors into grey, but it does not allow you to make the result lighter or darker because their total is normalized. This is quite a common operation on greyscaled images though, so I added a fourth parameter: brightness. It defaults to 100% so that it does not have any effect when not used (making this backwards compatible). $brightness = 50 will make it darker, $brightness = 150 will overlight it.
@JorisDebonnet
Copy link
Contributor Author

Right, should have checked that. Working now!

@sminnee sminnee merged commit 9eac541 into silverstripe:3 Apr 21, 2016
@JorisDebonnet
Copy link
Contributor Author

Since this pull request, I have actually added another parameter, but I'm not sure how far this should go ...

The brightness parameter allows all color values to be scaled linearly, which effectively makes the image darker or more bright, but can lead to weak contrast. I didn't fuss about a fully-functional $contrast parameter, but just added a $shift parameter which reduces or increases all values by the same amount (instead of scaling). Although this has helped me get the result I wanted (a darker but also still contrasty greyscale version), I don't think it's general enough to include into the core... Perhaps if I did in fact create a functional $contrast parameter, but if I do, it would make more sense to create a separate method for brightness and contrast instead of putting it into the greyscale function!...

I think I could actually implement the whole Levels functionality like in Photoshop, but of course the question is whether anyone would ever use it (instead of doing that before you actually upload). So I'm not really sure whether I should add anything now, or just leave it as it is.

@sminnee
Copy link
Member

sminnee commented Apr 23, 2016

Hey Joris,

I'm happy to review & merge work you make on this, but I wonder if efforts are better put into swapping the GD class for something that links to a more mature (and independently maintained) image processing library.

Introducing new features would be okay for the 3 branch, but the best bet might be to implement such a feature as an integration into the new asset system master.

What do you think? If you're keen I'm sure that @tractorcow and others could give some pointers on how a new image-variant-generator could plug into the asset system.

@tractorcow
Copy link
Contributor

I like the idea of still relying on GD, but avoiding as much manual manipulation logic as possible in core code. I'd much rather we swap in something like https://github.com/Gregwar/Image, which still relies on GD, but relieves us of a maintenance burden.

Implementing it as a new Image_Backend implementor could even allow us to get it into 3.

@tractorcow
Copy link
Contributor

There's even a WIP to support Imagick

Gregwar/Image#65

I think someone like @JorisDebonnet or @jonom (all the jo-s) would be great at finishing such an implementation, should they think it worth their time. :)

@tractorcow
Copy link
Contributor

Adding to my personal TODOs. :D tractorcow#16

@jonom
Copy link
Contributor

jonom commented May 5, 2016

(all the jo-s)

Lol. Using something like Gregwar/Image sounds cool! Here are some other possible contenders:

Sidenote: currently greyscale() is a bit out of reach because it's only on the GD backend, no equivalent on the Imagick one, and lack of Greyscale() function on Image means you can't use it out the box with e.g. $Image.ScaleWidth(300).Greyscale. You'd need to extend Image and add a custom accessor method and accompanying generator method to achieve that functionality.

If we relied on a third party library it would be great if you could access and chain the majority of the provided methods from templates.

@JorisDebonnet JorisDebonnet deleted the patch-1 branch January 9, 2017 17:12
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