Page MenuHomeWildfire Games

Cleanup of OpenGL loading (add extension loader library)
Changes PlannedPublic

Authored by echotangoecho on Jul 7 2017, 8:11 PM.

Details

Summary

Add loaders for gl, glx, wgl generated with glad. Some issues remaining

  • Do we need to include the license of glad (given that the loader is generated with the tool) - I would say likely yes (because why not?) (also note that it is not in this patch)
  • Will loading wgl functions in this way work (may nullptr be passed as HDC?)
  • Is there a way to get premake to compile a C lib without C++ flags? otherwise compiling the glad lib as C++ seems to work (changing extensions .c to .cpp)

Next step: remove all deprecated OpenGL usage and generate core contexts where available (so newer GL functions are available on OS X).

Test Plan

Test on windows, see whether game still works and doesn't crash.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
glload
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 2547
Build 4313: Vulcan BuildJenkins
Build 4309: arc lint + arc unit

Event Timeline

echotangoecho created this revision.Jul 7 2017, 8:11 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jul 7 2017, 8:11 PM
echotangoecho planned changes to this revision.Jul 7 2017, 8:12 PM
vladislavbelov requested changes to this revision.Jul 7 2017, 9:05 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
source/lib/ogl.cpp
115

I didn't see #ifdef _MSC_VER to use sscanf_s. It doesn't compile on linux.

230

Unnecessary brackets, also we use brackets on a new line. The same applies to the line below.

source/lib/ogl.h
37

Why extern was removed? The same for few lines below.

vladislavbelov added inline comments.Jul 7 2017, 9:07 PM
source/lib/ogl.cpp
149

Why so? By https://www.khronos.org/opengl/wiki/OpenGL_Error it's ok to call it GL_INVALID_FRAMEBUFFER_OPERATION.

Vulcan added a subscriber: Vulcan.Jul 7 2017, 11:32 PM

Build is green

Updating workspaces.
Build (release)...
../../../source/graphics/ShaderProgramFFP.cpp:30:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
../../../source/renderer/InstancingModelRenderer.cpp:379:5: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if CONFIG2_GLES
     ^
../../../source/graphics/ShaderTechnique.cpp:33:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
../../../source/graphics/ShaderTechnique.cpp:63:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
Build (debug)...
../../../source/graphics/ShaderProgramFFP.cpp:30:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
../../../source/renderer/InstancingModelRenderer.cpp:379:5: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if CONFIG2_GLES
     ^
../../../source/graphics/ShaderTechnique.cpp:33:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
../../../source/graphics/ShaderTechnique.cpp:63:6: warning: "CONFIG2_GLES" is not defined [-Wundef]
 #if !CONFIG2_GLES
      ^
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1719/ for more details.

echotangoecho added inline comments.Jul 8 2017, 12:28 PM
source/lib/ogl.cpp
115

(this code already existed previously) -- refer to lib/secure_crt.h

149

because GL_INVALID_FRAMEBUFFER_OPERATION is part of GL 3.0, and initially I only generated all GL 2.1 functions + extensions, note that IIRC now I generate 3.0 as well, as something we used does not exist as part of an extensions, so it could be changed back.

source/lib/ogl.h
37

because extern is unneeded here (note that I see that I forgot a few), see also rP18591

leper added subscribers: fabio, leper.

Do we need to include the license of glad (given that the loader is generated with the tool) - I would say likely yes (because why not?) (also note that it is not in this patch)

Yes. One could argue that it just generates things from listings that might not be copyrightable in some jurisdictions, however I'm not sure if that argument would hold given that there is some wrapper code. Also the license is nice enough and just adding it to the generated files (if it isn't already added) is the proper thing to do. Also update LICENCE.txt while at it.

source/lib/ogl.cpp
31

I prefer full paths.

235

Seems somewhat strange that we end up having more platform specific code due to using a library.

source/lib/ogl.h
56

Any specific reason to remove the sentinel annotation?

64

This is broken.

Assume checking agains 2.1 while we have 3.0.

source/ps/Profiler2GPU.cpp
501

Is this intended?