7
\$\begingroup\$

I just wrote a small class that generates perfectly unique colors(or that is the plan anyway). I found the original index shifting snippet online, and was just wondering if it could be improved in anyway, or if any problems could arise from using it.

Here is the class which is used to create the unique colors.

public class UniqueColorGenerator
    {
        private int _colorIndex;
        public UniqueColorGenerator()
        {

        }

        public Color Next()
        {
            _colorIndex++;
            byte red = ( byte )( _colorIndex & 0x000000FF );
            byte green = ( byte )( ( _colorIndex & 0x0000FF00 ) >> 08 );
            byte blue = ( byte )( ( _colorIndex & 0x00FF0000 ) >> 16 );
            return Color.FromArgb( red , green , blue );
        }
    }

I am using this for "geometry picking" in OpenGL. It works beautifully, too. Here is a picture of 16,384 quads with their own unique color... or I guess I should say "unique shade of red and black".

enter image description here

When I first ran it, I could swear that there were obvious duplicates, but there aren't. Not only did the picking work flawlessly, but I also ran through the Colors and didn't get a single collision. Though I am still wondering if any problems could arise from how the generator is currently built—besides hitting integer max, which will also never happen.

Also, how can I randomize the colors more, so instead of getting different shades of red, I could receive more colors along the spectrum?

\$\endgroup\$
  • \$\begingroup\$ You can get some pretty awesome gradients if you increase the index growth, too. imgur.com/yUrboaC \$\endgroup\$ – Krythic Apr 16 '15 at 6:48
6
\$\begingroup\$

There's not much code here, but I think it can be pretty cleanly divided into two parts:

  • Generate three bytes for a color from an integer
  • Have a way of getting multiple integers which are guaranteed to be unique

Simple as your algorithm is, you're already running into limitations by coupling these two pieces of functionality together (which, by the way, makes this an excellent object lesson in the importance of the Single Responsibility Principle!)

Namely, in your question, all the colors are red, and in your answer, you're still getting colors following a completely deterministic numeric pattern. Depending on your offset, this pattern will be more or less obvious (it's not just low vs. high- I'm pretty sure there's an offset which would give you the first image with red turned to blue, for example).

So, let's see what we can do about this unwanted coupling.


Separating Responsibilities

As I said before, you have two pieces of functionality. The first is given by these lines:

byte red = ( byte )( _colorIndex & 0x000000FF );
byte green = ( byte )( ( _colorIndex & 0x0000FF00 ) >> 08 );
byte blue = ( byte )( ( _colorIndex & 0x00FF0000 ) >> 16 );
return Color.FromArgb( red , green , blue );

This makes a nice unit of functionality itself, with a single, simple purpose: Get a color from an integer index. So put the above in a method (say, GetColorFromIndex, with index as a parameter replacing _colorIndex).

The other ingredient is generating unique integers. This is what you really want to separate out into its own class, implementing an interface. That way, you can generate numbers however you want to get different patterns: increment by one, increment by an offset, use the numbers 1 to N shuffled randomly, whatever you can think of! By using polymorphism, instead of being limited to one way of generating numbers with some potential for parametrization (like offsetIncrement), you will be able to use whatever algorithms you want.

Fortunately, an interface already exists for exactly what you want: IEnumerable<int>. You can often think of an IEnumerable<T> as a collection to iterate over, but in some situations I find it more conceptually useful to think of it as a box that I can just keep asking for a new T (though it may eventually reach the end of its Ts). In this case, that's exactly what you want- a box you can just keep pulling integers out of.

So let's pull this together:

public class ColorGenerator
{
    private IEnumerator<int> _indexGenerator;
    public ColorGenerator(IEnumerable<int> indexGenerator)
    {
        _indexGenerator = indexGenerator.GetEnumerator();
    }

    public Color Next()
    {
        _indexGenerator.MoveNext();
        return GetColorFromIndex(_indexGenerator.Current);
    }

    private Color GetColorFromIndex(int index)
    {
        byte red = ( byte )( index & 0x000000FF );
        byte green = ( byte )( ( index & 0x0000FF00 ) >> 08 );
        byte blue = ( byte )( ( index & 0x00FF0000 ) >> 16 );
        return Color.FromArgb( red , green , blue );
    }
}

Note that now I've removed Unique from the name- it's now the responsibility of the passed in IEnumerable<int> to decide if it's unique or not.

I'll leave implementing IEnumerable<int> for your 'color offset' version as an exercise, it should be straightforward.


IEnumerable

Okay, the above is a bit ugly- lots of fiddling with enumerators, and unhandled error cases (what happens if you reach the end of a finite collection of integers?). But this is because we're missing another trick: ColorGenerator itself is a box you can pull Colors out of, so it should itself be an IEnumerable<Color>. The structure of just repeatedly asking for a "Next" value very closely matches how IEnumerator<T>is laid out. So that aforementioned ugliness is a code smell specific to C# hinting at this to us.

So, making that adjustment, we end up with:

public class ColorGenerator : IEnumerable<Color>
{
    private IEnumerable<int> _indexGenerator;
    public ColorGenerator(IEnumerable<int> indexGenerator)
    {
        _indexGenerator = indexGenerator;
    }

    public IEnumerator<Color> GetEnumerator()
    {
        foreach(var index in _indexGenerator)
        {
            yield return GetColorFromIndex(index);
        }
    }

    private Color GetColorFromIndex(int index)
    {
        byte red = ( byte )( index & 0x000000FF );
        byte green = ( byte )( ( index & 0x0000FF00 ) >> 08 );
        byte blue = ( byte )( ( index & 0x00FF0000 ) >> 16 );
        return Color.FromArgb( red , green , blue );
    }
}

Now, as a bonus from our refactoring we get all the power and expressiveness of LINQ extensions for our color generator. If we want 10 colors, instead of:

var colors = new List<Color>();
for(int i=0; i<10; i++)
{
    colors.Add(colorGenerator.Next());
}

We can do:

var colors = colorGenerator.Take(10);
\$\endgroup\$
  • \$\begingroup\$ I'd definitely be interested to see the output of that final version with various input IEnumerable<int>s. If you want a simple "0 to N in random order" verison, this might be useful: stackoverflow.com/questions/273313/randomize-a-listt-in-c-sharp , together with Enumerable.Range. This still isn't perfect for N < Int32.MaxValue because it will always exclude certain colors, but would be a good first pass. \$\endgroup\$ – Ben Aaronson Apr 16 '15 at 12:59
  • \$\begingroup\$ Can you give an example of what should be added into the constructor? \$\endgroup\$ – Krythic Apr 16 '15 at 17:02
  • 1
    \$\begingroup\$ @Krythic Enumerable.Range(0, 16384) should get you what you have in your first image (if I'm understanding correctly and there are 16384 colors). If you implement the Fisher-Yates shuffle as in the top answer to the question I linked in my previous comment, then Enumerable.Range(0, 16384).ToList().Shuffle() would get you the same colors, but in a random order. Or you could make a small method which fills a list with N random integers between 0 and Int32.MaxValue then pass in that list- though this wouldn't be guaranteed unique. \$\endgroup\$ – Ben Aaronson Apr 16 '15 at 18:34
7
\$\begingroup\$

You don't need the constructor, as it's empty.

You're taking red as the low bits and blue as the high bits, which is backwards.

You appear to be using System.Drawing and not System.Windows.Media, judging by the overloads. If so, there is an overload that takes an int:

public class UniqueColorGenerator
{
    private int _colorIndex;

    public Color Next()
    {
        return Color.FromArgb(++_colorIndex);
    }
}

You also wouldn't need to use &, as casting to a byte will cut off the upper bits.

return Color.FromArgb(
    (byte)(_colorIndex >> 24), 
    (byte)(_colorIndex >> 16), 
    (byte)(_colorIndex >> 8), 
    (byte)_colorIndex);
\$\endgroup\$
  • \$\begingroup\$ I used your code; it generated colors that are blue instead of red. \$\endgroup\$ – Krythic Apr 16 '15 at 7:07
  • \$\begingroup\$ That's because 0x000000FF doesn't mean red. It means blue. See my second sentence above. It's RGB, not BGR. If you want red, start from 0x10000. \$\endgroup\$ – Millie Smith Apr 16 '15 at 7:10
  • \$\begingroup\$ I see that now. Also, if I toy with the index I start to get some green in there, too. imgur.com/XZNYcPZ It looks pretty freaking cool. \$\endgroup\$ – Krythic Apr 16 '15 at 7:12
2
\$\begingroup\$

I just wanted to post my own little something. I won't be accepting this as the answer, but I just wanted to show a simple mockup using the suggestions that I had already recieved.

public class UniqueColorGenerator {
        private int _colorIndex;
        private readonly int _offsetIncrement;

        public UniqueColorGenerator() {
            this._offsetIncrement = 1;
        }

        public UniqueColorGenerator( uint offsetIncrement ) {
            this._offsetIncrement = ( int )offsetIncrement;
        }

        public Color Next() {
            return Color.FromArgb( _colorIndex += _offsetIncrement );
        }
    }

That class does the exact same thing as my original example, but is a lot simpler, and even adds room for a bit of uniqueness. The higher number that you add in the constructor, the more crazy the final pattern becomes; here is a quick example of what happens if you feed "2834783" into the constructor:

enter image description here

And yes, all of the colors are still unique. The only issue that I can see from this new code is an issue that would arise if the user put in a ridiculously high number.

\$\endgroup\$
  • 1
    \$\begingroup\$ You ask for random colors, but your result is a pattern. Do you want random unique colors or well distributed unique colors? \$\endgroup\$ – 0xBADF00D Apr 16 '15 at 11:03
  • 1
    \$\begingroup\$ In order to guarantee that the colors are unique, you must ensure that 2^24 and your offset are coprime. That is, you must check that the offset is not divisible by 2. Otherwise you get repeating colors even before the whole color space with 16777216 is exhausted. Add this check into the constructor of UniqueColorGenerator, otherwise it's not worth its name. \$\endgroup\$ – Andrey Tyukin Apr 16 '15 at 16:31
  • \$\begingroup\$ @user27811 Well distributed unique colors. I fixed the typo in the title. \$\endgroup\$ – Krythic Apr 16 '15 at 17:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.