DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.74k stars 231 forks source link

Remove unnecessary `find_package(OpenGL)` #400

Closed Jayman2000 closed 1 month ago

Jayman2000 commented 1 month ago

Pull Request Type

Description

Before this change, CMakeLists.txt would run find_package(OpenGL), but it would never use any of the targets or variables that calling find_package(OpenGL) produces. As a result, the call to find_package(OpenGL) didn’t really do anything.

Related Issues

Checklist

Additional Comments

winterheart commented 1 month ago

Actually OpenGL functions get loaded with dlopen() in dyna_gl.h. On Linux OpenGL functions get loaded via SDL_GL_GetProcAddress. By checking OpenGL we ensuring that system has OpenGL and SDL2 can use functions from it.

Lgt2x commented 1 month ago

Actually OpenGL functions get loaded with dlopen() in dyna_gl.h. On Linux OpenGL functions get loaded via SDL_GL_GetProcAddress. By checking OpenGL we ensuring that system has OpenGL and SDL2 can use functions from it.

However this logic only applies on the machine building the game, not the one running it. Using find_package is only useful at compile time to link our executable against the targets found, which is not the case here. A computer compiling the game in theory should not need OpenGL at all.

Jayman2000 commented 1 month ago

@winterheart

Actually OpenGL functions get loaded with dlopen() in dyna_gl.h.

If we were linking the library at compile time, then I would agree that it would be good to give an error at compile time. However, if we’re using dlopen() to open the library at runtime, then I think think that we should only give an error at runtime. We already give an error at runtime.


@Lgt2x

A computer compiling the game in theory should not need OpenGL at all.

Interestingly enough, there actually is a situation where computer compiling the game would need OpenGL. renderer/dyna_gl.h says:

#if defined(WIN32)
#include <GL/gl.h>
#else
#include "SDL_opengl.h"
#endif

Windows is the only platform that has a direct dependency on OpenGL, but it’s also the only platform where we don’t run find_package(OpenGL). (The line of code that I deleted was in an if(UNIX)). It’s OK to not call find_package(OpenGL) on Windows because we already depend on the Windows API, and the Windows API has included GL/gl.h since Windows 2000.