Colours are broken

[vault-thirteen]

Go version: 1.20. Go-SDL2 version: v0.4.34. SDL2 version: 2.26.5. OS: Windows 10. Architecture: Intel x86-64.

I draw a simple rectangle using a white colour, and get the yellow colour. The reason of the bug is stated in the message: https://github.com/veandco/go-sdl2/issues/561#issuecomment-1500958405

package main

import (
    "fmt"
    "github.com/vault-thirteen/errorz"
    "github.com/veandco/go-sdl2/sdl"
    "log"
)

func main() {
    var err error
    err = x()
    if err != nil {
        log.Fatal(err)
    }
}

func x() (err error) {
    err = sdl.Init(sdl.INIT_EVERYTHING)
    if err != nil {
        return err
    }
    defer sdl.Quit()

    var window *sdl.Window
    window, err = sdl.CreateWindow("SDL Test", sdl.WINDOWPOS_CENTERED, sdl.WINDOWPOS_CENTERED, 800, 600, sdl.WINDOW_SHOWN)
    if err != nil {
        return err
    }
    defer func() {
        derr := window.Destroy()
        if derr != nil {
            err = errorz.Combine(err, derr)
        }
    }()

    var surface *sdl.Surface
    surface, err = window.GetSurface()
    if err != nil {
        return err
    }

    colour := sdl.Color{
        R: 0,
        G: 255,
        B: 255,
        A: 255,
    }

    err = surface.FillRect(nil, colour.Uint32())
    if err != nil {
        return err
    }

    rect := sdl.Rect{0, 0, 200, 200}
    err = surface.FillRect(&rect, 0xFFFF0000)
    if err != nil {
        return err
    }

    err = window.UpdateSurface()
    if err != nil {
        return err
    }

    running := true
    for running {
        for event := sdl.PollEvent(); event != nil; event = sdl.PollEvent() {
            switch event.(type) {
            case *sdl.QuitEvent:
                fmt.Println("QuitEvent")
                running = false
                break
            }
        }
    }

    return nil
}

colour := sdl.Color{
    R: 255,
    G: 255,
    B: 255,
    A: 255,
}

renders as White

colour := sdl.Color{
    R: 255,
    G: 255,
    B: 255,
    A: 0,
}

renders as Yellow

colour := sdl.Color{
    R: 0,
    G: 255,
    B: 255,
    A: 0,
}

renders as Yellow

colour := sdl.Color{
    R: 0,
    G: 255,
    B: 255,
    A: 255,
}

renders as White

It looks like Red channel and Alpha channel are swapped for some reason !

[vault-thirteen]

// Uint32 return uint32 representation of RGBA color.
func (c Color) Uint32() uint32 {
    var v uint32
    v |= uint32(c.R) << 24
    v |= uint32(c.G) << 16
    v |= uint32(c.B) << 8
    v |= uint32(c.A)
    return v
}

The reason is here. This method should not have been created. The reason is stated below.

To my mind, pixel format depends on the O.S. and we can not just hard-code one format to work everywhere, because it does not work everywhere.

[vault-thirteen]

I have opened the source code of SDL, and it looks like they are using an RGBA channel sequence.

/**
 * The bits of this structure can be directly reinterpreted as an integer-packed
 * color which uses the SDL_PIXELFORMAT_RGBA32 format (SDL_PIXELFORMAT_ABGR8888
 * on little-endian systems and SDL_PIXELFORMAT_RGBA8888 on big-endian systems).
 */
typedef struct SDL_Color
{
    Uint8 r;
    Uint8 g;
    Uint8 b;
    Uint8 a;
} SDL_Color;
#define SDL_Colour SDL_Color

So, this is not a Big-Endian vs Little-Endian problem.

[vault-thirteen]

int SDL_FillRect (SDL_Surface * dst, const SDL_Rect * rect, Uint32 color); Uint32 here is a pixel, this a typo in their docs !

Documentation of SDL says that color argument in SDL_FillRect is an Uint32, and this is not the colour type SDL_Color. SDL_Color is a separate type !

https://wiki.libsdl.org/SDL2/SDL_FillRect Docs say:

color should be a pixel of the format used by the surface, and can be generated by SDL_MapRGB() or SDL_MapRGBA().

Uint32 SDL_MapRGBA(const SDL_PixelFormat * format,
                   Uint8 r, Uint8 g, Uint8 b,
                   Uint8 a);

That Uint32 is returned by SDL_MapRGBA.

All this means that SDL_Color does NOT have any method returning a Uint32. Instead, a value having type Uint32 is created by a SDL_MapRGBA function.

typedef struct SDL_Color
{
    Uint8 r;
    Uint8 g;
    Uint8 b;
    Uint8 a;
} SDL_Color;
#define SDL_Colour SDL_Color

SDL_Color (r,g,b,a) + SDL_PixelFormat => Uint32

So, the error is much and much deeper than I thought at the first time !

[vault-thirteen]

https://wiki.libsdl.org/SDL2/SDL_GetRGBA

SDL_GetRGBA

void SDL_GetRGBA(Uint32 pixel,
                 const SDL_PixelFormat * format,
                 Uint8 * r, Uint8 * g, Uint8 * b,
                 Uint8 * a);

Looks like Uint32 is a pixel, not a colour.

[vault-thirteen]

I have found more evidence about that typo. On other pages they call this Uint32 value as pixel.

https://wiki.libsdl.org/SDL2/SDL_PixelFormat#bytesperpixel

/* Extracting color components from a 32-bit color value */
SDL_PixelFormat *fmt;
SDL_Surface *surface;
Uint32 temp, pixel;

[veeableful]

Hi @vault-thirteen, would using sdl.MapRGBA() solve the problem for you?

...
    cc := sdl.MapRGBA(surface.Format, c.R, c.G, c.B, c.A)
    err = surface.FillRect(nil, cc)
    if err != nil {
        return
    }
...

[vault-thirteen]

Yes, it solves the problem, but the Go's method called Uint32 of the colour is misleading.

Uint32 method uses a hard-coded channel layout which is not cross-platform. It may work on some systems and it does not work on other systems. So, it is very misleading.

I think, you should at least add comments to it about its behaviour (it works here and does not work there). The best thing would be to get rid of non-cross-platform methods, or to add a platform name postfix to it, if you are sure that it works on a specific platform. But if you add a method for a platfom X like Uint32X then you should add methods for other platforms, like Uint32Y and Uint32Z, etc.

// Uint32 return uint32 representation of RGBA color.
func (c Color) Uint32() uint32 {
    var v uint32
    v |= uint32(c.R) << 24
    v |= uint32(c.G) << 16
    v |= uint32(c.B) << 8
    v |= uint32(c.A)
    return v
}

If you want to save the method, it would be cool to rename it as Uint32RGBA and create it's Uint32ARGB sister.

The name without any postfix is misleading.

[veeableful]

Hi @vault-thirteen, I understand. I have updated it to have comments on the documentation to warn users not to use it for SDL2's rendering operations.