Added a new constructor that takes single Uint32 to Color

[TankOs]

Added a new constructor that takes single Uint32 to Color

[LaurentGomila]

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]

Yep, I agree to everything. ;) @FRex

[FRex]

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]

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]

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 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]

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]

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]

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]

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]

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]

What's the status of this PR/discussion?

[LaurentGomila]

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]

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]

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]

@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]

@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]

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

[LaurentGomila]

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]

@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]

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]

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]

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]

@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]

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]

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

[LaurentGomila]

I would rather write it as (r << 24) | (g << 16) | (b << 8) | a to make compilers happy.

[MarioLiebisch]

Not 100% sure right now, but does this even work without casting, considering the components are Uint8?

[eXpl0it3r]

Updated!

I haven't gotten any warnings with -Wall -Wextra.

[MarioLiebisch]

Just tried it, there's an implicit conversion happening.

unsigned char a = 5;
std::cout << (a << 8) << std::endl;

Result is indeed 1280.

[mantognini]

The rules are rather confusing with << operators: the return type is the (promoted) left operand's type which is promoted to int here because of the right operand being int. So, from my understanding of the standard it's safe as is.

But it's quite tricky and maybe I'm wrong and the compiler should emit a warning.. 😁

[TankOs]

👍

[Bromeon]

Should be explicit.

And maybe another name for the parameter? "integer" would be consistent with toInteger.

[eXpl0it3r]

Right forgot to adapt that.

[eXpl0it3r]

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

[mantognini]

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]

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]

So what about toInteger() then?

[mantognini]

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

[TankOs]

Same. 👍

[eXpl0it3r]

Reverted to the color parameter.

[eXpl0it3r]

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

[mantognini]

It would be really nitpicking but shouldn't it be 32-bit (with a dash)? Do we care?

[eXpl0it3r]

Done

[mantognini]

👍