libsdl-org / sdl12-compat

An SDL-1.2 compatibility layer that uses SDL 2.0 behind the scenes.
Other
193 stars 40 forks source link

scorched3d/X11: Blank screen #261

Closed smcv closed 1 year ago

smcv commented 1 year ago

Prerequisites:

To reproduce:

You'll see a menu: Play / Start Server / Settings / Help / Donate / Quit. Choose Play. You should get a larger window with a 3D island and a menu that starts with Tutorial.

Expected result: it runs

Actual result: Real SDL 1.2 works. With sdl12-compat, the second (larger) window is blank.

Native Wayland is out of scope here (doesn't work, I'll open a separate bug).

icculus commented 1 year ago

Disabling GL scaling fixes this, so I added a quirk, but I haven't looked further to see why GL scaling breaks this...it doesn't appear to use FBOs, but I could be wrong.

icculus commented 1 year ago

Also, this patch needs work, but applying this to Debian's GLEW makes it use SDL_GL_GetProcAddress if the symbol is in the process, and fall back to glXGetProcAddress if not.

And if it's using SDL, it won't try to initialize the GLX portions.

This is enough to make scorched3d work natively on Wayland with sdl12-compat. If this patch were to go forward, we'd want it to make sure glxewInit isn't in the library, to keep the sdl12-compat heuristic that forces X11, for builds of GLEW that don't have this.

--- glew-2.2.0/src/glew.c   2020-09-26 12:41:58.000000000 -0400
+++ new-glew/src/glew.c 2022-10-19 14:06:42.712664769 -0400
@@ -74,11 +74,13 @@
 #  endif
 #  define glGetProcAddressREGAL GLEW_GET_FUN(__glewGetProcAddressREGAL)

-#elif defined(__sgi) || defined (__sun) || defined(__HAIKU__) || defined(GLEW_APPLE_GLX)
+#elif defined(__sgi) || defined (__sun) || defined(__HAIKU__) || defined(GLEW_APPLE_GLX) || defined(__linux__)
 #include <dlfcn.h>
 #include <stdio.h>
 #include <stdlib.h>

+static int is_sdl = 0;
+
 void* dlGetProcAddress (const GLubyte* name)
 {
   static void* h = NULL;
@@ -86,8 +88,13 @@

   if (h == NULL)
   {
-    if ((h = dlopen(NULL, RTLD_LAZY | RTLD_LOCAL)) == NULL) return NULL;
-    gpa = dlsym(h, "glXGetProcAddress");
+    if ((h = dlopen(NULL, RTLD_NOW | RTLD_LOCAL)) == NULL) return NULL;
+
+    gpa = dlsym(h, "SDL_GL_GetProcAddress");
+    if (gpa != NULL)
+      is_sdl = 1;
+    else
+      gpa = dlsym(h, "glXGetProcAddress");
   }

   if (gpa != NULL)
@@ -175,7 +182,7 @@
 #elif defined(__native_client__)
 #  define glewGetProcAddress(name) NULL /* TODO */
 #else /* __linux */
-#  define glewGetProcAddress(name) (*glXGetProcAddressARB)(name)
+#  define glewGetProcAddress(name) dlGetProcAddress(name)
 #endif

 /*
@@ -22773,6 +22780,10 @@
   int major, minor;
   const GLubyte* extStart;
   const GLubyte* extEnd;
+
+  if (is_sdl)
+    return GLEW_OK;
+
   /* initialize core GLX 1.2 */
   if (_glewInit_GLX_VERSION_1_2()) return GLEW_ERROR_GLX_VERSION_11_ONLY;
   /* check for a display */
smcv commented 1 year ago

If this patch were to go forward, we'd want it to make sure glxewInit isn't in the library

I think that would be an ABI break, because it's public ABI in GLEW. Instead, we'd have to make the version of GLEW that supports automatic choice of GLX vs. EGL export more symbols, and then make the quirk work like this (pseudocode):

if (we found glxewInit && we didn't find the new symbol that indicates support for runtime platform selection) {
    force X11;
}

... and adding or changing ABI downstream is generally not something that Debian and other downstream distros should be doing (it often causes us worse problems at some point in the future), so that should happen in GLEW upstream, not in distros.

https://github.com/nigels-com/glew/issues/172 seems to be the issue for supporting this properly in a future GLEW version. https://github.com/nigels-com/glew/issues/306, https://github.com/nigels-com/glew/issues/315 and https://github.com/nigels-com/glew/pull/343 also look relevant.

I'm not so sure that GLEW upstream would be willing to call into SDL (which from their perspective is an arbitrary third party library), but I might be wrong...

icculus commented 1 year ago

Yeah, I don't have the energy to get into it with upstream, but it would solve so many problems if something like this landed.

(for example, with this patch to GLEW, you don't need the quirk we added for this game, either.)

smcv commented 1 year ago

Confirmed fixed with https://github.com/libsdl-org/sdl12-compat/commit/67f8b3a85b782eefb4db90f34d5b0742ef2cb5fc, 1.2.58 + 29 commits