C++

Forum

 
Color Class - Help Needed

HellfireXP (143)
I am building a color class that I will be using with graphic programs but running into a problem. The way I WANT to use the class is like this:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
 
int main()
{
  //THESE WORK!
  Color aClr; // <-- Defines a variable of type Color
  aClr.Red; // <-- Returns the color red as ALLEGRO_COLOR type in format RGB(255,0,0)
  
  //Also can be done as...
  Color().Red; // <-- Returns the color red same as above
  
  //Custom colors:
  Color().RGBA(115,255,0,255); // <-- Returns a custom color with alpha channel included

  //WHAT I WANT TO DO THAT DOESN'T WORK:
  Color(255,0,0); // <-- Would return the color red as above examples

}


Here is the class and I commented over the line that doesn't work right:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
 
class Color{
public:
	ALLEGRO_COLOR Black, White, LightGray, DarkGray, Pink, Red, Maroon, Magenta, 
	Brown, Yellow, LightGreen, Green, DarkGreen, LightBlue, Blue, DarkBlue, Purple;
	Color() 
	:Black(al_map_rgb(0,0,0)), White(al_map_rgb(255,255,255)),
	LightGray(al_map_rgb(195,195,195)), DarkGray(al_map_rgb(127,127,127)),
	Pink(al_map_rgb(255,174,201)), Red(al_map_rgb(255,0,0)), Maroon(al_map_rgb(136,0,21)),
	Magenta(al_map_rgb(255,0,255)), Brown(al_map_rgb(185,122,87)), Yellow(al_map_rgb(255,255,0)),
	LightGreen(al_map_rgb(128,255,128)), Green(al_map_rgb(0,255,0)),
	DarkGreen(al_map_rgb(0,128,0)), LightBlue(al_map_rgb(128,128,255)),
	Blue(al_map_rgb(0,0,255)), DarkBlue(al_map_rgb(0,0,128)), Purple(al_map_rgb(163,73,164))
	{}
        //LINE BELOW DOESN'T WORK RIGHT !? -Error says constructor can't return a value
	Color(int r, int g, int b, int a=255) { return al_map_rgba(r,g,b,a); }
	
        ALLEGRO_COLOR RGBA(int r=0, int g=0, int b=0, int a=255) 
	{
		if (r < 0 || r > 255) r = 0;
		if (g < 0 || g > 255) g = 0;
		if (b < 0 || b > 255) b = 0;
		if (a < 0 || a > 255) a = 255;
		ALLEGRO_COLOR classCLR = al_map_rgba(r,g,b,a);
		return classCLR;
	}
};
Disch (13742)
I don't see the point in creating a separate class for this. ALLEGRO_COLOR kind of already is the class. Furthermore, you wouldn't want each Color object to have copies of all those predefined colors -- it's a waste of memory/cpu time to construct them all.

This would be better as a series of constants / functions.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
namespace Colors
{
  const ALLEGRO_COLOR Black = al_map_rgb(0,0,0);
  const ALLEGRO_COLOR White = al_map_rgb(255,255,255);
  // ... etc
}

ALLEGRO_COLOR Color( int r, int g, int b, int a=255) { return al_map_rgba(r,g,b,a); }


// ....

Colors::Red;  // <- returns the color red as ALLEGRO_COLOR type, without need to construct any objects

Color(255,0,0);  // <- constructs a new color and returns it as ALLEGRO_COLOR type
HellfireXP (143)
I usually use it in the form: Color().Red, or create a single variable and just reset the color as needed. So I'm not actually creating multiple variables of the Color class. But the point of the color class is just to simplify and consolidate the ALLEGRO_COLOR type. The drain on memory is relatively negligible if I"m not creating lots of instances of Color.

Again, I'm trying to figure out how I can return a color value in the form Color(255,0,0) as stated above. I'm not really interested in completely changing my methods of storing/retrieving color.
Last edited on
Disch (13742)
I usually use it in the form: Color().Red, or create a single variable and just reset the color as needed. So I'm not actually creating multiple variables of the Color class


Doing Color() creates a new, unnamed Color object. Each Color object contains 17 ALLEGRO_COLOR objects. Each ALLEGRO_COLOR object is being initialized with a call to al_map_rgb.

This means that doing Color().Red has the following effect:
1) a new Color object is created
2) al_map_rgb is called 17 times to create 17 colors for that Color object
3) the 'Red' object is taken
4) the 16 other objects are thrown away without being used

See how it's wasteful? You're using classes incorrectly.


But whatever. It's very unlikely that this will make any performance difference in your program. And if you don't want to change it, that's fine. But you should at least be aware of it for future reference so you don't make this mistake again.


I'm trying to figure out how I can return a color value in the form Color(255,0,0) as stated above.


Well like I said... the easiest way is to just create a Color function, rather than a class. Since that really seems to be the behavior you want:

1
2
3
4
5
 
// make it a function instead of a class... like this:
ALLEGRO_COLOR Color( int r, int g, int b, int a=255 ) { return al_map_rgba(r,g,b,a); }

// now this will work:
Color(255,0,0);  // <- gives you an ALLEGRO_COLOR



Otherwise if you want to stick with a class, you'll have to redesign your class to be an actual class that keeps track of an internal state.

The constructor cannot return a value because it already returns the instance of the object its constructing (ie: when you do Color(), it returns a Color object... you can't make it return something else. Likewise if you do Color(1,2,3), that's also always going to return a Color object).

Now you can cast a Color object to an ALLEGRO_COLOR, but that means the Color object must "remember" a color between the constructor call, and the cast.

So now you have this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
 
class Color
{
private:
  ALLEGRO_COLOR clr;  // the color this object represents

public:
  Color(int r, int g, int b, int a = 255)
    : clr( al_map_rgba(r,g,b,a) )  // record the color
  {
  }

  // now overload the cast operator so this can be implicitly cast
  //   to an ALLEGRO_COLOR
  operator ALLEGRO_COLOR () const
  {
    return clr;  // return the constructed color
  }
};


Hopefully you can see how silly it is to do it this way rather than the way I explained previously. But I'll leave the final judgement up to you, since it's your program and you can do whatever you want with it.
HellfireXP (143)
This class is part of a Game Engine I'm working on http://www.cplusplus.com/forum/windows/88717/ that I intend to use for numerous game programs. That's kinda why I wanted the colors stored - so I don't have to redeclare all the colors I want to use each time I make a new program. But I understand now about your memory waste issue so I'll stop doing Color().Red and instead just declare a single variable up front, ie. Color gameClr; Then use it throughout the program as needed:

1
2
3
 
gameClr.Red;
gameClr.Blue;
//etc...


Will this prevent the memory issues you mentioned above?
Last edited on
WhiteWind (103)
Surely you can just declare all of the colours and write the function in a separate file then include the file in future programs? That's what I do.

The problem is that a colour isn't an object - it doesn't make sense for it to be an object. What use do you want from the Color class, other than just returning an RGBA value? Do you want the colour class to hold items in your game? Is it going to control physics? Hopefully not.

Even if colour is somehow important for other elements in your games (perhaps territory colours or player X is colour X), you would add that functionality to other classes while getting the colour value from the function Disch suggests.

That's just my two cents, anyway.
Last edited on
HellfireXP (143)
I tweaked this alot.. I broke out the "default" colors that I want to be able to use from custom colors. Also, defined "Colors" as type COLORS so I wouldn't have to create multiple instances...

Any advice on improving this would be appreciated or if you see anything I might have missed.. memory leaks?

Also, a side note - the ALLEGRO_COLOR type can't use the == or != operators thus the reason for adding them.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
 
namespace palette{

//COLOR PALETTES
struct COLORS{
	ALLEGRO_COLOR Black, White, LightGray, DarkGray, Pink, Red,
                      Maroon, Magenta, Brown, Yellow, LightGreen, 
                      Green, DarkGreen, LightBlue, Blue, DarkBlue, Purple;
	COLORS()
		:Black			(al_map_rgba(0,0,0,255)),
		 White			(al_map_rgba(255,255,255,255)),
		 LightGray	        (al_map_rgba(195,195,195,255)),
		 DarkGray		(al_map_rgba(127,127,127,255)),
		 Pink		        (al_map_rgba(255,174,201,255)),
		 Red			(al_map_rgba(255,0,0,255)),
		 Maroon		        (al_map_rgba(136,0,21,255)),
		 Magenta		(al_map_rgba(255,0,255,255)),
		 Brown			(al_map_rgba(185,122,87,255)),
		 Yellow			(al_map_rgba(255,255,0,255)),
		 LightGreen	        (al_map_rgba(128,255,128,255)),
		 Green			(al_map_rgba(0,255,0,255)),
		 DarkGreen	        (al_map_rgba(0,128,0,255)),
		 LightBlue    	        (al_map_rgba(128,128,255,255)),
		 Blue			(al_map_rgba(0,0,255,255)),
		 DarkBlue		(al_map_rgba(0,0,128,255)),
		 Purple			(al_map_rgba(163,73,164,255)) 
		 {}
}Colors;

class COLOR{
private:
	ALLEGRO_COLOR CLR;
public:
	COLOR() :CLR(al_map_rgba(0,0,0,255)) {}
	COLOR(int r, int g, int b, int a) :CLR(al_map_rgba(r,g,b,a)) {}
	void Set(ALLEGRO_COLOR clr) { CLR = clr; }
	void Set(int r=0,int g=0, int b=0, int a=255)
	{
		if (r < 0 || r > 255) r = 0;
		if (g < 0 || g > 255) g = 0;
		if (b < 0 || b > 255) b = 0;
		if (a < 0 || a > 255) a = 255;
		CLR = al_map_rgba(r,g,b,a);
	}
	ALLEGRO_COLOR Get() { return CLR; }
	bool operator==(COLOR &Clr)
	{
		if (CLR.r == Clr.Get().r &&
		    CLR.g == Clr.Get().g &&
		    CLR.b == Clr.Get().b &&
		    CLR.a == Clr.Get().a) return true;
		return false;
	}
		bool operator!=(COLOR &Clr)
	{
		if (CLR.r != Clr.Get().r ||
		    CLR.g != Clr.Get().g ||
		    CLR.b != Clr.Get().b ||
		    CLR.a != Clr.Get().a) return true;
		return false;
	}
};

//FONT PALETTE
class FONT{
private:
	bool VALID;
	int Size;
	TEXT_ALIGN Alignment;
	ALLEGRO_COLOR Clr;
	string Name;
public:
	FONT() :VALID(false), Size(12), Alignment(LEFT), Clr(al_map_rgb(0,0,0)), Name("arial.ttf") {}
	void Initialize(string name, int size = 12, TEXT_ALIGN alignment = LEFT, ALLEGRO_COLOR clr = al_map_rgb(0,0,0))
	{
		VALID = false;
		Size = size;
		Alignment = alignment;
		Clr = clr;
		char * dir = _getcwd(NULL, 0); 
		stringstream ss;
		ss << dir << "\\" << name;
		string fPath = ss.str();
		Name = fPath;
		if (ifstream(fPath.c_str())) VALID = true;
	}
	string GetName() { return Name; }
	ALLEGRO_COLOR GetColor() { return Clr; }
	TEXT_ALIGN GetAlignment() { return Alignment; }
	int GetSize() { return Size; }
	bool IsValid() { return VALID; }
};

} //End of palette namespace
Salamander3 (1)
If you're already happy with predefining the colours as an RGBA value within the code, why not store the RGBA values in a vector of values, then set up an enum to act as reference? Like so:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
 
//palette.h

class Palette
{
public:

   enum Colours
   {
      RED = 0,
      BLUE,
      GREEN,
      BLACK
   }
   
   Palette();
   ~Palette();
    ALLEGRO_COLOR getColour(int c){return al_map_rgba(colourMap.at(c)[0],colourMap.at(c)[1],colourMap.at(c)[2],colourMap.at(c)[3]) };

private:
   vector<int[4]> colourMap;
}



//Palette.cpp
#include "Palette.h"

Palette::Palette()
{
   int colours[4] = {255,0,0,255};
   colourMap.push_back(colours);
   colours[4] = {0,0,255,255};
   colourMap.push_back(colours);
   //etc etc

}


then the above code would be called by using the enum as the vector reference: 
1
2
3
 

ALLEGRO_COLOR colour = Palette.getColour(Palette::RED);


disclaimer: just wrote this code while I was posting, and it's my first post. Not been parsed, let alone error checked, don't take it for working code. Just saying, this is how I'd do it if I didn't want to have to call a colour by it's composite numbers every time.
Topic archived. No new replies allowed.