Simon Hug
I'm proposing some changes to the IME test program test/testime.c. The patch includes support for the GNU Unifont hex file, making the SDL_ttf dependency optional. There were also one or two bugs that prevented the text and underline from showing up poperly.
Simon Hug
There's a call to the POSIX function random in test/testqsort.c. Naturally, Windows doesn't do that. The attached patch changes the call to the SDLtest framework random functions and adds some seed control.
Looking at SDLTest_RandomInitTime, I just want to say that 'srand((unsigned int)time(NULL)); a=rand(); srand(clock()); b=rand();' is an absolutely terrible way to initialize a seed on Windows because of its terrible LCG.
Simon Hug
The SDLmain file src/main/windows/SDL_windows_main.c defines both entry points for console applications, main and wmain. This seems to confuse MSVC. It outputs a LNK4067 warning and then chooses main, which is a shame because only wmain has the unicode handling. Using SDLmain.lib provided on libsdl.org, the linker also goes for main.
I'm proposing to not define the main entry point at all. wmain should be supported well enough with MSVC.
Simon Hug
The two tests test/testaudioinfo.c and test/testaudiohotplug.c are missing error checking when they call SDL_GetAudioDeviceName. This function can return NULL which the tests pass straight to SDL_Log.
Eric Wasylishen
The bug here is that a dead keys pressed before calling SDL_StartTextInput() carries over into future text input, so the next key pressed will have the deadkey applied to it.
This in undesirable, imho, and doesn't occur on OS X (haven't check Linux or elsewhere). It's causing a problem for Quakespasm on German keyboard layouts, where we use the ^ deadkey to toggle the console (which enables/disables text input), and ^ characters are showing up in the TEXTINPUT events.
Simon Hug
The function console_wmain in src/main/windows/SDL_windows_main.c does not null terminate the argument list it is creating. As specified by the C standard, "argv[argc] shall be a null pointer."
The SDLTest framework makes use of that null pointer and some test programs can cause an access violation because it's missing.
Simon Hug
The description of the SDL_RenderClear function in the SDL_render.h header says the following:
"This function clears the entire rendering target, ignoring the viewport."
The word "entire" implies that the clipping rectangle set with SDL_RenderSetClipRect also gets ignored. This is left somewhat ambiguous if only the viewport is mentioned. Minor thing, but let's see what the implementations actually do.
The software renderer ignores the clipping rectangle when clearing. It even has a comment on this: /* By definition the clear ignores the clip rect */
Most other render drivers (opengl, opengles, opengles2, direct3d, and psp [I assume. Can't test it.]) use the scissor test for the ClipRect and don't disable it when clearing. Clearing will only happen within the clipping rectangle for these drivers.
An exception is direct3d11 which uses a clear function that ignores the scissor test.
Simon Hug
When updating the viewport in GLES_UpdateViewport, the OpenGL ES renderer doesn't flip the projection matrix for target textures. The lines, rectangles and textures (if drawn with glDrawArrays) are upside down when drawing to target textures.
Simon Hug
The OpenGL ES 2 renderer does not check the target texture format when using SDL_RenderReadPixels and just always uses ABGR8888. This can result in swapped or wrong colors.
The attached patch adds a check and selects the target texture format, if a texture is set as the target.
Simon Hug
All OpenGL renderers always flip the rows of the pixels that come from glReadPixels. This is unnecessary for target textures since these are already top down.
Also, the rect->y value can be used directly for target textures for the same reason. I don't see any code that would handle the logical render size for target textures. Or am I missing something?
The attached patch makes the renderers only the flip rows if the data comes from the default framebuffer.
Simon Hug
The current SDL_SaveBMP_RW function that saves surfaces to a BMP uses an old bitmap header which doesn't officially support alpha channels. Applications just ignore the byte where the alpha is stored. This can easily be extended by using a newer header version and setting the alpha mask.
The attached patch has these changes:
- Extending the description of the function in the SDL_surface.h header with the supported formats.
- Refining when surfaces get stored to a 32-bit BMP. (Must have bit depth of 8 or higher and must have an alpha mask or colorkey.)
- Fixing a small bug that saves 24-bit BGR surfaces with a colorkey in a 24-bit BMP.
- Adding code that switches to the bitmap header version 4 if the surface has an alpha mask or colorkey. (I chose version 4 because Microsoft didn't lose its documentation behind a file cabinet like they did with version 3.)
- Adding a hint that can disable the use of the version 4 header. This is for people that need the legacy header or like the old behavior better. (I'm not sure about the hint name, though. May need changing if there are any rules to that.)
Simon Hug
The GL_SetBlendMode and GLES_SetBlendMode functions of the opengl and opengles renderers call the glTexEnvf to set the texture env mode to either GL_MODULATE (the default) or GL_REPLACE for the NONE blend mode. Using GL_REPLACE disables color and alpha modulation for textures.
These glTexEnv calls were put in the SetBlendMode function back in 2006 [1], but there the NONE code still used the GL_DECAL mode. The GL_REPLACE mode came in 2008 [2]. I'm a bit confused why that wasn't always GL_MODULATE and a bit surprised nobody reported that yet (unless I missed it). I guess only a few use the gles renderer and the newish shaders mask the issue.
Simon Hug
The GL_CreateTexture function doesn't have any checks for the case where the driver doesn't support the framebuffer object extension. It will call into GL_GetFBO which will call the non-existent glGenFramebuffersEXT.
Also, for some reason GL_CreateContext always sets the SDL_RENDERER_TARGETTEXTURE info flag, even if it is not supported. Changeset cc226dce7536 [1] makes this change, but doesn't explain why. It seems to me like the code would already have taken care of this [2].
The attached patch adds some checks and stops SDL from reporting render target support if there is none. The application can then properly inform the user instead of just crashing.
Simon Hug
When the SDL_Blit_Slow function compares the pixel to the color key it does so without removing the alpha component from the pixel value and the key. This is different from the optimized 32-bit blitters which create a rgb mask and apply it to both to filter the alpha out. SDL_Blit_Slow will only skip the pixels with the exact alpha value of the key instead of all pixels with the same color.
The attached test case blits a surface with a color key and prints the pixel values to the console. The third row is expected to be skipped.
Simon Hug
It seems not everyone implemented glDrawTexfOES the same. Intel and Mesa ignore the viewport entirely, whereas the Raspberry Pi implementation offsets the coordinates and does viewport clipping.
The glDrawTexfOES extension text [1] for the function says "Xs and Ys are given directly in window (viewport) coordinates." I guess this wasn't clear enough.
Alex Szpakowski
Honestly I'd probably remove that codepath from SDL_Render entirely. It's an OpenGL ES 1-specific extension that isn't likely to give huge performance gains and adds additional maintenance overhead to SDL_Render while also having bugs in some drivers (as seen here).
SFC30 controller: http://www.8bitdo.com/sfc30/
The SFC30 controller can present itself in a variety of modes and it offers up
different names in each. This patch captures data for three modes (one USB and
two Bluetooth) on three platforms (Mac OS X, Windows, Linux).
However, USB mode on Linux and Windows is missing as the button events did not
make it through to SDL's controllermap tool on Fedora 24/Linux 4.5.5 nor Steam
Big Picture mode on Windows. The two Bluetooth modes were indistinguishable on
Windows. Two modes on OS X were indistinguishable.
There exists a similar controller called the SNES30 (And some others) that are
very likely identical except for the name, but I have not verified this yet so
haven't synthesized lines for those controllers until I can.
Kai Sterker
There are already patches available from mingw64 that fix the issue
https://github.com/Alexpux/MINGW-packages/tree/master/mingw-w64-SDL2
With those applied, I could compile SDL2 without problems. But of course, it would be preferable if SDL built cleanly from source.
Vitaly Novichkov
Line 124
====================================================================
const DWORD flags = thread->stacksize ? STACK_SIZE_PARAM_IS_A_RESERVATION : 0;
====================================================================
Error of compiler:
====================================================================
CC build/SDL_systhread.lo
src/thread/windows/SDL_systhread.c: In function 'SDL_SYS_CreateThread':
src/thread/windows/SDL_systhread.c:124:45: error: 'STACK_SIZE_PARAM_IS_A_RESERVA
TION' undeclared (first use in this function)
const DWORD flags = thread->stacksize ? STACK_SIZE_PARAM_IS_A_RESERVATION :
0;
^
src/thread/windows/SDL_systhread.c:124:45: note: each undeclared identifier is r
eported only once for each function it appears in
make: *** [build/SDL_systhread.lo] Error 1
====================================================================
Fixing when I adding into begin of the file:
====================================================================
#ifndef STACK_SIZE_PARAM_IS_A_RESERVATION
#define STACK_SIZE_PARAM_IS_A_RESERVATION 0x00010000
#endif
====================================================================
ny00
This report is going to cover three issues, with a suggestion for fixes. For reference, tests were done using an installation of android-x86-5.1-rc1.iso within a VirtualBox session. I've actually used an adapter that accepts up to two Playstation 1/2 controllers.
A ZIP file should be attached, with the following contents:
- The patch file itself.
- Outputs of joysticks lists from testjoystick with different orders (before fixing bug).
- Game controllers database entries (for reference).
--- Different outputs for different platforms may stem from different tools being used; The Android mapping was manually constructed using a previously available mapping as a base.
--- Note that it turns out the Linux mapping is already out there in some form: https://github.com/gabomdq/SDL_GameControllerDB/blob/master/gamecontrollerdb.txt
And so, let's begin listing the issues:
1. While changeset https://hg.libsdl.org/SDL/rev/9b540bea3cf1 has good intentions, it appears to make various input devices being mistakenly detected as SDL joysticks. I got lists of the devices from joysticktest, given in the ZIP file. "badordering.txt" is what I get if the device has been plugged since a reboot of the virtual machine, while I've gotten "goodordering.txt" after hot-plugging the USB adapter. As expected, only in the latter case I could use the controller in the test program (assuming it isn't modified).
To take care of this, I updated pollInputDevices and added the function SDLActivity.isDeviceSDLJoystick, in order to have a better filter. Note that it also checks that the device id is non-negative, since VIRTUAL_KEYBOARD appears to include a SOURCE_DPAD, and I should probably keep accepting it as an SDL_joystick (good if you want, say, to support multiple independent d-pad devices).
I hope the device id filter does not break support for the virtual remote (mentioned in the changeset above).
2. So there's a weird glitch here, where the game controller is reported to have SOURCE_KEYBOARD and SOURCE_JOYSTICK, while each controller button press/release emits a KeyEvent with SOURCE_KEYBOARD only. So obviously any test going over the event's own sources is expected to fail.
It is possible to try and filter by the key code, but then there are the dpad key codes, which can also be emitted by actual PC keyboard's key presses/releases (the arrow keys).
So instead, I just call the newly added isDeviceSDLJoystick function again and check if the input device (not the event) has any source considered to be a joystick/gamepad for us.
3. Finally, if SDL2 properly detects an SDL_Joystick being connected, but it is not opened, then whenever a KeyEvent is received after a button press/release from the same controller, SDLActivity.onNativePadDown/onNativePadUp returns a negative value. In such a case, the onKey handler continues to check for SOURCE_KEYBOARD (and possibly also SOURCE_MOUSE), which is clearly not desired.
And so, in the given patch, the return values of onNativePadDown and onNativePadUp are ignored and "true" is returned either way.
(Note: Maybe the native functions should be modified to have the return value of "void".)
Finally, as another side-note, I've noticed that the various bitwise tests for sources are wrong. For instance, to check if an InputDevice 'device' has source SOURCE_JOYSTICK, the value (device.getSources() & SOURCE_JOYSTICK) should be compared to SOURCE_JOYSTICK, not 0.
However, I think there's enough that this patch covers. At the least, isDeviceSDLJoystick partially solves this.
neoaggelos
It is common for Mac OS X to use the SDL2.framework instead of the classic UNIX dynamic lib.
Therefore, it makes sense for AM_PATH_SDL2() to be able to locate it. Attached is a patched sdl2.m4 (updated for that purpose).
Changes:
* look for SDL2.framework ONLY if pkg-config and sdl2-config tests failed (this is to ensure compatibility with the old behaviour)
* adds option ('--disable-sdlframework') to retreat to the old behaviour
* adds variable 'SDL2_FRAMEWORK' for the user to specify the exact path to SDL2.framework (e.g. if it is installed in a non-standard location)
Open to suggestions for further improvements
Sam Lantinga
Bastien Nocera
Jonathan Dowland
Cole Campbell
Philipp Wiesemann
Ryan C. Gordon
Alex Szpakowski