SFML community forums

General => Feature requests => Topic started by: FRex on September 20, 2014, 12:12:19 am

Title: small & stupid api changes: radians, vectors, colors
Post by: FRex on September 20, 2014, 12:12:19 am
1.
setRadians and getRadians functions for angles in sf::Transformable. It would be backwards compatible, would not break the API, would let everyone use degrees OR radians, whichever they wish.
Yes, I know there is this thread, but it seems that it degraded into pointless argument about what is more 'friendly'...
http://en.sfml-dev.org/forums/index.php?topic=205.0
But yeah, sure - radians are less friendly and SFML is not hardcore-math-knowing-programmer's library. Only more reason for this change - it's very OK that 'default' unit is degrees and radians have to be explicitly present by name in functions that use them.
Yes, public API can do that but it's (literally) few lines of SFML code to save anyone working with radians from few lines of their own code, having to include the header with that code and then calling that single function differently than all other functions on all SFML transformables. It'll allow consistency in common user code. Radians are less common and friendly than angles but still common. This isn't as obscure and user specific as something like asking for allowing sin+cos for rotation would be.
It's a dumb change code wise, it won't break anything that now works with SFML and it'll make radians work out of the box.


2.
Small backwards compatible convenience change to sf::Vector2 constructor, instead of what we have now for casting from different SFML vector types (https://github.com/SFML/SFML/blob/master/include/SFML/System/Vector2.inl#L47) we could have this:
template <typename T>
template <typename U>
inline Vector2<T>::Vector2(const U& vector) :
x(static_cast<T>(vector.x)),
y(static_cast<T>(vector.y))
{
}
 
which just requires x and y fields in the class, not that it's SFML own class with another type, so vector from another library (say Box2D...) can be passed and work out of the box. This constructor is explicit anyway so it's pretty OK to do that IMO. It really makes sense that vector can be constructed out of something that has x and y in it (in particular - another vector with x and y in it).


3. Color should really have implicit constructor from sf::Uint32 (and possibly operator= too).
It's a bit annoying that I have to write that function myself and use it instead of just doing sf::Color(0xff0000ff) (or even passing number directly to function needing a color). Color and sf::Uint32 actually have same memory layout even, so you could memcpy from one to another so it's really funny there is no function to do that.
There is even implicit constructor from single UTF32 character to sf::String for convenience (which once bit someone in the ass) so why no that too?
HTML, GIMP, Ogre and probably many other use hex numbers like that already. Irrlicht has that kind of constructor so you can pass number to some of its' functions that need a color (some need float 0..1 colors, can't pass the number there).
It's really not at all obscure or unknown and it's really unlikely someone will pass a number instead of color accidentally and wonder what went wrong.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Hapax on September 20, 2014, 01:36:03 am
I agree with all three of these with varying degrees (pun not intended) of strength.

1. If it's short, simple, doesn't break the API, expands its flexibility, and seems commonly wanted, I don't see why this wouldn't be included.

2. I've often found that I'm "converting" between a custom vector to an SFML one which requires manually assigning each member so I can see how this would be useful although I'm not good enough to know if this code solves everything without causing any problems.

3. This is strange because I've never even previously considered this but, now you mention it, it's something I've been missing for quite some time. Photoshop also uses hex numbers for colours; I thought I'd mention that one since you missed it off your list  ;) I must say, though, it would require still being able to assign the colours without the alpha part, and I'm not sure how that would be done here.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: eXpl0it3r on September 20, 2014, 01:53:47 am
3. This is strange because I've never even previously considered this but, now you mention it, it's something I've been missing for quite some time. Photoshop also uses hex numbers for colours; I thought I'd mention that one since you missed it off your list  ;) I must say, though, it would require still being able to assign the colours without the alpha part, and I'm not sure how that would be done here.
Notice that FRex proposed an overload for sf::Uint32 which has 8 hex digits, 0xRRGGBBAA (R = Red, G = Green, B = Blue, A = Alpha).
I guess it could make things a tiny bit easier as oposed to using:
sf::Color c(0xff, 0x00, 0xee, 0xaa);
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Hapax on September 20, 2014, 01:58:10 am
Notice that FRex proposed an overload for sf::Uint32 which has 8 hex digits, 0xRRGGBBAA (R = Red, G = Green, B = Blue, A = Alpha).
I guess it could make things a tiny bit easier as oposed to using:
sf::Color c(0xff, 0x00, 0xee, 0xaa);
I noticed it was for 8 hex digits, and you're right that it would make it easier to use than specifying the four components separately. However, it wouldn't allow to specify only the colour components.
That said, I'd be all for the four components in one number. I'd have no trouble getting into an automatic habit of typing an extra two fs for a standard (fully opaque) colour  :)
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Laurent on September 20, 2014, 10:30:05 am
1. Bloated API. I don't think anybody will care about writing these two lines of code if they really need this feature
inline float deg(float rad) {return ...}
inline float rad(float deg) {return ...}

2. I haven't got any strong argument against it, but it feels like a hack, not a clean feature. It would only work for half of the other vector classes that exist in the wild, and may even produce hard to understand errors with some of them. Example: in Qt, vectors have x and y members but they are functions; I let you deduce what kind of error the compiler would output in this case.

3. Looks ok.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on September 20, 2014, 04:05:41 pm
1. It's really minor bloat (if any) and sf::Transformable is a convenience class anyway.
It's still annoying to write your own code for something that common, it makes user code inconsistent with every other sf::Transformable use ever and many other things that you could write yourself as global functions using public sf::Transformable API (rotate, move, scale, Vector2f + two floats overloads of many functions) are in the class for convenience reasons already (I know Vector2f + two float overloads are supposed to go out but they somehow went in while they made less sense than this addition).
Right now we can do every common thing using built in methods except use radians.
You can't even rotate sprite to face mouse position without looking up a magic constant (either degrees per radian or PI, since M_PI is non standard) because atan2 returns radians. It's really annoying that a "simple" library can't do that out of the box.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Laurent on September 20, 2014, 04:41:25 pm
Quote
1. It's really minor bloat (if any) and sf::Transformable is a convenience class anyway.
It's not just sf::Transformable, it's everything that takes and returns angles.

Quote
I know Vector2f + two float overloads are supposed to go out
This is the direction we are taking. A lot of useless overloads were removed in SFML 2, and we'd rather remove others than add new ones.

Quote
You can't even rotate sprite to face mouse position without looking up a magic constant (either degrees per radian or PI, since M_PI is non standard) because atan2 returns radians. It's really annoying that a "simple" library can't do that out of the box.
Degrees to radians and the other way round are really common and simple operations, I've never been annoyed by them in any graphics program that I wrote with SFML.

If we ever add something to SFML, it would most likely be the two conversion functions that I posted previously. So it would be isolated and not require to add ugly overloads everytime we add a function that deals with angles.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on September 20, 2014, 05:55:20 pm
There is something else that uses angles in SFML?
Vector shortcuts save you typing sf::Vector2f(), these ones save you a function call, looking up the constant online and writing the two functions (and then including them like crazy every single time you want to use a text, a sprite, a shape, etc.).
And there still are convenience functions like scale, move and rotate, do they go out too? They are even easier to write than degtorad and radtodeg.
Does everything go out except bare minimum? This is like heading towards "simple to read, simple to write, hard as f**k to use because you need to write 5 helpers for everything" direction. Simple doesn't have to mean poor and bare bones.

Quote
Degrees to radians and the other way round are really common and simple operations,
Now it's this, but before it often would be that "pi and radians are scary" and "degrees are more friendly". Seriously...

This has been mentioned times and times again by people:
http://en.sfml-dev.org/forums/index.php?topic=315.0
http://en.sfml-dev.org/forums/index.php?topic=205.0
http://en.sfml-dev.org/forums/index.php?topic=12146.0
http://en.sfml-dev.org/forums/index.php?topic=313.0
http://en.sfml-dev.org/forums/index.php?topic=14840.0
In 2008 you already said everyone is complaining about it missing.

This is really a wider problem (one of many) with SFML - mentality on new features: some things go in, some don't and there is no consistency about it other than someone having a gut feeling and/or eyeballing it. Up vector was pretty wonderful in that regard, few lines + oscillating between "I'll do it soon" and "we don't need it" for few years:
http://en.sfml-dev.org/forums/index.php?topic=2782.0
http://en.sfml-dev.org/forums/index.php?topic=6057.0
http://en.sfml-dev.org/forums/index.php?topic=11671.0
http://en.sfml-dev.org/forums/index.php?topic=14566.0

Concerning API bloat and DIY philosophy I honestly though I'd have 3. dismissed in record time by you. It's new implicit overload, it can be done by public API super easily (easier than radians) and it can be done with C++11 user literals easily too.
But somehow it's OK because other libraries, graphic programs and HTML, QML and so on do it too. ???
(Sadly that others-do-it reasoning doesn't work with adding radian helpers).
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Nexus on September 20, 2014, 06:31:24 pm
1. Concerning get/setRadians, it would lead to a duplication of all angle-related methods in the API without adding new functionality. I don't consider this a good idea for the same reason we have no getSeconds/getMilliseconds/... in every place where time is used, or getX/Y for vectors, or getR/G/B/A for colors: combinatorial explosion. In my opinion, a more promising approach would be a dedicated type for angles (http://en.sfml-dev.org/forums/index.php?topic=12146.msg84751#msg84751), similar to sf::Time. Like that, type conversion and places where the type is used can be kept orthogonal.

2. Adding a template constructor is problematic because it accepts anything. If a type doesn't match, the compiler error will only appear in the constructor's implementation and not in overload resolution; it is usually more difficult to understand. I also think that providing support for only "x" and "y" is somewhat arbitrary... There's also no backwards conversion.

3. I wonder whether a constructor itself is the most expressive way... an alternative is a static function (named constructor idiom). What about reverse conversion (from colors to integers)?
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Hapax on September 20, 2014, 07:55:08 pm
dedicated type for angles[/url], similar to sf::Time
I agree with this if only to keep consistency within SFML. sf::Time can seem a little extra work sometimes but I like the forced type thing to help reduce errors.
As for not including degrees to radians functions because they're short and easy, think about the sf::Time functions. Divided by 1000? Thanks  :P

3. I wonder whether a constructor itself is the most expressive way... an alternative is a static function (named constructor idiom). What about reverse conversion (from colors to integers)?
redColor = sf::Color(#ff0000ff);
redInt = redColor.asInteger();
Possibles?
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Laurent on September 20, 2014, 09:20:25 pm
Don't complain about my opinion. Now that SFML is driven by a team, it's just my own personal opinion and nothing more ;)

If everybody else is ok about 1. 2. and 3. then they will all be part of SFML, no matter what I say.

I just wanted to answer to something important in my opinion: there's a difference between adding a new feature to a class, and adding overloads related to a type everywhere. sf::Color, sf::String, ... I'd be happy to add new stuff to them, but I would strongly disagree to add overloads of every function that takes or returns them. That's why I like 3. but not 1.

And no, I don't like 3. just because other libraries do it. I thought you knew me better :P
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Hiura on September 21, 2014, 04:05:25 am
Regarding 1, my opinion is the following. In order to avoid having a bloated API and possibly non consistent API the only solution I see is the addition of `sf::Angle` to represent angle regardless of radian or degree system. However, even if such class is relatively simple to implement we would probably provide a very minimalistic class (like we do with `sf::Vector2` for example) which would not be useful in practice to a lot of people since they would probably want an overloaded operator for their own type or a way to convert it to their own type of mathematical vector or simply because they already have such class in their code (this is from a personal experience). Instead I'd recommend keeping one type of angle and using primitive type as we currently do.

Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Foaly on September 22, 2014, 03:06:40 pm
Just wanted to say that I also think that 3 is a good idea!
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: cepro on September 23, 2014, 02:09:05 am
Indeed #3 would be a nice feature especially for people that are used to web development.

On another subject, it would also be great to add a setAlpha function to SFML graphic entities so one would avoid writing code like this :
sf::Uint8 alpha = 100;
const sf::Color& c = text.getColor();// text is an sf::Text
text.setColor(sf::Color(c.r, c.g, c.b, alpha));

Or maybe there is another straightforward way of writing this code that I'm not aware of?
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: dabbertorres on September 23, 2014, 03:04:42 am
sf::Color newColor = text.getColor();
newColor.a = newAlpha;
text.setColor(newColor);

Looks a little cleaner to me.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Nexus on September 23, 2014, 06:54:39 am
Or maybe there is another straightforward way of writing this code that I'm not aware of?
Global function:
template <typename T>
void setAlpha(T& object, sf::Uint8 alpha)
{
    sf::Color color = object.getColor();
    color.a = alpha;
    object.setColor(color);
}

Works now with any class providing access to color, not just sf::Text.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: cepro on September 23, 2014, 12:51:39 pm
Thank you @dabbertorres and @Nexus, well that looks a bit cleaner :) It will always need to copy the object's color thought but hey it does the job and it's clean. :)
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Tank on September 26, 2014, 07:19:43 am
I like 1. but in a way Nexus suggested. It's a very clean approach.

I'm not sure about 2. Like others said, it would only support a subset of possible structures. If the source uses a different naming convention, it wouldn't work already. Thus the constructor isn't generic.

3. sounds like a very good idea, as using single values for colors is very common.  I wouldn't use named constructors (static functions) here, but only a constructor that takes sf::Uint32, representing RGBA.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: achpile on September 26, 2014, 01:27:49 pm
May be it will be interesting to implement sf::Angle class with methods, returning Deg value, Rad value and sf::Vector2f value. for example

inline sf::Vector2f sf::Angle::toVector(float len = 1.0f) {
    return sf::Vector2f(cos(m_angle), sin(m_angle)) * len;
};
 

But it will be a class with only 1 member (m_angle), so it's better to make it just a functions. And I think everyone can implement what he wants. And now I don't even know why I've written this reply :D
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on October 10, 2014, 05:49:16 pm
Since this thread died here's a patch:
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: wintertime on October 12, 2014, 01:29:49 pm
The problem I see with doing that is: there is no standardized way of packing rgba into an int.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on October 12, 2014, 03:55:25 pm
Let's be serious now - SFML uses R8G8B8A8 exclusively, not ARGB or BGRA or any other format so it's natural to expect 4 bytes packed into int in that order.
By the logic you use someone would say that RGBA is not that standard channel order either and say it's not intuitive to have the Color(Uint8 r, Uint8 g, Uint8 b, Uint8 a) constructor too (because he has to read the docs/names of arguments to know the order, and he expected it to be ARGB).
Besides RGBA is (arguably) the most intuitive/popular order (so even newbs will get it) and is basically same as HTML/CSS hex with two more digits at end (for alpha).
This also doesn't get in the way for any current users because it's just a shortcut, there is no danger someone will accidentally pass unsigned there and be confused what happened, they need to read the doc to know it's there before using it and then, seeing the docs comment, they will know it goes in RGBA order.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: wintertime on October 12, 2014, 10:14:51 pm
Even if someone uses r,g,b,a it can still result in a different byte order depending on the endianness:
union {
  Uint32 rgba;
  sf::Color sfcolor;
} mycolor;

mycolor mc;
mc.sfcolor=sf::Color::Green;
sf::Color color(mycolor.rgba);
if(color==sf::Color::Blue)
  std::cout << "argh" << endl;
 
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Laurent on October 12, 2014, 10:56:49 pm
It can result in a different byte order in memory, but we don't care. RGBA usually refers to the "logical" byte order (R is the MSB, then G, then B, and A is the LSB).

sf::Uint32 color = 0x11223344;
sf::Uint8 r = (color >> 24) & 0xFF; // 0x11
sf::Uint8 g = (color >> 16) & 0xFF; // 0x22
sf::Uint8 b = (color >>  8) & 0xFF; // 0x33
sf::Uint8 a = (color >>  0) & 0xFF; // 0x44

... which doesn't depend on the machine's endianness.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on October 12, 2014, 11:16:09 pm
You declared unnamed global union named mycolor, not a new union type called mycolor.
This is one of the reasons it won't compile.
A new union type named mycolor would be:
union mycolor{
  Uint32 rgba;
  sf::Color sfcolor;
};
Also, this is quite an artificial problem, no wants to put sf::Color and sf::Uint32 into an union like you shown, we only intend to add the new constructor.
And actually, an union containing sf::Color will not compile, in C++98 it has a constructor which is forbidden, in C++11 (which relaxed the rules) it has non trivial constructor so it won't compile either.

IF you mean that this byte packing doesn't work on big endian then you are not right either, look:
#include <cstdio>

#define rf 0xff000000
#define gf 0x00ff0000
#define bf 0x0000ff00
#define af 0x000000ff

typedef unsigned char u8;
typedef unsigned u32;

class Color
{
public:

    Color(u8 r, u8 g, u8 b, u8 a = 255):
    r(r), g(g), b(b), a(a) {}
   
    Color(u32 col):
    r((col & rf) >> 24), g((col & gf) >> 16), b((col & bf) >> 8), a(col & af) {}

    u8 r, g, b, a;
};

int lilend()
{
    int i = 1;
    return (*(char *)&i) == 1;
}

void printcol(Color c)
{
    std::printf("r = %x, g = %x, b = %x, a = %x\n", c.r, c.g, c.b, c.a);
}

int main()
{
    std::printf("endian lil = %d\n", lilend());
    std::printf("sizeof u32 = %d sizeof u8 = %d\n", sizeof(u32), sizeof(u8));
    Color c(0x00ff00ff);
    std::printf("next line is 0, ff, 0, ff\n");
    printcol(c);
    c.r = 255;
    c.g = 0;
    std::printf("next line is ff, 0, 0, ff\n");
    printcol(c);
}

I ran this code on few machines (since I can't get full SFML there, we use equivalent code like one we intend to use in sf::Color).

This is from an old machine running 32 bit (at least I think so because size of pointer is 4) Sun-OS 5.10 on big endian CPU (it's sparc I think, not sure now, but definitely big endian...).
endian lil = 0
sizeof u32 = 4 sizeof u8 = 1
next line is 0, ff, 0, ff
r = 0, g = ff, b = 0, a = ff
next line is ff, 0, 0, ff
r = ff, g = 0, b = 0, a = ff

This is from my local machine, which is obviously standard Intel CPU, 64 bit capable but running 32 bit Fedora 20 with little endian.
endian lil = 1
sizeof u32 = 4 sizeof u8 = 1
next line is 0, ff, 0, ff
r = 0, g = ff, b = 0, a = ff
next line is ff, 0, 0, ff
r = ff, g = 0, b = 0, a = ff

I by mistake ran it on a multicore FreeBSD on AMD64 (also little endian) and result was same as on my machine.

As you see, endian and bitness doesn't affect this kind of thing, the constructor itself works as intended, we can use the color normally, etc. and unions are non issue, since sf::Color can't be put in one anyway.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: wintertime on October 12, 2014, 11:49:11 pm
Using your new contructor with a hex literal will obviously work like you want.

Doing an unsafe pointercast or using such a union, setting bytes first, then as a misoptimization reading the whole as an Uint32 would reverse it on little endian. And sure one should not do such as it depends on undefined behaviour, it was just a tiny example to demontrate its not needed to willingly use abgr and still get data in reverse order, to show it needs good documentation.

PS: Yes, I should not have shortened the example by replacing the struct I had there first with sf::Color and possibly have tried to compile it beforehand. :-[
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on October 13, 2014, 12:07:31 am
Quote
using such a union
As I said above, you can't use such an union with sf::Color.

What does the fact someone can get wrong result if they (mis)use an union or reinterpret cast on one endian or the other have to do with this feature and this implementation of it?
As Laurent said, by RGBA order we mean it in "logical"/"human" order, the "first" byte is the first one you write when typing the number by hand (so the one under 0xff000000 mask regardless of the fact if it's last or first in RAM) and so on.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Laurent on October 13, 2014, 07:57:18 am
Please can we stop this useless discussion? The feature is perfectly valid, has been accepted and is just waiting to be added to the code...
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: FRex on October 13, 2014, 09:48:50 am
Go ahead then, copy the patch I gave into a file and git am it.
We can add the color -> unsigned member function later if needed and it's less important than this constructor anyway.
Title: Re: small & stupid api changes: radians, vectors, colors
Post by: Tank on October 13, 2014, 12:04:01 pm
Thanks for the patch. Maybe it's possible to send in a GitHub pull request next time, as that eases the process a bit. For now, here's the PR: https://github.com/SFML/SFML/pull/722