fix color to Uint32

[dedalqq]

Hmm. Maybe I don't understand something, but shouldn't it be for RGBA format? 0_o"

[veeableful]

Hmm according to 9c86fb40, it should be the correct format since it was a fix but we also don't seem to use it anywhere?

If you find a case where it's wrong, let me know!

[dedalqq]

I found the wrong behavior when working with textures. Maybe everything worked fine on the surfaces, but I'm not sure. (I immediately refused them)

package main

import (
    "github.com/veandco/go-sdl2/sdl"
)

func castomUint32(c sdl.Color) 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
}

func drawPixels(tx *sdl.Texture, r sdl.Rect, color uint32) {
    data := make([]uint32, 100)
    for i := 0; i < 100; i++ {
        data[i] = color
    }
    tx.UpdateRGBA(&r, data, 10)
}

func main() {
    sdl.Init(sdl.INIT_EVERYTHING)
    win, _ := sdl.CreateWindow("test", sdl.WINDOWPOS_UNDEFINED, sdl.WINDOWPOS_UNDEFINED, 200, 100, sdl.WINDOW_SHOWN)
    rd, _ := sdl.CreateRenderer(win, -1, sdl.RENDERER_ACCELERATED)

    winTx := rd.GetRenderTarget()

    tx, _ := rd.CreateTexture(sdl.PIXELFORMAT_RGBA8888, sdl.TEXTUREACCESS_TARGET, 100, 100)
    rd.SetRenderTarget(tx)

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

    drawPixels(tx, sdl.Rect{W: 10, H: 10, X: 0}, redColor.Uint32())       // expect red, see yellow
    drawPixels(tx, sdl.Rect{W: 10, H: 10, X: 10}, castomUint32(redColor)) // expect and see red

    rd.SetRenderTarget(winTx)
    rd.Copy(tx, &sdl.Rect{W: 20, H: 10}, &sdl.Rect{W: 20, H: 10})
    rd.Present()

    for {
        event := sdl.WaitEvent()
        switch event.(type) {
        case *sdl.QuitEvent:
            return
        }
    }
}

[malashin]

I'd say it makes sense to make this function return RGBA. But there are several issues.

I'm not sure why this function even exists in the first place. Currently it returns ARGB, proposed one is RGBA. The issue is that if we really want a function that returns uint32 representation of a color then the functions should be named accordingly, Color.Unit32RGBA for example.

Then we get another question to answer. Why do we need those if there are SDL functions already made for it? Efficiency-wise they might be useful, but do we really want to make those for each format?

// MapRGB maps an RGB triple to an opaque pixel value for a given pixel format.
// (https://wiki.libsdl.org/SDL_MapRGB)
func MapRGB(format *PixelFormat, r, g, b uint8) uint32 {
    return uint32(C.SDL_MapRGB((*C.SDL_PixelFormat)(unsafe.Pointer(format)),
        C.Uint8(r), C.Uint8(g), C.Uint8(b)))
}

// MapRGBA maps an RGBA quadruple to a pixel value for a given pixel format.
// (https://wiki.libsdl.org/SDL_MapRGBA)
func MapRGBA(format *PixelFormat, r, g, b, a uint8) uint32 {
    return uint32(C.SDL_MapRGBA((*C.SDL_PixelFormat)(unsafe.Pointer(format)),
        C.Uint8(r), C.Uint8(g), C.Uint8(b), C.Uint8(a)))
}

I'd rather get rid of it to be honest.

[dedalqq]

I probably agree with you. But with one exception. I believe that such simple functions as color conversion are better implemented in GO, since the CGO calls are relatively heavy and in this case we get a big drawdown in performance.

I think it's better to write five lines and implement mapRGBA in GO because it's a very simple operation.

imho

[malashin]

Ye, I'd rather update those functions to use Go. This is the C code. Do you want to do it yourself?

/* Find the opaque pixel value corresponding to an RGB triple */
Uint32
SDL_MapRGB(const SDL_PixelFormat * format, Uint8 r, Uint8 g, Uint8 b)
{
    if (format->palette == NULL) {
        return (r >> format->Rloss) << format->Rshift
            | (g >> format->Gloss) << format->Gshift
            | (b >> format->Bloss) << format->Bshift | format->Amask;
    } else {
        return SDL_FindColor(format->palette, r, g, b, SDL_ALPHA_OPAQUE);
    }
}

/* Find the pixel value corresponding to an RGBA quadruple */
Uint32
SDL_MapRGBA(const SDL_PixelFormat * format, Uint8 r, Uint8 g, Uint8 b,
            Uint8 a)
{
    if (format->palette == NULL) {
        return (r >> format->Rloss) << format->Rshift
            | (g >> format->Gloss) << format->Gshift
            | (b >> format->Bloss) << format->Bshift
            | ((a >> format->Aloss) << format->Ashift & format->Amask);
    } else {
        return SDL_FindColor(format->palette, r, g, b, a);
    }
}

[dedalqq]

I would prefer to use this in my project:

func castomUint32(c sdl.Color) 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
}

Since this is quite enough, since I work with only one color format.

[veeableful]

Thanks @dedalqq for the example! It clearly proves that the current code is not working as expected and looking further the current code really does not make sense so I'll be merging this.

I guess this can be used as a fast alternative to MapRGBA since the color struct is already structured in RGBA at field-level. I'll leave it there if people find it to be handy. We could also re-implement MapRGBA in Go like @malashin said as an additional thing.