RGB <-> HSL converter + hue shifting

Question

My input is basically a .png image from the img/ directory (I figured out that I should parse it, because there is an exploit possible at the moment) and a color which I want to be applied to the image.

The RGB color of every pixel from the image and my input RGB color are being converted to HSL, the H value from my input RGB is merging with the pixel S and L values. This is then being converted back to RGB. My output is the picture with the new colors.

shifthue($image, $mode, $R, $G, $B)  
$image = your PNG image  
$mode = 0 just shifts the hue of the colors, but 1 will also colorize the greys  
$R, $G, $B = R, G and B value the color is thaken from.

I don't really have much programming experience and this is the biggest thing I wrote so far. I am looking for any kind of feedback here!

Demo available here.

Convert.php

<?php

    function validatergb($key){  //doesnt really need to be a own function
        $key = ($key < 0 ? 0 : $key);
        $key = ($key > 255 ? 255 : $key);
        return $key;
    }

    function rgb2hsl($R = 0, $G = 0, $B = 0){  //formulas taken from here: http://www.rapidtables.com/convert/color/rgb-to-hsl.htm
        $R = validatergb($R);
        $G = validatergb($G);
        $B = validatergb($B);

        $R /= 255.0;
        $G /= 255.0;
        $B /= 255.0;

        $Cmax = max([$R, $G, $B]);
        $Cmin = min([$R, $G, $B]);
        $Cdelta = $Cmax - $Cmin;

        if ($Cdelta == 0){
            $H = 0;
        } else {
            switch ($Cmax){
                case $R:
                    $H = fmod((($G-$B)/$Cdelta),6);
                    break;
                case $G:
                    $H = (($B-$R)/$Cdelta)+2.0;
                    break;
                case $B:
                    $H = (($R-$G)/$Cdelta)+4.0;
                    break;
                default:
                    $H = 0;
            }
        }
        $H *= 60;
        $L = ($Cmax + $Cmin) / 2;
        $S = ($Cdelta == 0 ? 0 : ($Cdelta/(1-(abs(2*$L - 1)))))*100;

        $L *= 100;

        return array($H, $S, $L);
    }

    function hsl2rgb($H = 0, $S = 0, $L = 0){  //formulas taken from here: http://www.rapidtables.com/convert/color/hsl-to-rgb.htm

        $H = ($H < 0 ? $H += 360 : $H);  //if $H is smaller than 0, then add 360 because it is measured in degree (sice hue is on a circular model)

        $H /= 60.0;
        $S /= 100.0;
        $L /= 100.0;

        $C = (1-abs(2*$L-1))*$S;
        $X = $C*(1-abs(fmod($H,2)-1));
        $m = $L-($C/2);

        $R = 0;
        $G = 0;
        $B = 0;

        if (($H >= 0)&&($H < 1)){
            $R = $C; $G = $X;
        } elseif (($H >= 1)&&($H < 2)){
            $R = $X; $G = $C;
        } elseif (($H >= 2)&&($H < 3)){
            $G = $C; $B = $X;
        } elseif (($H >= 3)&&($H < 4)){
            $G = $X; $B = $C;
        } elseif (($H >= 4)&&($H < 5)){
            $R = $X; $B = $C;
        } elseif (($H >= 5)&&($H <= 6)){
            $R = $C; $B = $X;
        }

        $R = ($R + $m) * 255;
        $G = ($G + $m) * 255;
        $B = ($B + $m) * 255;

        return array($R, $G, $B);
    }                           

    function hue($R1, $G1, $B1, $R2, $G2, $B2, $mode = 0){  //Takes either the hue ($mode=0) or hue and saturation ($mode=1) of the given color
        $HSL1 = rgb2hsl($R1, $G1, $B1);
        $HSL2 = rgb2hsl($R2, $G2, $B2);

        $H = $HSL2[0];
        $S = ($mode == 1 ? $HSL2[1] : $HSL1[1]);  //picture seem to turn red when r=g=b  &mode=0&r=0&g=0&b=0
        $L = $HSL1[2];

        $RGB = hsl2rgb($H, $S, $L);
        return array($RGB[0], $RGB[1], $RGB[2]);
    }

    function shifthue($image, $mode, $R, $G, $B){  //hue() function over every pixel of the image; goes for every pixel; 
        $img = imagecreatefrompng($image);
        if (($R != NULL)&&($G != NULL)&&($B != NULL)){
            $msk = file_get_contents($image.".txt");  //$msk is a textfile that say which pixel should stay untouched
            $t = 0;  //index for the mask
            $size = getimagesize($image);
            for ($i = 0; $i < $size[0]; $i++){
                for ($j = 0; $j < $size[1]; $j++){
                    if(($msk[$t] == "1")||($msk == NULL)){  //draw a pixel if there is no mask file OR if the char in the mask says "1"
                        $rgb = imagecolorsforindex($img, imagecolorat($img, $i, $j)); //colors from the current pixel are red
                        $R1 = $rgb['red'];
                        $G1 = $rgb['green'];
                        $B1 = $rgb['blue'];
                        $hue = hue($R1, $G1, $B1, $R, $G, $B, $mode); //new color is calculated

                        $colorInt  = round($hue[2]);
                        $colorInt += round($hue[1])<<8;
                        $colorInt += round($hue[0])<<16;  //RGB is saved in a int so we can give it to "imagesetpixel()"

                        imagesetpixel($img, $i, $j, $colorInt);
                    }
                    $t++;
                }
            }
        }
        return $img;
    }

    function output($image, $mode, $R, $G, $B){
        header('Content-Type: image/png');  //comment this out for debugging purposes  
        $img = shifthue($image, $mode, $R, $G, $B);
        imagepng($img);
        imagedestroy($img);
    }

    output("img/".str_replace('/','',$_GET["img"]), $_GET["mode"], $_GET["r"], $_GET["g"], $_GET["b"]);
?>

index.php

 <html>
    <head>
        <link rel="stylesheet" type="text/css" href="style.css"/>
        <script type"text/javascript">
            function initialize(){
                imgElem = document.getElementById("image");
                width = imgElem.naturalWidth;
                height = imgElem.naturalHeight;

                imgElem.width = width*2;
                imgElem.height = height*2;
            }

            function generateImage(){
                imgElem = document.getElementById("image");
                R = document.getElementById("R").value;
                G = document.getElementById("G").value;
                B = document.getElementById("B").value;
                mode = (document.getElementById("mode").checked ? 1 : 0);
                imageID = document.getElementById("imageID").options[document.getElementById("imageID").selectedIndex].innerHTML;
                img = "convert.php?img=" + imageID + "&mode=" + mode + "&r=" + R + "&g=" + G +"&b=" + B;

                imgElem.setAttribute("src", img);

                imgElem.onload = function(){
                    initialize();
                }
            }
        </script>
    </head>
    <body onload="initialize()">
        <div class="content">
            <div class="input">
                <form>
                <?php
                    echo '<select id="imageID" size="1">'.PHP_EOL;
                    $files = glob('img/*.{png}', GLOB_BRACE);   //all .png files from the img/ folder are being dropped in a dropdown menu
                    $value = 0;
                    foreach($files as $file) {  
                         echo '<option value='.$value.'>'.str_replace('img/','',$file).'</option>'.PHP_EOL;
                         $value++;
                    }
                    echo '</select>'.PHP_EOL;
                ?>
                    <p>R: <input type="number" min="0" value="150" max="255" id="R"></p>
                    <p>G: <input type="number" min="0" value="0" max="255" id="G"></p>
                    <p>B: <input type="number" min="0" value="250" max="255" id="B"></p>
                    <p><input type="checkbox" id="mode" text="test"> <label for="mode">Alternative mode</label></p>
                    <p>Some sprites may using a mask and are only partially colored. (ex. 106, 117, 45, 48 etc.)</p>
                    <p><input class="button" type="button" value="Generate" onClick="JavaScript:generateImage()"></p>
                </form>
            </div>
            <div class="output">
                <img id="image" src="convert.php?img=0.png">
            </div>
        </div>
    </body>
</html>

Answer 1

Been a while since I coded PHP, but I'll give it a shot. I'll stick to the conversion functions for this (don't want to re-read the imagemagick PHP API).

Overall, I found the following things:

• Naming.
  Your function names are alloneword. Use snake_case instead, like most of PHP tends to do. Also, you have a lot of variables that are UPPERCASE, like $R, $G, and $B, though there's really no reason. Personally, I'd stick to lowercase or perhaps camelCase for variables. Classes are named with PascalCase and constants are ALL_CAPS_AND_UNDERSCORES, but nothing else should start with a capital letter.

• Whitespace.
  Use more spaces, please. It makes it a lot easier to read a line like fmod((($G-$B)/$Cdelta),6); if you give a bit of breathing room.

• Parentheses.
  Use fewer, please. For instance, you've wrapped all your ternaries, though they don't all have to be. echo $x ? $y : $z works the same as echo ($x ? $y : $z). Also, you're unnecessarily putting parentheses around function calls, like this:

  ($Cdelta/(1-(abs(2*$L - 1)))))*100
  

  That is a lot of close-parens in a row there. But the same can be written as:

  100 * $Cdelta / (1 - abs(2 * $L - 1))
  

  Multiplication is handled before division, and the abs call already has parentheses, so no need to wrap it further. Also note the extra bit of spacing.

Another example of the last two points:

} elseif (($H >= 1)&&($H < 2)){

Why have a space after elseif, but no space between the close-parenthesis and the opening brace (){)? Why wrap the comparisons in parentheses? The same could be written more readably as just:

} elseif ($H >= 1 && $H < 2) {

More specific review items (just going from the top):

function validatergb($key){  //doesnt really need to be a own function

Firstly, spellcheck your comments. Secondly, if it doesn't need to be its own function, why is it its own function? Thirdly, this function does not "validate" anything. It clamps a value to the range 0-255. And lastly, why the name $key? If anything, a single component color of RGB is called a "channel" or simply a "value".

Here's my interpretation:

function clamp_rgb_channel($value) {
    return min(max($value, 0), 255);
}

With regard to rgb2hsl, here's my take. Just general clean-up and some mild refactoring:

function rgb2hsl($r = 0, $g = 0, $b = 0) {
    // you may inline the min(max()) clamping, if you
    // don't want to use a separate function for it
    $r = clamp_rgb_channel($r) / 255.0; 
    $g = clamp_rgb_channel($g) / 255.0;
    $b = clamp_rgb_channel($b) / 255.0;

    // note: No need to use an array; min and max are variadic
    $maxC = max($r, $g, $b);
    $minC = min($r, $g, $b);
    $deltaC = $maxC - $minC;

    if ($deltaC == 0){
        $h = 0;
        $s = 0; // we might as well set this too
    } else {
        $s = $deltaC / (1 - abs(2 * $l - 1));
        switch ($maxC){
            case $r:
                $h = fmod(($g-$b) / $deltaC, 6);
                break;
            case $g:
                $h = (($b-$r) / $deltaC) + 2.0;
                break;
            case $b:
                $h = (($r-$g) / $deltaC) + 4.0;
                break;
            default:
                $h = 0;
        }
    }

    $l = ($maxC + $minC) / 2;

    // put all the final multiplication at the end; bookends
    // nicely with the initial division-by-255
    $h *= 60;
    $l *= 100;
    $s *= 100;

    return array($h, $s, $l);
}

I'll leave off here, since some of these points can be applied to the rest of the code as well.

Only thing to note is that you sometimes use ternaries where a simple if would be clearer, because there's only one branch. For instance:

$H = ($H < 0 ? $H += 360 : $H);

For one, those parentheses are unnecessary, but then so is the ternary. It'd be simpler to just say:

if ($H < 0) {
    $H += 360;
}

Or:

if ($H < 0) $H += 360;

But I recommend always using braces, even for one-liners. An extra keystroke or two never hurt anyone, and only makes the code absolutely unambiguous.

Of course, there's the risk that $H is waaay below zero, in which case adding 360 still wouldn't bring it above zero. Hence, you'll want to use $H % 360 instead. You might even do something like this, if you still want the ternary:

$value %= 360;
$value >= 0 ?: $value += 360;

That ?: is so-called the Elvis operator, by the way. I wouldn't actually recommend coding like this - it's fairly convoluted - but I couldn't help but use the Elvis operator somewhere.

Answer 2

Bearing in mind that this post is almost three years old and your last page view on this site was almost that old as well, you may have likely learned a lot about PHP, JS and HTML since then. Thus the information below may be old news

The answer by @Flambino has covered the main PHP (i.e. in Convert.php) code well. I have a few other points about the other PHP page (i.e. in Index.php) as well as the JavaScript code.

PHP

Use foreach on array in associative manner

The code that adds the <option> elements to the select list manually increments the variable used to generate the value attribute:

$files = glob('img/*.{png}', GLOB_BRACE);   //all .png files from the img/ folder are being dropped in a dropdown menu

$value = 0;
foreach($files as $file) {  
    echo '<option value='.$value.'>'.str_replace('img/','',$file).'</option>'.PHP_EOL;
    $value++;
}

Because glob() returns an array (except when an error occurs, in which case it returns FALSE), the keys will be numeric starting at 0 so use the foreach (array_expression as $key => $value) form of foreach to avoid manually updating $value:

foreach($files as $value => $file) {  
    echo '<option value='.$value.'>'.str_replace('img/','',$file).'</option>'.PHP_EOL;
}

JavaScript

Separate JavaScript code from HTML

While this is currently a small single page application, it is good practice to separate JavaScript function calls from the HTML. For example, the onload attribute of the body tag and the onclick attribute of the button could be removed and instead event listeners can be added in the JavaScript (e.g. using document.addEventListener() for the DOMContentLoaded event on the page and the click event for the button. That way if one person was updating the HTML while the other updated the JavaScript, they could do so independently.

Cache DOM references

It would also be good practice to store references to DOM elements in variables. For example, document.getElementById("image"), and document.getElementById("imageID"). Consider the latter element - if it was stored in a variable:

var imageSelect = document.getElementById("imageID");

Then it could simplify the following line:

imageID = document.getElementById("imageID").options[document.getElementById("imageID").selectedIndex].innerHTML;

To this:

imageID = imageSelect.options[imageSelect.selectedIndex].innerHTML;

which is much shorter, and has a lot fewer queries in the DOM.

Limit scope of variables

Whenever a variable is assigned without a keyword like var, const or let, it is added to the global scope (i.e. window). This can lead to unintentional side-effects if multiple functions use the same variable name.

While it is was posted a few years ago, many of the topics are still relevant in this article about optimizing JavaScript code .