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

Added a new constructor that takes single Uint32 to Color #722

Merged
merged 2 commits into from Feb 23, 2015

Conversation

Projects
None yet
8 participants
@TankOs
Member

TankOs commented Oct 13, 2014

Added a new constructor that takes single Uint32 to Color

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 13, 2014

Member

It would be safer to make this constructor explicit. And why not add the Color -> Uint32 conversion as well (let's call it Color::toInteger())?

This will be similar to what's done in sf::IpAddress.

Member

LaurentGomila commented Oct 13, 2014

It would be safer to make this constructor explicit. And why not add the Color -> Uint32 conversion as well (let's call it Color::toInteger())?

This will be similar to what's done in sf::IpAddress.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Oct 13, 2014

Member

Yep, I agree to everything. ;) @FRex

Member

TankOs commented Oct 13, 2014

Yep, I agree to everything. ;) @FRex

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Oct 13, 2014

Contributor

No, no - the whole point was to NOT make it explicit to allow using unsigned as if it itself is a color (setColor(0xff00ffff)) instead of having to bother typing anything more than that. Making it explicit makes this feature redundant becuase you could have written your own conversion function (or 10) already and named it however you liked. There is little risk making it implicit because it's very unlikely someone will pass unsigned accidentally to function expecting a color without knowing what they are doing. As for why IpAdress does it this way - I don't know and I don't care, especially since hex adress representation is very rare (IMO..), way more rare than the normal 4-octets-in-decimal one, there are tons of insonsistency about this (implicit constructors) already anyway - single char or unsigned can be turned into 1 length sf::String implicitly (kind of weird) and const char * and std::string can be turned into IpAdress implicitely (I guess because it's more common to use names or dot separated octets that it is to use hex).

Contributor

FRex commented Oct 13, 2014

No, no - the whole point was to NOT make it explicit to allow using unsigned as if it itself is a color (setColor(0xff00ffff)) instead of having to bother typing anything more than that. Making it explicit makes this feature redundant becuase you could have written your own conversion function (or 10) already and named it however you liked. There is little risk making it implicit because it's very unlikely someone will pass unsigned accidentally to function expecting a color without knowing what they are doing. As for why IpAdress does it this way - I don't know and I don't care, especially since hex adress representation is very rare (IMO..), way more rare than the normal 4-octets-in-decimal one, there are tons of insonsistency about this (implicit constructors) already anyway - single char or unsigned can be turned into 1 length sf::String implicitly (kind of weird) and const char * and std::string can be turned into IpAdress implicitely (I guess because it's more common to use names or dot separated octets that it is to use hex).

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 13, 2014

Member

Hm... may I suggest using ARGB rather than RGBA for the color format? Or is that some consistency thing (e.g. with SFML's texture format, didn't check that right now).

This would allow passing 24 bit colors (without an alpha channel) as well, since colors with 0 alpha don't make any real sense (unless hiding some mask for post processing but those wouldn't be used with sf::Color IMO).

This would make this far more useful for people being used to classic HTML/CSS colors not defining an alpha channel:

sf::Color red(0xff0000);
sf::Color redtoo(0xffff0000);
sf::Color halfred(0x70ff0000);
Member

MarioLiebisch commented Oct 13, 2014

Hm... may I suggest using ARGB rather than RGBA for the color format? Or is that some consistency thing (e.g. with SFML's texture format, didn't check that right now).

This would allow passing 24 bit colors (without an alpha channel) as well, since colors with 0 alpha don't make any real sense (unless hiding some mask for post processing but those wouldn't be used with sf::Color IMO).

This would make this far more useful for people being used to classic HTML/CSS colors not defining an alpha channel:

sf::Color red(0xff0000);
sf::Color redtoo(0xffff0000);
sf::Color halfred(0x70ff0000);
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 13, 2014

Member

Even if rarely used, turning alpha = 0 to alpha = 0xFF looks like a dangerous ugly confusing thing 😕

And no, I don't want to change the order or components anyway.

Member

LaurentGomila commented Oct 13, 2014

Even if rarely used, turning alpha = 0 to alpha = 0xFF looks like a dangerous ugly confusing thing 😕

And no, I don't want to change the order or components anyway.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Oct 13, 2014

Contributor

This disallows the alpha of 0 completely, so it's rather bad, considering this is (in theory...) a 1:1 shortcut to the usual constructor (same order, same values, etc. just minus the alpha = 255 default argument).

CSS (Color Module 4 introduces 8 digit hex to specify alpha, among other things, so far we just had 6 hex digit RGB colors with no alpha possible, the new 8 hex digit ones are in RGBA order too, BTW) can do this because they control the parser, we don't know if we got 6 digits or if we got 8 and the first two were 0s.

If it really is a big issue to write 8 digits and not lose count you can make convention to type last two fs in caps if you don't care for them: red = 0xff0000FF or similar. It kind of sucks still and it's so bad that C++14 actually allows using ' to split number literals to make them more readable. 6 hex digits is not super more readable than 8 either.

Also your "halfred" is a bit wrong, 0x70 is 112, I'd expect "half" of range of 0..255 to mean something like 127 (0x7f). ;)

Also, why wouldn't postprocessing constant with 0 alpha be used with sf::Color? sf::Shader has overload that takes sf::Color so you could (and probably should, instead of normalizing it to 4 floats yourself) use your hex color with 0 alpha there instead of having to type sf:Color(r, g, b, 0) if you want, since this is the purpose of this "feature".

Contributor

FRex commented Oct 13, 2014

This disallows the alpha of 0 completely, so it's rather bad, considering this is (in theory...) a 1:1 shortcut to the usual constructor (same order, same values, etc. just minus the alpha = 255 default argument).

CSS (Color Module 4 introduces 8 digit hex to specify alpha, among other things, so far we just had 6 hex digit RGB colors with no alpha possible, the new 8 hex digit ones are in RGBA order too, BTW) can do this because they control the parser, we don't know if we got 6 digits or if we got 8 and the first two were 0s.

If it really is a big issue to write 8 digits and not lose count you can make convention to type last two fs in caps if you don't care for them: red = 0xff0000FF or similar. It kind of sucks still and it's so bad that C++14 actually allows using ' to split number literals to make them more readable. 6 hex digits is not super more readable than 8 either.

Also your "halfred" is a bit wrong, 0x70 is 112, I'd expect "half" of range of 0..255 to mean something like 127 (0x7f). ;)

Also, why wouldn't postprocessing constant with 0 alpha be used with sf::Color? sf::Shader has overload that takes sf::Color so you could (and probably should, instead of normalizing it to 4 floats yourself) use your hex color with 0 alpha there instead of having to type sf:Color(r, g, b, 0) if you want, since this is the purpose of this "feature".

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 13, 2014

Member

I would definitely make the conversion explicit. When you can pass any number (not just integers with 0x prefix) as a color, there are a lot of places where bugs can easily be introduced.

void DrawCircles(int howMany, const sf::Color& color);

// Caller doesn't remember signature
DrawCircles(0xaabbccdd, 4); // silent bug (not even a warning)

Even worse:

sf::Shader shader;
shader.setParameter("color", 0xaabbccdd); // guess which overload is called
Member

Bromeon commented Oct 13, 2014

I would definitely make the conversion explicit. When you can pass any number (not just integers with 0x prefix) as a color, there are a lot of places where bugs can easily be introduced.

void DrawCircles(int howMany, const sf::Color& color);

// Caller doesn't remember signature
DrawCircles(0xaabbccdd, 4); // silent bug (not even a warning)

Even worse:

sf::Shader shader;
shader.setParameter("color", 0xaabbccdd); // guess which overload is called
@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 13, 2014

Member

I'd also vote for the explicit constructor. Although in case of the shader parameter you could still just forget about it (and you wouldn't receive a warning or error).

Member

MarioLiebisch commented Oct 13, 2014

I'd also vote for the explicit constructor. Although in case of the shader parameter you could still just forget about it (and you wouldn't receive a warning or error).

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Oct 13, 2014

Contributor

The first "example" is very meh.
The shader one is pretty wonderful though... It'll conflict with float setter, thanks to gratuitous overloads in setParameter (10 of them). :D
I guess we have to make this feature useless and redundant after all.
@MarioLiebisch You will get a vague error about GL_INVALID_OPERATION in sf::err() when you try to set a variable that is vec4 in glsl to a single float value.

Contributor

FRex commented Oct 13, 2014

The first "example" is very meh.
The shader one is pretty wonderful though... It'll conflict with float setter, thanks to gratuitous overloads in setParameter (10 of them). :D
I guess we have to make this feature useless and redundant after all.
@MarioLiebisch You will get a vague error about GL_INVALID_OPERATION in sf::err() when you try to set a variable that is vec4 in glsl to a single float value.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Oct 13, 2014

Member

Well... if you are really paranoid about setParameter, templated versions with 1, 2, 3 and 4 parameters could be added that are made to fail compiling on purpose (via C++03 static assert or a private method). This will make sure that no implicit conversions can be performed when calling setParameter. I did this with GLS although using = delete. This might also solve an issue we had with #538.

Member

binary1248 commented Oct 13, 2014

Well... if you are really paranoid about setParameter, templated versions with 1, 2, 3 and 4 parameters could be added that are made to fail compiling on purpose (via C++03 static assert or a private method). This will make sure that no implicit conversions can be performed when calling setParameter. I did this with GLS although using = delete. This might also solve an issue we had with #538.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Oct 13, 2014

Member

The first "example" is very meh.

Well, I have already wasted a lot of time debugging similar mistakes (sometimes introduced by other people, over longer time, and not rarely in the depths of code where no one would suspect them), so I began to appreciate APIs that are a bit less convenient but more type-safe ;)

Well... if you are really paranoid about setParameter, templated versions with 1, 2, 3 and 4 parameters could be added that are made to fail compiling on purpose

setParameter is only an example that shows the problematic very well, there are enough cases... And before beginning to fix other parts of the API to comply with this feature, I'd rather add an explicit constructor ;)

The sf::Shader API could potentially be improved nonetheless, but let's keep that for the corresponding forum thread.

Member

Bromeon commented Oct 13, 2014

The first "example" is very meh.

Well, I have already wasted a lot of time debugging similar mistakes (sometimes introduced by other people, over longer time, and not rarely in the depths of code where no one would suspect them), so I began to appreciate APIs that are a bit less convenient but more type-safe ;)

Well... if you are really paranoid about setParameter, templated versions with 1, 2, 3 and 4 parameters could be added that are made to fail compiling on purpose

setParameter is only an example that shows the problematic very well, there are enough cases... And before beginning to fix other parts of the API to comply with this feature, I'd rather add an explicit constructor ;)

The sf::Shader API could potentially be improved nonetheless, but let's keep that for the corresponding forum thread.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 29, 2014

Member

What's the status of this PR/discussion?

Member

eXpl0it3r commented Dec 29, 2014

What's the status of this PR/discussion?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 29, 2014

Member

I'd say that we should make this constructor explicit for now, we can always make it implicit in the future if it really bothers users.

Forget about the counterpart (Color::toInteger()), it's probably not useful.

Member

LaurentGomila commented Dec 29, 2014

I'd say that we should make this constructor explicit for now, we can always make it implicit in the future if it really bothers users.

Forget about the counterpart (Color::toInteger()), it's probably not useful.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 29, 2014

Member

I'm also in favor of an explicit constructor, because hiding sf::Color isn't very useful and can lead to quite some confusion, e.g. people assuming sf::Color is the same as Uint32 or casting Uint32 to sf::Color.
If you want to type less, you can then always use the C++11 uniform initialization braces.

Member

eXpl0it3r commented Dec 29, 2014

I'm also in favor of an explicit constructor, because hiding sf::Color isn't very useful and can lead to quite some confusion, e.g. people assuming sf::Color is the same as Uint32 or casting Uint32 to sf::Color.
If you want to type less, you can then always use the C++11 uniform initialization braces.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 29, 2014

Member

I'm also in favor of explicit conversions rather than silent ones. An explicit constructor is more consistent with the current API but a (good) alternative would be a factory function.

Personally I would include toInteger(): we would be able to load a color from a file but not serialise it with the proposed API (well, we can, but it's not a symmetrical API...).

@eXpl0it3r using brace initialisation doesn't work with explicit constructors. http://ideone.com/cVRjvq ;-)

Member

mantognini commented Dec 29, 2014

I'm also in favor of explicit conversions rather than silent ones. An explicit constructor is more consistent with the current API but a (good) alternative would be a factory function.

Personally I would include toInteger(): we would be able to load a color from a file but not serialise it with the proposed API (well, we can, but it's not a symmetrical API...).

@eXpl0it3r using brace initialisation doesn't work with explicit constructors. http://ideone.com/cVRjvq ;-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Dec 29, 2014

Member

@mantognini Ah right! 😄

I'm not sure how many people will actually use this for serialization, because having some numbers written in a non-hex representation feels kind of odd (color = 4278190335), as such you'd end up having to write converters anyways at which point the new constructor or the toInteger() function get kind of useless.

I personally see this feature only as a convenience when writing code, but maybe I'm short sighted.

Member

eXpl0it3r commented Dec 29, 2014

@mantognini Ah right! 😄

I'm not sure how many people will actually use this for serialization, because having some numbers written in a non-hex representation feels kind of odd (color = 4278190335), as such you'd end up having to write converters anyways at which point the new constructor or the toInteger() function get kind of useless.

I personally see this feature only as a convenience when writing code, but maybe I'm short sighted.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 29, 2014

Member

@eXpl0it3r std::cout << "0x" << std::hex << 42; will print something nice. ;-)

But readability of the output was not my main point. If we have a config file (that no human will ever read), and we are able to load color from a single integer with one call to the API, then it strange that we are not able to do the opposite operation with one call of the API.

Member

mantognini commented Dec 29, 2014

@eXpl0it3r std::cout << "0x" << std::hex << 42; will print something nice. ;-)

But readability of the output was not my main point. If we have a config file (that no human will ever read), and we are able to load color from a single integer with one call to the API, then it strange that we are not able to do the opposite operation with one call of the API.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Dec 29, 2014

Contributor

It HAS to be explicit because of the overloads in sf::Shader, no question about it.

Contributor

FRex commented Dec 29, 2014

It HAS to be explicit because of the overloads in sf::Shader, no question about it.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 29, 2014

Member

This serialization "issue" is less obvious than with sf::IpAddress, because sf::Color can already be encoded as 4 integer bytes with its RGBA components taken individually. So you can achieve the same thing without the toInteger() function, it's just slightly more verbose.

But yes, we can add it for consistency.

It HAS to be explicit because of the overloads in sf::Shader, no question about it.

Since there's no setParameter(unsigned int), the call is ambiguous and triggers a compiler error, right? So this is the same effect and solution as if the constructor was explicit, so this is not a strong reason for making it explicit.

Member

LaurentGomila commented Dec 29, 2014

This serialization "issue" is less obvious than with sf::IpAddress, because sf::Color can already be encoded as 4 integer bytes with its RGBA components taken individually. So you can achieve the same thing without the toInteger() function, it's just slightly more verbose.

But yes, we can add it for consistency.

It HAS to be explicit because of the overloads in sf::Shader, no question about it.

Since there's no setParameter(unsigned int), the call is ambiguous and triggers a compiler error, right? So this is the same effect and solution as if the constructor was explicit, so this is not a strong reason for making it explicit.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Dec 30, 2014

Contributor

@LaurentGomila Wrong - I just merged this feature to latest master and recompiled my SFML.
And this code has just compiled for me on Fedora 21 with both GCC Red Hat 4.9.2-1 and clang 3.5.0 RELEASE_350/final.

#include <SFML/Graphics.hpp>

int main()
{
    sf::Color hehe2(0xff0000ff); //just to verify we DO have that feature
    sf::Shader shader;
    shader.setParameter("hehe", 0xff0000ff); //what does this call?
}

They just doesn't care for implicit constructors when there is a single built-in conversion available.
See for yourself: http://ideone.com/B4dJ6L
This means, that passing a literal like 0xff0000ff to sf::Sprite::setColor or sf::Vertex::Vertex constructor works as sf::Color via this implicit constructor, but passing it to sf::Shader::setParameter works as float.
P.S. A compile error would be a bit silly anyway. Everything works when given an unsigned.. except sf::Shader::setParameter.
P.P.S. You know me ;) so you should have known this has to be a big thing if I gave up fighting against the stupidity of making this explicit so easily thanks to it.

Contributor

FRex commented Dec 30, 2014

@LaurentGomila Wrong - I just merged this feature to latest master and recompiled my SFML.
And this code has just compiled for me on Fedora 21 with both GCC Red Hat 4.9.2-1 and clang 3.5.0 RELEASE_350/final.

#include <SFML/Graphics.hpp>

int main()
{
    sf::Color hehe2(0xff0000ff); //just to verify we DO have that feature
    sf::Shader shader;
    shader.setParameter("hehe", 0xff0000ff); //what does this call?
}

They just doesn't care for implicit constructors when there is a single built-in conversion available.
See for yourself: http://ideone.com/B4dJ6L
This means, that passing a literal like 0xff0000ff to sf::Sprite::setColor or sf::Vertex::Vertex constructor works as sf::Color via this implicit constructor, but passing it to sf::Shader::setParameter works as float.
P.S. A compile error would be a bit silly anyway. Everything works when given an unsigned.. except sf::Shader::setParameter.
P.P.S. You know me ;) so you should have known this has to be a big thing if I gave up fighting against the stupidity of making this explicit so easily thanks to it.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 30, 2014

Member

He he... Thanks for confirming that you were right about this. So this is indeed a strong argument against the implicit constructor -- until we possibly change the sf::Shader API.

Member

LaurentGomila commented Dec 30, 2014

He he... Thanks for confirming that you were right about this. So this is indeed a strong argument against the implicit constructor -- until we possibly change the sf::Shader API.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Dec 30, 2014

Contributor

Believe me, I'd love to be wrong about this and defend implicity of that constructor (with coarse langauge if necessary) but I'm sadly right.
So: the case for THIS piece of code is closed - make it explicit and merge.

P.S. I did that exact check vs. sf::Shader::setParameter months ago, when I originally have given up the idea of implict constructor. Who do you think I am? ;)

P.P.S. Addding (or not) the symetric getter is another thing which can be discussed, decided and implemented separately from this IMO.

Contributor

FRex commented Dec 30, 2014

Believe me, I'd love to be wrong about this and defend implicity of that constructor (with coarse langauge if necessary) but I'm sadly right.
So: the case for THIS piece of code is closed - make it explicit and merge.

P.S. I did that exact check vs. sf::Shader::setParameter months ago, when I originally have given up the idea of implict constructor. Who do you think I am? ;)

P.P.S. Addding (or not) the symetric getter is another thing which can be discussed, decided and implemented separately from this IMO.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Dec 30, 2014

Member

But yes, we can add it for consistency.

Yes, my point exactly (but better explained!).

@FRex, why rushing that fast? It's not like SFML 2.3 is due tomorrow. ;)

Unless someone has an argument against toInteger(), I suggest we add it to this commit.

Member

mantognini commented Dec 30, 2014

But yes, we can add it for consistency.

Yes, my point exactly (but better explained!).

@FRex, why rushing that fast? It's not like SFML 2.3 is due tomorrow. ;)

Unless someone has an argument against toInteger(), I suggest we add it to this commit.

@FRex

This comment has been minimized.

Show comment
Hide comment
@FRex

FRex Dec 30, 2014

Contributor

@mantognini Because I find it stupid how long it takes for minute things like this, it's over 3 months at this point (the idea originally appeared on forum in September) and already lost my interest once because of that, if exploiter didn't bump this I'd not care in the least.

Contributor

FRex commented Dec 30, 2014

@mantognini Because I find it stupid how long it takes for minute things like this, it's over 3 months at this point (the idea originally appeared on forum in September) and already lost my interest once because of that, if exploiter didn't bump this I'd not care in the least.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Dec 30, 2014

Member

Because I find it stupid how long it takes for minute things like this, it's over 3 months at this point

This is mostly because we were busy finalizing many non-code things for the SFML 2.2 release... Now you can expect things to go slightly faster ;)

Member

LaurentGomila commented Dec 30, 2014

Because I find it stupid how long it takes for minute things like this, it's over 3 months at this point

This is mostly because we were busy finalizing many non-code things for the SFML 2.2 release... Now you can expect things to go slightly faster ;)

@eXpl0it3r eXpl0it3r removed the s:unassigned label Jan 8, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 11, 2015

Member

I rebased onto master and implemented toInteger(). Let me know if there's something we need to change.

Member

eXpl0it3r commented Feb 11, 2015

I rebased onto master and implemented toInteger(). Let me know if there's something we need to change.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Feb 13, 2015

Member

👍

Member

TankOs commented Feb 13, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 16, 2015

Member

Added explicit and renamed the parameter to integer, but I'd still like to get some feedback on that!

Member

eXpl0it3r commented Feb 16, 2015

Added explicit and renamed the parameter to integer, but I'd still like to get some feedback on that!

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 16, 2015

Member

About the integer name, I think color was better for the simple fact that its type already contains this information. It's also pretty obvious and common to have toInteger function and a corresponding constructor/factory so I expect people to make the link themselves. However, we could add a \see in the doc to link the two, too.

Member

mantognini commented Feb 16, 2015

About the integer name, I think color was better for the simple fact that its type already contains this information. It's also pretty obvious and common to have toInteger function and a corresponding constructor/factory so I expect people to make the link themselves. However, we could add a \see in the doc to link the two, too.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Feb 16, 2015

Member

I'm with @mantognini, the name "color" is more expressive. "integer" describes the type and is similar to Hungarian notation. ;-) Just imagine the case for all elements:

Color( Uint32 integer0, Uint32 integer1, Uint32 integer2, Uint32 integer3 );
Color( Uint32 r, Uint32 g, Uint32 b, Uint32 a );

Which one is more expressive? :P

Member

TankOs commented Feb 16, 2015

I'm with @mantognini, the name "color" is more expressive. "integer" describes the type and is similar to Hungarian notation. ;-) Just imagine the case for all elements:

Color( Uint32 integer0, Uint32 integer1, Uint32 integer2, Uint32 integer3 );
Color( Uint32 r, Uint32 g, Uint32 b, Uint32 a );

Which one is more expressive? :P

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 16, 2015

Member

So what about toInteger() then?

Member

eXpl0it3r commented Feb 16, 2015

So what about toInteger() then?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 16, 2015

Member

toInteger is fine with me: It explicitly states we want a compact representation of the color.

Member

mantognini commented Feb 16, 2015

toInteger is fine with me: It explicitly states we want a compact representation of the color.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Feb 17, 2015

Member

Same. 👍

Member

TankOs commented Feb 17, 2015

Same. 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 18, 2015

Member

Reverted to the color parameter.

Member

eXpl0it3r commented Feb 18, 2015

Reverted to the color parameter.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 20, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 20, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Feb 20, 2015

@eXpl0it3r eXpl0it3r self-assigned this Feb 20, 2015

@eXpl0it3r eXpl0it3r merged commit 1f2bc14 into master Feb 23, 2015

@eXpl0it3r eXpl0it3r deleted the feature/color_ctor branch Feb 23, 2015

@zsbzsb zsbzsb referenced this pull request Mar 14, 2015

Closed

Track releasing 2.3 #70

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment