Page MenuHome

Ghost Context Refactor
AbandonedPublic

Authored by Antony Riakiotakis (psy-fi) on Jul 12 2014, 8:29 AM.

Details

Summary

This patch separates graphics context creation from window code in Ghost so that they can vary separately.
This allows desktop (CGL,GLX,WGL) and embedded (EGL) code to be separated.
(More ambitiously it sets the stage for somebody to write a Mantel, Metal, Direct3D, or whatever backend for the game engine)

The meaning of the new build options are here: http://wiki.blender.org/index.php/User:Jwilkins/VFX/GHOST_Refactor_for_EGL

Recently I added an option WITH_X11 so I could build the glX context on OSX that is not documented yet. I only meant it for development purposes, but having a WITH_X11 variable actually makes some of the build code cleaner in places.

The new WGL and EGL context classes have more features than the CGL or glX contexts, but all the contexts should behave as they did before. This just means that only WGL and EGL are capable of requesting extended functionality such as core GL.

Note: Visual selection is done after creating an X window, which is technically backwards, but it worked on OSX.
Note: I removed some code that was not being used.
Note: I moved the OSX locale query to the boost locale wrapper

Sorry for the delay, this has been ready for somebody to look at it for a while, but I was not assertive enough about getting it looked at.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Did some basic review. More comes later once the patch is applyable on current master. Seems it needs an update.

CMakeLists.txt
166

Seems this could be simplified avoiding extra variables here (which you don't unset btw), and not adding WITH_X11 option on linux.

172

Wouldn't really call option like this. It's not gonna to give you workable binary i bet anyway. More like WITH_X11_DEVEL_TEST ?

188

I've got serious doubts it'll ever work outside of the linux world.

552

It doesn't seem to follow our CMake code style. @Campbell Barton (campbellbarton)?

641

What is this for?

1152

This is weird and seems to be reverting fixes from me.

2253

Don't use XXX for stuff which is different from dirty hacks which violates some design assumptions. This is a TODO mainly.

build_files/cmake/macros.cmake
363

Could depend on particular configuration/feature set enabled. I'd leave this alone.

intern/cycles/app/CMakeLists.txt
26

I guess such a things could be hidden in external_libs.cmake making code here more obvious and easier to maintain etc?

intern/ghost/intern/GHOST_SystemWin32.cpp
237

Not fan of having dead code.

intern/ghost/intern/GHOST_SystemX11.cpp
58

You sure it's needed?

intern/ghost/intern/GHOST_Window.cpp
88

Why do you like adding empty lines all over the place? Tempting to request getting rid of them.

intern/ghost/intern/GHOST_WindowCocoa.mm
36

As per code style we do indent inner preprocessor with two spaces.

intern/ghost/intern/GHOST_WindowWin32.h
46

Dead code. Also, does it still work with mingw?

intern/glew-mx/intern/glew-mx.c
43

It's not really glew, it's some sort of our extension to glew. Think we shouldn't use glew prefix for such functions.

intern/locale/CMakeLists.txt
39

Why?

source/blender/gpu/GPU_extensions.h
39

I don't see bool in this header.

60

What's the point of making it lowercase?

source/blender/gpu/GPU_material.h
38

Bool, where?

source/blender/windowmanager/intern/wm_window.c
397

Why to clean this up? This only adds noise to the logs.

Forgot to mention, SCons needs an update as well.

CMakeLists.txt
172

I did have a working version of Blender running on X11 on OSX. I would have taken it out, but the changes were small enough that I decided to leave it in here for review.

641

This was copied from the previous X11 variable setup code.
If it isn't actually used anywhere I can remove it, but this is what is in master right now.

1152

I didn't mean to include this (I removed some other similar changes). I had to make this change to get Blender to build.

See e01449fa4bb0df252d32a3b98ee1e8f195923d96
and 6da8fa60f58b458bca12e8655cea11c148e4fc2f

I'm not sure why my build fails but it doesn't seem to be a problem for others.

intern/cycles/app/CMakeLists.txt
26

I was thinking much the same thing after making so many modifications to CMakeLists.txt for OpenGL that were all identical.

intern/ghost/intern/GHOST_SystemWin32.cpp
237

I believe this code is also dead in master and I kept it here because I'm not sure why it was commented instead of removed.

intern/ghost/intern/GHOST_SystemX11.cpp
58

I think it was needed on OSX, but I'll double check.

intern/ghost/intern/GHOST_Window.cpp
88

I can remove them

intern/ghost/intern/GHOST_WindowWin32.h
46

This just seems to be a mistake.

I moved the definition of _WIN32_WINNT without also copying the condition.

Since GHOST_TaskbarWin32 already depends on window.h this could actually just be removed entirely right?

intern/locale/CMakeLists.txt
39

This was implemented as part of Ghost but all it was doing was setting a global variable that could be read from the boost_locale_wrapper.

I moved it because it was keeping Blender from linking on OSX when using GHOST_SystemX11 instead of GHOST_SystemCocoa. However, I think this is probably how the locale reading on OSX should have been done in the first place.

source/blender/gpu/GPU_extensions.h
39

I removed the function that needs it, but missed removing this include as well.

60

Uppercase GPU_ means it is part of the interface while gpu_ is an internal function. GPU_extensions_init has been replaced with GPU_init.

CMakeLists.txt
641

Apparently, XF86keysym is a library that has key codes for media keyboards
WITH_XF86KEYSYM is automatically defined for Ghost if the include file is found.

intern/ghost/intern/GHOST_SystemX11.cpp
58

Apparently, XStringLookup is in Xutil.h

source/blender/gpu/GPU_material.h
38

bool appears at least 10 times in this file

Jason Wilkins (jwilkins) updated this revision to Unknown Object (????).Jul 18 2014, 11:59 PM

Addressed some of Sergey's comments:

  • simplified X11 on non-unix option
  • removed some unintended edits and whitespace modification

Addressed Antony's suggestion that context options be made into a single option.

Also addressed in the previous revision are the removal of some dead code and changing the glew context functions to be prefixed with "mx" instead of "glew"

Tested on Linux, needed to make some edits to build with strict warnings.

Commented on minor issues.

Tried to use this on X11, Opens and closes single window fine but asserts when closing a second window.

GHOST_ContextGLX.cpp:92: virtual GHOST_ContextGLX::~GHOST_ContextGLX(): Assertion `s_sharedCount > 0' failed

Also, think it may work best to finalize this patch in its own branch (so fixes can be made there more easily then a patch - which would mean we have to commandeer each time)

CMakeLists.txt
188

Yep, not sure its really worth to support X11 outside Linux/BSD's

355

Would call WITH_GL_ANGLE

552

not sure, in what way?

2141

if (NOT OPENGLES_DLL) should work here

intern/ghost/intern/GHOST_Context.cpp
41

Should be static

56

Shouldn't this be ifdef ?

Getting error: "WITH_GLEW_ES" is not defined

Otherwise WITH_GLEW_ES should be defined as 0 (but our convention is to use ifdef)

77

Should be static

intern/ghost/intern/GHOST_Window.cpp
55

can you avoid reformatting code when making minor changes?
(That, or propose new code-style convention - but in this case just avoid imho)

intern/ghost/intern/GHOST_WindowWin32.cpp
702

and again, no need to reformat.

intern/ghost/intern/GHOST_WindowWin32.h
76

again, please dont re-format unrelated code, just adds noise when doing code-review

intern/ghost/intern/GHOST_WindowX11.cpp
199

again, dont reformat.

235

again, dont reformat.

1129

Code style is to do GHOST_Context *context - also existing code in this file was like that.

intern/locale/CMakeLists.txt
39

This should be commented then

source/blender/gpu/GPU_extensions.h
60

Convention here is only use lowercase names in this fashion for local static functions.

source/blender/gpu/GPU_init_exit.h
2

Keep license text at top of file, header guard isnt following convention (see existing)

source/blender/gpu/intern/gpu_draw.c
1883

should be static (and lowercase)

A couple of responses, but the rest I'll just fix.

Should I upload another revision or make a new branch or what?

source/blender/gpu/GPU_extensions.h
60

That seems to contradict what Brecht told me.

source/blender/gpu/intern/gpu_draw.c
1883

This function is intended to be public.

One thing I forgot to mention is that there is no sdl context. I don't actually think that will be very complex though.

Had to make a few changes to make it build on ubuntu, see P92

WITH_SYSTEM_GLEW is not very well supported. If we are to use our own version we should always disable this option.

Similarly to Campbell I'm getting trash content with two windows and a crash when attempting to close one of them (assertion failing).

Still more to review, but doing a preliminary flush.

intern/ghost/intern/GHOST_Context.cpp
80

You could add a #define ERR_CODE_TO_STR(code) case code: return #code ;
and reuse that.

146

Maybe use a flag or NULL check to know if context has been initialized once before destroying (in the function itself, too)?

intern/ghost/intern/GHOST_ContextEGL.cpp
51

Similarly here (you can use define)

This revision now requires changes to proceed.Jul 20 2014, 7:30 PM

Added SDL2 Support,

rB633dbd01d7d18df280a6ae58724ac66c95a8c0b1

@jwikins - can you check on how I've used glew from GHOST_ContextSDL?
I based this off GLX, which uses GLXEW, but for SDL it seems using glew-mx is OK.

@cambellbarton The sdl context looks exactly like I imagined it. I've got a bit on insomnia, so I'm glad I didn't start working on it myself.

I think you are right about the mx pointers for sdl. SDL will take care of that itself and only the glwe mx context is needed.

There are some things I think might need fixing here, but it might be easier to just make the changes than explain.

@Antony Riakiotakis (psy-fi) - Looks like your concerns have been addressed, could you check this works now?

As far as I can see this is now very close to being ready for master,

I tried to use OpenGL ESv2 and it gave many linking errors with glColor4f GL_POLYGON ... etc, Assume this is intended/known, until we support OpenGL ES properly.

One minor style issue,

I noticed C function being called like this:

::glXQueryVersion

I'd rather be consistent here, we could add :: prefix all over where C calls are made, but is this really needed? (C API's for windowing typically have verbose ID's, not sure name-space collision is worth worrying about)

source/blender/gpu/GPU_extensions.h
60

Okay, this is internal but shared between C files, moved into own private header (rB9c9efe7a36aecc3e79f614778433f855fc5174c8)

Pushed some minor changes to the SDL context.

The ES profile will not compile properly until the vertex streaming code is in.

I noticed you added a _private.h. I have several files like this but I named them _intern.h, however it isn't really a big deal if I rename them.

As for :: on globals, this is just my habit from doing a lot of template programming in my research and I like how it sets apart the system calls from other functions. You are right is isn't really needed in this case.

regarding _intern.h vs _private.h, I don't have a strong opinion, it just seemed a bit odd to have intern/foo_intern.h - but if you prefer, simple to change.

intern/glew-mx/intern/glew-mx.c
31

Talking to Sergey and its not exactly clear what glew-mx is. (ok,. I get-it) but still, would be good to have an explanation, and this doxy description seems like a good place to have it.

Something like...

glew-mx abstracts the OpenGL's GLEW library because ..., wrapping EGL, GLEW, WGLEW.... etc.
Its maintained by ... (this is your code right?) and fits into replace GLEW ... explain where it sits between opengl / glew / and other libs

Also, what does *mx* even mean?

A paragraph overview should be enough.

Note on ::glXQueryVersion for global C calls, checked and in fact this is already used a fair bit for ghost (although mainly for Win32),

Since some OS system calls aren't named very clearly, maybe fine to have convention of using :: for C functions.

Tried building on MSVC2013, Windows7, And I get this error:

C:\b\blender\intern\ghost\intern\GHOST_ContextWGL.cpp(290): error C2065: 'GWL_HWNDPARENT' : undeclared identifier
C:\b\blender\intern\ghost\intern\GHOST_ContextWGL.cpp(296): error C2065: 'GWL_HINSTANCE' : undeclared identifier

I did not succeed to build branch under ubuntu 14.04 64 bits ( i7 4770k _ Nividia Titan Black) Nvidia Driver 340.24.

Here is compilation failure log :
http://www.pasteall.org/53095

I did not succeed to build branch under ubuntu 14.04 64 bits ( i7 4770k _ Nividia Titan Black) Nvidia Driver 340.24.
Here is compilation failure log :
http://www.pasteall.org/53095

It looks like you have a version of glew that doesn't support the GLEW_MX option. You can either disable WITH_GLEW_MX or disable WITH_SYSTEM_GLEW or upgrade your development libraries.

I can't compile with the system GLEW on Debian Wheezy: http://www.pasteall.org/53121 My guess is that somewhere the file gets included twice? Not sure glewContextIsSupported is not found tho. But we really need to support building with system GLEW.

Will check on the updated code later.

Also is the revision up-to-date?

Getting linking error. after clean build.

rB07c3d981962ef17950f78961d7a504a99014d80b

../../lib/libbf_windowmanager.a(wm_draw.c.o): In function `wm_triple_gen_textures':
/src/blender/source/blender/windowmanager/intern/wm_draw.c:438: undefined reference to `_mx_context'
/src/blender/source/blender/windowmanager/intern/wm_draw.c:438: undefined reference to `_mx_context'
/src/blender/source/blender/windowmanager/intern/wm_draw.c:438: undefined reference to `_mx_context'
../../lib/libbf_editor_space_view3d.a(drawvolume.c.o): In function `draw_smoke_volume':
/src/blender/source/blender/editors/space_view3d/drawvolume.c:359: undefined reference to `_mx_context'
/src/blender/source/blender/editors/space_view3d/drawvolume.c:451: undefined reference to `_mx_context'
../../lib/libbf_editor_space_view3d.a(drawvolume.c.o):/src/blender/source/blender/editors/space_view3d/drawvolume.c:471: more undefined references to `_mx_context' follow
../../lib/libbf_intern_ghost.a(GHOST_ContextGLX.cpp.o): In function `GHOST_ContextGLX::initContextGLXEW()':
/src/blender/intern/ghost/intern/GHOST_ContextGLX.cpp:143: undefined reference to `glew_chk'
../../lib/libbf_intern_ghost.a(GHOST_ContextGLX.cpp.o): In function `GHOST_Context::~GHOST_Context()':
/src/blender/intern/ghost/intern/GHOST_Context.h:61: undefined reference to `mxDestroyContext'
../../lib/libbf_intern_ghost.a(GHOST_ContextGLX.cpp.o): In function `GHOST_Context::activateGLEW() const':
/src/blender/intern/ghost/intern/GHOST_Context.h:132: undefined reference to `mxMakeCurrentContext'
../../lib/libbf_intern_ghost.a(GHOST_Context.cpp.o): In function `GHOST_Context::initContextGLEW()':
/src/blender/intern/ghost/intern/GHOST_Context.cpp:142: undefined reference to `mxDestroyContext'
/src/blender/intern/ghost/intern/GHOST_Context.cpp:144: undefined reference to `mxCreateContext'
/src/blender/intern/ghost/intern/GHOST_Context.cpp:144: undefined reference to `mxMakeCurrentContext'
/src/blender/intern/ghost/intern/GHOST_Context.cpp:146: undefined reference to `mxGetCurrentContext'
collect2: error: ld returned 1 exit status
source/creator/CMakeFiles/blender.dir/build.make:165: recipe for target 'bin/blender' failed
make[2]: * [bin/blender] Error 1
CMakeFiles/Makefile2:4237: recipe for target 'source/creator/CMakeFiles/blender.dir/all' failed
make[1]:
* [source/creator/CMakeFiles/blender.dir/all] Error 2
Makefile:147: recipe for target 'all' failed

Building works correct now on FreeBSD & Linux

Checked whole patch again at last. Looks good apart from a little thing that can be handled later anyway.
An issue is that we should probably use a glew that supports multiple contexts for this to work - btw @Jason Wilkins (jwilkins) which version is that based on?
If that's the case we should really disable system glew, until distributions catch up

Also committed some edits in branch. I think it can be applied after bcon1 so we have plenty of time to iron out any issues.

extern/glew-es/include/GL/eglew.h
632

One little thing here, we should either maintain a list of patches (similar to how we do for other libraries) for upstream application or to help us when updating versions. Having only the modified code here may prove difficult later on.

Isn't it committed now?