veandco / go-sdl2

SDL2 binding for Go
https://godoc.org/github.com/veandco/go-sdl2
BSD 3-Clause "New" or "Revised" License
2.2k stars 218 forks source link

sdl: add integer scale bindings for Renderer #401

Closed flga closed 5 years ago

flga commented 5 years ago

Add the missing SDL_RenderGetIntegerScale and SDL_RenderSetIntegerScale methods on the Renderer struct.

I'm not aware of an easy way to test using older versions of sdl (since both these functions are >=2.0.5). Got any tips on how to do that easily? I'd like to make sure it works as expected on older versions before merge.

veeableful commented 5 years ago

Thanks @flga for implementing missing bindings! The PR looks really good 😃

If you want to test if it builds with older SDL2, you could do similar thing as our Travis CI where we compile and install SDL2 from source and run go build or go install to make sure it builds successfully.

Or.. I believe you can also enable Travis CI build on your repository but that does require you to push it to Github first.

flga commented 5 years ago

@veeableful thanks for the tip, I'll give it a go tomorrow!

flga commented 5 years ago

@veeableful fixed error handling on getIntegerScale, it returns 0 on error not -1.

Added a more descriptive error on sdl < 2.0.5, I wasn't setting the error before so it would just return the last one. And finally added some tests to make sure that on sdl < 2.0.5 it returns the appropriate error.

flga commented 5 years ago

@veeableful just noticed that error handling isn't ok yet, I need to clear the error before call and check the error after call when it returns SDL_FALSE.

flga commented 5 years ago

@veeableful ok, I think it's ready now :smile:

veeableful commented 5 years ago

@flga Thanks for the touch-ups! I thought it is fine without clearing the error because you are returning nil when it succeeds. Am I missing something?

flga commented 5 years ago

@veeableful when it succeeds there's no problem, it's when it returns 0, without failing, having a previous error set.

Here's a quick writeup of what happens if we do not clear the error:

// create a streaming texture and set it as a render target,
// it will fail with "Texture not created with SDL_TEXTUREACCESS_TARGET"
tex, _ := renderer.CreateTexture(PIXELFORMAT_ABGR1555, TEXTUREACCESS_STREAMING, 200, 200)
renderer.SetRenderTarget(tex)

// now get the scale value, which is SDL_FALSE, since we have not modified it
v, err := renderer.GetIntegerScale()
// since we have not cleared the error, and sdl returns false, we call GetError()
// and it will return the "texture not created with" error from before

That's why we need to explicitly clear the error.

veeableful commented 5 years ago

@flga Ah! So renderer.GetIntegerScale() also returns SDL_FALSE when it's not an error? That's really interesting! I wonder if SDL2 people are doing this too. I will merge this then and thank you for the explanation! :relaxed: