Some minor issues with sfColor/sfImage

[s_p_oneil]

1) sfColor::ToRGBA returns 0xFFFF0000, which is backwards. Whenever I've specified RGB values in hex, I've always used FF0000 for red, never 0000FF.


Copied-n-pasted straight from this forum's help:
Font color: text[\/color]  Tip: you can also use color=#FF0000


2) When I call sfImage::CreateMaskFromColor(), I'd like to pass it 0 for black (getting it to use the UINT constructor above). However, it fails unless I set the alpha channel to 255. Since I'm trying to create an alpha mask, I would think it should ignore the source alpha.

3) Just a thought. It might be nice if sfImage::CreateMaskFromColor() took an optional parameter for target alpha (defaulted to 0) to allow simple translucency.

[Laurent]

1) You're right, it's more ABGR than RGBA. I guess RGBA is more intuitive for people, so I'll rather change some internal code.

2) Doing this would make the function less flexible. I just can't ignore the alpha value of the color key as some people may need to use it, just like you need to set a target alpha value other than 0.

3) Done

[s_p_oneil]

Thanks for #1 and #3.

Regarding #2, you may be right, but at the moment I don't see it. If you're loading an image without an alpha channel, the point is moot. If you're loading an image with an alpha channel, you shouldn't need to call this function at all. If you're creating an image in memory, you should set the alpha channel when you set each pixel's color (which is more efficient than going back to change it later).

If you added another defaulted parameter, like bIgnoreSourceAlpha=true (which I would add after the targetAlpha parameter), I'd be willing to bet that no one would ever want to change it, and the newbie programmers would be a lot happier because "it just works".

I'm not trying to argue. I'm just explaining why I brought it up. It really is a minor issue, and I think everyone will be ok with it either way.

P.S. - As a side note, you could make a small performance enhancement to sfColor by changing it to a union of a UINT and a struct { a,b,g,r }. When someone calls ToRGBA just return the UINT, and when someone calls the UINT constructor, you just set it. (It may seem minor, but if someone is generating procedural textures, it will be inside a very big nested loop.)

P.P.S. - On a similar note, it would be great if sfImage::GetPixelsPtr() took optional x and y parameters (defaulted to 0) and returned a non-const pointer. That way if I want to update a small rectangle within an image, it would be easy to get the starting position of each row and iterate quickly across the row.

[Laurent]

I agree with you, giving a non-opaque color key will be useless in 99.99% cases. But as you said this is a minor issue, so I'll wait for more feedback to see if it's really needed

P.S. - As a side note, you could make a small performance enhancement to sfColor by changing it to a union of a UINT and a struct { a,b,g,r }. When someone calls ToRGBA just return the UINT, and when someone calls the UINT constructor, you just set it. (It may seem minor, but if someone is generating procedural textures, it will be inside a very big nested loop.)

Using unions for this leads to undefined behaviors (according to the C++ standard). More practically, the result won't be the same whether you run it on a big-endian processor or a little-endian one.
The only way to get a portable behavior is to combine / extract the components using bitwise operators.

P.P.S. - On a similar note, it would be great if sfImage::GetPixelsPtr() took optional x and y parameters (defaulted to 0) and returned a non-const pointer. That way if I want to update a small rectangle within an image, it would be easy to get the starting position of each row and iterate quickly across the row.

Pixels are not stored in a two-dimensional array, so I can't return a pointer to a sub-rectangle. To give access to a sub-part of the pixels array, I'd need to return a proxy class which would have to add offsets to match the new X and Y values.

[s_p_oneil]

Using unions for this leads to undefined behaviors (according to the C++ standard). More practically, the result won't be the same whether you run it on a big-endian processor or a little-endian one.
The only way to get a portable behavior is to combine / extract the components using bitwise operators.

Woops, I forgot about the endian issue. You are right, of course. ;-)

Pixels are not stored in a two-dimensional array, so I can't return a pointer to a sub-rectangle. To give access to a sub-part of the pixels array, I'd need to return a proxy class which would have to add offsets to match the new X and Y values.

I wasn't suggesting that you could return a pointer to a sub-rectangle. I was thinking of something like this:

Code: [Select]


int nStartY = 50, nStartX = 50, nWidth = 50, nHeight = 50;
for(int y = 0; y < nHeight; y++) {
  unsigned int *pRow = image.GetPixelsPtr(nStartX, nStartY+y);
  for(int x = 0; x < nWidth; x++) {
    // Generate and set color based on x and y
    pRow[x] = color;
  }
}


See what I mean? I think this is about as efficient as you can get while keeping the code clean. It would be even more efficient to call GetPixelsPtr() once and use pRow += image.GetWidth() after the inner loop, but that would break if the rows have any padding at all.

Actually, was GetRGBA() backwards so these UINT values could be converted to/from sfColor easily? If so, I imagine THAT would break based on whether the system was little-endian or big-endian (though I'm sure there are ways to address that). As a side note, you could change GetPixelsPtr() to a BYTE pointer and have sfColor take a BYTE pointer when it needs to reverse it based on big-endian vs. little-endian.

At this point I'm speaking before I think (I'm in a hurry because I'm supposed to be working), so feel free to ignore all of this. ;-)