Ytdyklly

I wrote this little piece of code to linearly interpolate between a winforms control's background colour and any arbirtrarly chosen colour.

I don't like the way I wrote this piece and I was wondering if any of you would be interested in suggesting improvements.

Some improvements/thoughts from the top of my head:

1. If the user calls the method while a previous iteration is still ongoing, the background colour will not be predictable. This is not intended. I would require some form of if(!colourChanging.IsActive)... and so on. Any ideas on how to implement that?


2. Do I need to dispose the task? No, right? The task will take a threadpool thread, release all of its resource at its end and return the thread back to the pool?


3. Is this a stupid approach to begin with?


4. How do I get the complementary colour of Color _eventColour? I think it might make sense to change the foreground color property to maintain readability.


5. Is this approach scalable to lets say 100 controls - to be used in an event driven environment?

private void ChangeControlColour(Control _activeControl,  Color _eventColour) 

{

    int _rgbEventColours = new int[3] { _eventColour.R, _eventColour.G, _eventColour.B };

    int _rgbOriginalColours = new int[3] { _activeControl.BackColor.R, _activeControl.BackColor.G, _activeControl.BackColor.B };

    int _rgbIntervals = new int[3];

    int _rgbCurrentValues = _rgbEventColours; 

    int _intervals = 20;



    /* linear steps between each fading interval */

    for (int i = 0; i < 3; i++)

        _rgbIntervals[i] = (_rgbOriginalColours[i] - _rgbEventColours[i]) / _intervals;



    /*  Start with EventColour  */

    if (_activeControl.InvokeRequired)

        _activeControl.Invoke((MethodInvoker)delegate { _activeControl.BackColor = _eventColour; });

    else 

        _activeControl.BackColor = _eventColour;



    /*  LinearFading Process isolated in a seperate Task to avoid blocking UI   */

    Task t = Task.Factory.StartNew(() =>

    {

        System.Threading.Thread.Sleep(500);



        Color _fadingColour;

        for (int i = 0; i <= _intervals; i++)

        {

            _fadingColour = Color.FromArgb(_rgbCurrentValues[0], _rgbCurrentValues[1], _rgbCurrentValues[2]);



            if (_activeControl.InvokeRequired)

                _activeControl.Invoke((MethodInvoker)delegate {_activeControl.BackColor = _fadingColour;});

            else

                _activeControl.BackColor = _fadingColour;



            for (int n = 0; n < 3; n++) _rgbCurrentValues[n] += _rgbIntervals[n];



            System.Threading.Thread.Sleep(50);

        }



        /*  More important for non linear interpolation, but for completeness - set Control's Colour back

         *  to its original backgroundcolour    */

        Color _startingColour = Color.FromArgb(_rgbOriginalColours[0], _rgbOriginalColours[1], _rgbOriginalColours[2]);



        if (_activeControl.InvokeRequired)

            _activeControl.Invoke((MethodInvoker)delegate { _activeControl.BackColor = _startingColour; });

        else

            _activeControl.BackColor = _startingColour;

    });

}

Edit / Update on Sept. 20th 2014.

ChrisWue has answered the original question in a very helpful and detailed way. Thanks!

I do have a follow up:

Regarding "making sure that only one fading effect is running at a time" is the following theoretical solution a feasible approach or would there be a more efficient, pragmatic approach ? :

(let's assume I create a new instance of the ColorFader on some random event X for a control Y)

I store all ColorFader instances in a List and give them a field to remember their WinForms control Y. so every time the event is fired for a control Y I would check if there already is a ColorFader in the list referencing this particular Control Y.

In the ColorFader Class I would need to implement IDisposable . And as soon as the the object is disposed I would need to get rid of it in the List .

Got a better idea anyone?

The "problem" I see with this is the additional management of objects. It is not nicely encapsulated, because every project I would like to use this in I would have to create a new List just to keep track of the Objects + work to add and remove from said list.

(I will have around 20-40 labels which could fire a ColorFading event at any given time)

1. The standard naming convention in C# for local variables and parameters is camelCase. An underscore prefix is usually used for private class fields.




2. You repeat the code so the the background color three times - this should be extracted into a separate method.



3. 
   

   This will perform an integer division:

   
   
   

   _rgbIntervals[i] = (_rgbOriginalColours[i] - _rgbEventColours[i]) / _intervals;
   
   

   
   
   

   which means if the difference between two color part is less than _intervals the calculated step is 0. Also you will get quite big accumulated errors due to the rounding. For example if you start with the color (30, 30, 30) and want it to fade to (60, 60, 60) then you will end up only fading to (50, 50, 50) because 60 - 30 = 30 and 30 / 20 = 1.5 but the 1.5 will be truncated to 1 due to integer division.

   
   
   

   You do set the final color to the target color but it would be smoother to calculate the step sizes as floats.

   
   



4. You keep track of the various part and intermediate values in a bunch of arrays which do not have any direct relationship. It's also not very re-usable this way. I would encapsulate it in it's own class.

So the refactored code could look like this:

Class to encapsulate the fading between two colors:

public class ColorFader

{

    private readonly Color _From;

    private readonly Color _To;



    private readonly double _StepR;

    private readonly double _StepG;

    private readonly double _StepB;



    private readonly uint _Steps;



    public ColorFader(Color from, Color to, uint steps)

    {

        if (steps == 0)

            throw new ArgumentException("steps must be a positive number");



        _From = from;

        _To = to;

        _Steps = steps;



        _StepR = (double)(_To.R - _From.R) / _Steps;

        _StepG = (double)(_To.G - _From.G) / _Steps;

        _StepB = (double)(_To.B - _From.B) / _Steps;

    }



    public IEnumerable<Color> Fade()

    {

        for (uint i = 0; i < _Steps; ++i)

        {

            yield return Color.FromArgb((int)(_From.R + i * _StepR), (int)(_From.G + i * _StepG), (int)(_From.B + i * _StepB));

        }

        yield return _To; // make sure we always return the exact target color last

    }

}

And the refactored method in your form:

private void ChangeControlColour(Control activeControl,  Color eventColour) 

{

    int intervals = 20;



    var colorFader = new ColorFader(eventColour, activeControl.BackColor, intervals);



    SetControlBackColor(eventColor);



    /*  LinearFading Process isolated in a seperate Task to avoid blocking UI   */

    Task t = Task.Factory.StartNew(() =>

    {

        System.Threading.Thread.Sleep(500);

        foreach (var color in colorFader.Fade())

        {

            SetControlBackColor(color);

            System.Threading.Thread.Sleep(50);

        }

    });

}



private void SetControlBackColor(Color color)

{

    if (_activeControl.InvokeRequired)

        _activeControl.Invoke((MethodInvoker)delegate { _activeControl.BackColor = color; });

    else

        _activeControl.BackColor = color;

}