New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Surface #38

Merged
merged 2 commits into from Apr 28, 2015

Conversation

Projects
None yet
2 participants
@kr41
Contributor

kr41 commented Apr 28, 2015

It fixes Surface.fill() method and Surface color masks.

The following code doesn't work without fixing type of col argument of fill() method (changed from Color to Color4b). And it creates white PNG-file instead of transparent one without fixing masks.

Surface test = Surface(64, 64);
test.fill(Color4b.White.withTransparency(0));
test.saveToFile("test.png");
@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Good catch, thank you.

Dgame added a commit that referenced this pull request Apr 28, 2015

@Dgame Dgame merged commit f9ea39b into Dgame:master Apr 28, 2015

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Seems that this causes a problem with the Surface CTor which sets pixel memory. This CTor will set the Masks to zero, that seems to solve the problem.

Dgame added a commit that referenced this pull request Apr 28, 2015

Surface: remove redundant assert in CTor.
Surface: the pixel memory in the CTor was overriden by the changes of
#38. Now the CTor without pixel memory uses the Masks, the other sets
them to 0.
@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

@kr41 Can you confirm it works with the latest master for you? If so I will be releasing a new version. :)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

It seems, there might be issues. According to SDL documentation of SDL_CreateRGBSurfaceFrom:

...using zero for the Amask results in an Amask of 0.

However, there is no such warning for SDL_CreateRGBSurface

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

It makes no difference if I use Amask or 0 in SDL_CreateRGBSurfaceFrom (I tried it). My thought was that the pixel memory should take responsibility for the color AND for the transparency. I saw and see no contradiction on the docs. But maybe you know more or have a better idea of solving this?

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

Looks reasonable. Your commit f159b88 doesn't break my fixes. So you can release a new version.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Sounds good. Thanks for your help.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

You are welcome! :)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

Just tested creating Surface from memory. Transparency doesn't work. There is a code at source/DGame/test/main.d:

    uint[256] xpixels = [
        255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255,
        0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0,
        0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0,
        0, 0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0, 0,
        0, 0, 0, 0, 255, 0, 0, 0, 0, 0, 0, 255, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 255, 0, 0, 0, 0, 255, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 255, 0, 0, 255, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, 255, 255, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 255, 0, 0, 255, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 255, 0, 0, 0, 0, 255, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 255, 0, 0, 0, 0, 0, 0, 255, 0, 0, 0, 0,
        0, 0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0, 0,
        0, 0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0, 0,
        0, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 0,
        255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255,
    ];
    Surface xs = Surface(xpixels.ptr, 16, 16);
    xs.saveToFile("samples/images/XS.png");

It draws blue cross on black background. However, there is no way to make transparent area on the Surface with default masks. But if you revert your changes:

    @nogc
    static SDL_Surface* create(void* memory, uint width, uint height, ubyte depth = 32) nothrow {
        assert(memory, "Memory is empty.");
        assert(depth == 8 || depth == 16 || depth == 24 || depth == 32, "Invalid depth.");

        return SDL_CreateRGBSurfaceFrom(memory, width, height, depth, (depth / 8) * width, RMask, GMask, BMask, AMask);
    }

...and change xpixels array to:

    uint b = 0xffff0000;
    uint[256] xpixels = [
        b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b,
        0, b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b, 0,
        0, 0, b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b, 0, 0,
        0, 0, 0, b, 0, 0, 0, 0, 0, 0, 0, 0, b, 0, 0, 0,
        0, 0, 0, 0, b, 0, 0, 0, 0, 0, 0, b, 0, 0, 0, 0,
        0, 0, 0, 0, 0, b, 0, 0, 0, 0, b, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, b, 0, 0, b, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, b, b, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, 0, b, b, 0, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, 0, b, 0, 0, b, 0, 0, 0, 0, 0, 0,
        0, 0, 0, 0, 0, b, 0, 0, 0, 0, b, 0, 0, 0, 0, 0,
        0, 0, 0, 0, b, 0, 0, 0, 0, 0, 0, b, 0, 0, 0, 0,
        0, 0, 0, b, 0, 0, 0, 0, 0, 0, 0, 0, b, 0, 0, 0,
        0, 0, b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b, 0, 0,
        0, b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b, 0,
        b, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, b,
    ];

It will draw blue cross on transparent background.

I think, the best way to deal with it is to add ability to pass custom masks into CTor which sets pixels from memory. Something like this:

    @nogc
    static SDL_Surface* create(void* memory, uint width, uint height, ubyte depth = 32, 
                               uint rmask = RMask, uint gmask = GMask, uint bmask = BMask, uint amask = AMask) nothrow {
        assert(memory, "Memory is empty.");
        assert(depth == 8 || depth == 16 || depth == 24 || depth == 32, "Invalid depth.");

        return SDL_CreateRGBSurfaceFrom(memory, width, height, depth, (depth / 8) * width, rmask, gmask, bmask, amask);
    }

// ....

    /**
     * Make an new Surface of the given memory, width, height and depth.
     */
    @nogc
    this(void* memory, uint width, uint height, ubyte depth = 32, 
         uint rmask = RMask, uint gmask = GMask, uint bmask = BMask, uint amask = AMask) nothrow {
        _surface = Surface.create(memory, width, height, depth, rmask, gmask, bmask, amask);

        assert(_surface, "Invalid SDL_Surface.");
        assert(_surface.pixels, "Invalid pixel data.");
    }

I have tested these changes. And if you want, I will make a separate pull request.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Of course it draws on a black background, because the 0's represents black.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

I mean there is no way to make transparent pixel. Zero will represents black, if you don't use alpha channel. So this happens when zero is passed as alpha mask to SDL_CreateRGBSurfaceFrom. If 0xff000000 is passed as alpha mask, black will be 0xff000000, not 0. Zero will represent transparent in this way.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Ah, now I understand. Ok, then I have to change my tutorial. I would rather set transparent as default, instead of passing the masks to the create method through the CTor. I don't like such design. :)

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Now it looks like this:

    immutable uint black = Color4b.Black.asHex();
    immutable uint blue = Color4b.Blue.asHex();
    uint[256] xpixels = [
        blue, black, black, black, black, black, black, black, black, black, black, black, black, black, black, blue,
        black, blue, black, black, black, black, black, black, black, black, black, black, black, black, blue, black,
        black, black, blue, black, black, black, black, black, black, black, black, black, black, blue, black, black,
        black, black, black, blue, black, black, black, black, black, black, black, black, blue, black, black, black,
        black, black, black, black, blue, black, black, black, black, black, black, blue, black, black, black, black,
        black, black, black, black, black, blue, black, black, black, black, blue, black, black, black, black, black,
        black, black, black, black, black, black, blue, black, black, blue, black, black, black, black, black, black,
        black, black, black, black, black, black, black, blue, blue, black, black, black, black, black, black, black,
        black, black, black, black, black, black, black, blue, blue, black, black, black, black, black, black, black,
        black, black, black, black, black, black, blue, black, black, blue, black, black, black, black, black, black,
        black, black, black, black, black, blue, black, black, black, black, blue, black, black, black, black, black,
        black, black, black, black, blue, black, black, black, black, black, black, blue, black, black, black, black,
        black, black, black, blue, black, black, black, black, black, black, black, black, blue, black, black, black,
        black, black, blue, black, black, black, black, black, black, black, black, black, black, blue, black, black,
        black, blue, black, black, black, black, black, black, black, black, black, black, black, black, blue, black,
        blue, black, black, black, black, black, black, black, black, black, black, black, black, black, black, blue,
    ];

I will update my website and the tutorial tomorrow and also retag the release. Maybe you find another bug in the mean time. ;)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 28, 2015

Looks nice :) Don't forget revert these changes from f159b88

-        return SDL_CreateRGBSurfaceFrom(memory, width, height, depth, (depth / 8) * width, RMask, GMask, BMask, AMask);
+        return SDL_CreateRGBSurfaceFrom(memory, width, height, depth, (depth / 8) * width, 0, 0, 0, 0);
@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 28, 2015

Don't worry,I thought of it. ;) 2900fc7

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

@kr41 Seems that this code does not work correct anymore: https://wiki.libsdl.org/SDL_SetWindowIcon
Did you have a solution, besides giving the masks to the CTor/create method?

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

What doesn't work, I don't get it. I have just tested the latest master and it works. I mean setting window icon from blue cross at source/Dgame/test/main.d. I tested on Xubuntu 14.04. May be that is OS issue?

But I have concerns about these two methods of Color4b struct:

    /**
     * CTor
     * 
     * Expect ABGR as format
     */
    @nogc
    this(uint hexValue) pure nothrow {
        this.alpha = (hexValue >> 24) & 0xff;
        this.blue  = (hexValue >> 16) & 0xff;
        this.green = (hexValue >>  8) & 0xff;
        this.red   = hexValue & 0xff;
    }

    /**
     * Returns the RGBA color information as hex value (in ABGR format)
     */
    @nogc
    uint asHex() const pure nothrow {
        return ((this.alpha & 0xff) << 24) + ((this.blue & 0xff) << 16) + ((this.green & 0xff) << 8) + (this.red & 0xff);
    }

It seems, they should be wrapped into version(LittleEndian) as we do it with Surface masks.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

The SDL icon on the link: https://wiki.libsdl.org/SDL_SetWindowIcon
Copy the pixel data and try it for yourself.

Yes they should be wrapped, I must have been tired. :)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

Oh, of course it wouldn't work. Take note on the color depth and color masks:

  surface = SDL_CreateRGBSurfaceFrom(pixels,16,16,16,16*2,0x0f00,0x00f0,0x000f,0xf000);
@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

Yes, I know. But do you have a smart solution? ;) My only solution would be currently to extend your suggestion and wrap the Masks as optional parameter in a struct, as I do it with GLSettings in Window: https://github.com/Dgame/Dgame/blob/master/source/Dgame/Window/Window.d#L129

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

I was going to propose the same one—to wrap masks to a struct and pass it as an optional parameter to CTor :)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

I think, it would be pretty neat and flexible enough.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

It would be great to add some predefined masks as static attributes, as you have done for Color4b.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

It also would be great to add ability to pass mask struct to toHex() method of Color4b. So that the method will return packed value according to the passed mask.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

I don't get your last comment: How would the Masks change the hex conversion?
It looks now like that:

    /**
     * Returns the RGBA color information as hex value
     */
    @nogc
    uint asHex() const pure nothrow {
        version (LittleEndian)
            return ((this.alpha & 0xff) << 24) + ((this.blue & 0xff) << 16) + ((this.green & 0xff) << 8) + (this.red & 0xff);
        else
            return ((this.red & 0xff) << 24) + ((this.green & 0xff) << 16) + ((this.blue & 0xff) << 8) + (this.alpha & 0xff);
    }

How would your change it?

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

One more thought. I think it makes sense to join depth with color masks into single structure ColorFormat.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

How would your change it?

In the same way as SDL_MapRGBA does it. Or use this function directly, I'm not sure is this a good idea?

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

Here is how it is done in SDL:

/* 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);
    }
}
@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

One more thought. I think it makes sense to join depth with color masks into single structure ColorFormat.

Yeah, I thought about it too, but I think it would bring more harm. For example:

ColorFormat(0, 0, 0, 32); // ups I did forget the alpha value.

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

I think, playing with ColorFormat is advanced option. So people who deal with it should know what they do. It is some sort of low level API. So, there definitely must be an option to shot in your own leg :)

In other way, we can say that DGame library supports only 32-depth RGBA colors of Surface and every one should use Color4b.toHex() method to make pixels in memory. By the way, it is 21st century, memory is not an issue anymore :)

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

@kr41 3784d98 Could you also test it if it works as it should?

Interesting fact: the SDL logo only works if you use

Masks.Zero

with a depth of 32 bit, but not if you use

Masks(0x0f00, 0x00f0, 0x000f, 0xf000)

with a depth of 16 like they do here: https://wiki.libsdl.org/SDL_SetWindowIcon

It also would be great to add ability to pass mask struct to toHex() method of Color4b. So that the method will return packed value according to the passed mask.

I'll leave that to you, if you really want that. I see (currently) no big win.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

By the way, it is 21st century, memory is not an issue anymore :)

Then you could also use Java :D I think even in the 21st century, you should see how and why you waste memory and avoid it if it is possible. :) That's why I use struct instead of classes as often as possible. ;)

@kr41

This comment has been minimized.

Contributor

kr41 commented Apr 29, 2015

Interesting fact: the SDL logo only works if you use Masks.Zero with a depth of 32 bit, but not if you use Masks(0x0f00, 0x00f0, 0x000f, 0xf000) with a depth of 16
...
I'll leave that to you, if you really want that. I see (currently) no big win.

I will hack around tonight and send you pull request, if I get something useful.

I think even in the 21st century, you should see how and why you waste memory and avoid it if it is possible. :) That's why I use struct instead of classes as often as possible.

I like your code design. That is why I drop to work on my own 2D game library for D and going to contribute to your one.

@Dgame

This comment has been minimized.

Owner

Dgame commented Apr 29, 2015

I will hack around tonight and send you pull request, if I get something useful.

Alright, I will wait for it. After that, I believe, we can release Dgame 0.6.0 since the changes we're made aren't really patches. :)

I like your code design. That is why I drop to work on my own 2D game library for D and going to contribute to your one.

That is nice to hear, I am grateful for any help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment