1. CEGUI team
  2. CEGUI
  3. CEGUI
  4. Pull requests

Pull requests

#164 Merged at 2d90fe2
Repository
yaronct
Branch
v0-8
Repository
cegui
Branch
v0-8

Add support for the Epoxy OpenGL loading library and for OpenGL ES 2.0 renderer. Add support for GLFW 3 to the samples framework.

Author
  1. Yaron Cohen-Tal
Reviewers
Description

Changes in Detail

Add a class "OpenGL_API" to act as a wrapper around an OpenGL loading library. It gives information about the type of context (desktop OpenGL or OpenGL ES), its version, and the features supported by it.

EPoxy

Add support for the Epoxy OpenGL loading library (aside from GLEW, which is currently used). Epoxy provides several advantages over GLEW:

GLEW Drawbacks:

  • Doesn't know about aliases of functions (There are 5 providers of glPointParameterfv, for example, and you don't want to have to choose which one to call when they're all the same).
  • Doesn't support GLES.
  • Doesn't support EGL.
  • Has a hard-to-maintain parser of extension specification text instead of using the old .spec file or the new .xml.
  • Has significant startup time overhead when glewInit() autodetects the world.

Epoxy Features:

  • Automatically initializes as new GL functions are used.
  • Desktop OpengGL 4.4 core and compatibility context support.
  • OpenGL ES 1/2/3 context support.
  • Knows about function aliases so (e.g.) glBufferData() can be used with GL_ARB_vertex_buffer_object implementations, along with OpenGL 1.5+ implementations.
  • EGL, GLX, and WGL support.
  • Can be mixed with non-epoxy OpenGL usage.
  • Requires no initialization.
  • Has no run-time dependencies (loads shared libraries as needed and available at run-time).

  • Epoxy is the main Opengl loading library used by GTK 3 (and GNOME 3).

  • The user chooses whether to build CEGUI to use GLEW (the default) or Epoxy as an OpenGL loading library.

  • To use OpenGL (desktop or ES), one has to just include "CEGUI/RendererModules/OpenGL/GL.h", which will include all that is necessary.

OpenGL ES 2.0

Add support for OpenGL ES 2.0. Naturally, this adds support for OpenGL ES 3.0/3.1, which are compatible with 2.0. I don't think its worth adding support for OpenGL ES 1.0/1.1, as they're old, very different and seem to be hardly used anymore.

  • There's no new class for the new OpenGL ES 2 renderer. It uses the same class used for the OpenGL 3.2 Renderer ("OpenGL3Renderer") as its code is quite similar. The user doesn't have to do anything special to choose between OpenGL 3.2 and OpenGL ES 2.0 rendering: the renderer's constructor chooses automatically according to the type of current OpenGL(ES) context.

  • OpenGL ES rendering is only supported using Epoxy. GLEW doesn't support OpenGL ES.

  • Adjust the code to deal with implementations which don't support VAO-s (Vertex Array Objects). This includes OpenGL ES 2.0.

  • Add OpenGL vertex/fragment shaders for OpenGL ES 2.0 and 3.0/3.1.

  • Implement "OpenGLTexture::grabTexture()" for OpenGL ES. OpenGL ES doesn't support "glGetTexImage"/"glGetCompressedTexImage", so we need to emulate it with "glReadPixels".

  • Add CMake configuration variables "CEGUI_OPENGL_VER_MAJOR_FORCE" and "CEGUI_OPENGL_VER_MINOR_FORCE" which let us check how the code deals with a specific OpenGL(ES) version, e.g. OpenGL ES 2.0 when the context is really OpenGL ES 3.0.

  • Hopefully, this project would be the base for adding support for Android and iOS.

Samples Framework

  • Add support for GLFW 3 to the samples framework. The user can choose to build CEGUI with GLFW 2 or 3. The default is still version 2. GLFW 3 provides support for OpenGL ES and EGL.

  • Fix a bug in the samples framework, where the renderer is destroyed after GLFW (or another library) is terminated.

How to test the project

I've made some patches to GLFW 3 and Epoxy. Some have been pulled, some are still in the process. It's recommended, then, to take them from my repositories: GLFW 3 from here (branch "3_stable"), and Epoxy from here (branch "v1_stable"). To use GLFW 3, configure CEGUI with "-DCEGUI_GLFW_VER=3". To use Epoxy, configure CEGUI with "-DCEGUI_BUILD_RENDERER_OPENGL=OFF -DCEGUI_USE_EPOXY=ON -DCEGUI_USE_GLEW=OFF". To use OpenGL ES 2.0 through EGL, I'd recommend first installing PowerVR's OpenGL ES emulator. Note that when using EGL, if u want to build the samples, you'll first need to build GLFW 3 with the configuration "-DGLFW_USE_EGL=ON -DGLFW_CLIENT_LIBRARY=glesv2".

Comments (50)

  1. Lukas Meindl

    Amazing PR, thanks Yaron!

    This will take us a whole to review, but after looking through the changesets I must say this looks top-notch, follows our coding conventions and doesn't seem to conflict with existing Renderer functionality.

    One small mistake I found while reading: You used "destoryRenderer" instead of "destroyRenderer" a couple of times. This must be fixed.

    Edit: I just saw some functions were extracted from the GL Renderers ( e.g. OpenGL3Renderer::initialiseGLExtensions() ). This unfortunately breaks ABI, but the Renderers must be ABI compatible on v0-8 and API compatible on v0. The following should solve this problem: Retain the methods and if possible their functionality (by calling the new function) and mark them as \deprecated via doxygen. If they are marked deprecated I will remove them in default branch later. Please do the same to the destructors: I know you replaced the functionality but we must retain ABI compatibility: keep them but make them just empty (e.g. define them {} in the declaration) and mark them deprecated too with a comment above.

    The changes to the sample browser and the glfw base are acceptable, the sample browser doesnt have to be ABI compatible, nothing has to be changed there as far as I am concerned.

      1. Yaron Cohen-Tal author

        My pleasure.

        About the "destory" typo - you're right, sorry I should have tried to check the Direct3D renderers.

        About "OpenGL3Renderer::initialiseGLExtensions()" and the destructors breaking ABI - again my mistake. It's just the first time I've worked on a library, so I've never had to think about compatibility issues.

        I'll fix these issues.

        1. Lukas Meindl

          No big deal, in fact we have had some "destory" typos in the codebase too, I think I removed them in default branch already.

          And yeah keeping ABI compatibility is hard and annoying, but we have to do it for our users :)

              1. Yaron Cohen-Tal author

                Hmm... I've not noticed that the "initialiseGLExtensions" and "d_s3tcSupported" members of the "OpenGL3Renderer" class are private. Is there any reason to retain ABI compatibility for private members?

                  1. Lukas Meindl

                    I must redact my statement and go back to my initial correction regarding the d_s3tcSupported member: It cannot be removed due to ABI compatibility, this would change the size of the class (it's instances) since this is not static members. No members (that are non-static) can be removed, no matter which visibility. I got confused there a bit but now I just realised it makes no sense they would be removable. So only methods are removable if they are private, but members are never. Correct way to do it: Keep it, never use it, mark it \deprecated in doxygen

                    Since this is my fault I suggest I will merge this anyways and fix it myself, unless you feel like doing so before I merge

                    1. Yaron Cohen-Tal author

                      I see in the merge that u didn't fix it. Do u want me to fix it in a new PR?

                      This whole ABI compatibility is trickier than I thought. I understand that when an object is allocated the size of the class is used, and therefore u can't make any change to the class fields. I wonder, though, why compilers aren't implemented in way in which when allocating an object, the size of the class is taken from the dynamic library at run time, thus making it possible to change the class's size without breaking ABI compatibility.

                      1. Lukas Meindl

                        The merge is a pure merge and nothing else, otherwise this gets really confusing. Whatever I fix I fix in additional commits thereafter. I am not done making changes I will inform you here when I am done.

                          1. Lukas Meindl

                            Done: see https://bitbucket.org/cegui/cegui/commits/branch/v0-8

                            Regarding the "is..." convention, i actually found out that other libraries also use isVaoSupported, but i found none that had areVaosSupported, and the general Java convention relies on "is..." for all bool methods and variables. In C++ there is of course no clear convention but I think it is reasonable to have this in CEGUI. For example it helps when you are coding and rely on suggested methods and you start typing "object.get" and you get all the getters and then type "object.is" to get all bool getters. Having to additionally check for "are" can be bothersome in this context. And in general I feel liek this is "nicer".

                            Edit: Just noticed I missed some changes, pushed them now.

                            1. Lukas Meindl

                              I am going to write a news about this and make a tweet. Do you want to create a wiki page about this? I think it might be helpful. It could be based on the info you provide on top of this PR

                              1. Lukas Meindl

                                Ah that explains why we hadnt used it before, it didnt exist back then! Would be great if this was all adapted for the new function properly, i guess this would also allow us to remove some macros

                                Regarding drone.io: I informed timotei and hope he will fix it

                                    1. Lukas Meindl

                                      Martin just confirmed that 2.8.11 is fine, we should be able to rely on that on all supported OS's

                                      If you could update the other install-target related functionality then this would be very welcome. If some of those cmake macros become redundant by that, then this is welcome too.

  2. Christopher Beck

    Hi just want to comment,

    Thank you for making this OpenGLES2 port, the version that you have created looks much better and more feature complete than the one which I cobbled together in the emscripten port. I will likely try to switch to use your renderer code instead of mine, in that project and in my other projects using cegui.

      1. Yaron Cohen-Tal author

        Oh, sorry, I thought it was Lukas...

        Well, Lukas, Please tell me when you've finished reviewing so that I will make one additional commit with all the fixes.

        1. Lukas Meindl

          I was confused for a moment :)

          I am still reviewing, will probably take a while and I will have to test it later on too, probably it is best I wait until your final commits before i test

    1. Lukas Meindl

      A moment ago I was actually wondering if Yaron created this OpenGL ES 2/3 additions after he saw you using OpenGL ES 2 with CEGUI, because your emscripten port was posted just recently

        1. Lukas Meindl

          Nevertheless, something good can come from that there is now two approaches that can be compared. For example, maybe Christopher finds something that could be added or improved

  3. Lukas Meindl

    For the future: I assume you made all of the initial changes upfront and then added them all at once in one commit and that is why there was only one huge commit at first, but normally the right way to go on about this is to have every in itself complete and working "change" as one commit. So at least some of the changes could have gone into separate commits, definitely for example the change to Singleton. The more commits you have the easier it is to go back and check in case something got broken where it happened and also the easier it is for us to see, based on commit messages, what you changed overall and in which order. Right now for example the singleton change is practically not perceivable unless you go through all changes in this one commit. The nicer way would be to have a separate commit for it with a very short commit description saying what was done. This also helps us later when making change logs.

    Of course this is just a "cosmetic" feature and doesnt change the fact that the quality of your PR overall is fantastic.

  4. Lukas Meindl

    I think it is a bit confusing that "OpenGL(ES)" is supposed to mean "OpenGL or OpenGL ES" - it sounds more like "OpenGL (the ES version!)". Maybe you can grep and replace "OpenGL(ES)" with "OpenGL or OpenGL ES", I think this would clarify it. What do you think?

  5. Lukas Meindl

    Currently I am OK with merging this. I will leave it for one day so others can comment on the PR. If there will be no further comments I will merge it. If you still wanna change something after the merge you can simply make a new PR or multiple new PRs. Thank you!

    EDIT: I have rebuilt the solution via cmake and tested OpenGL, OpenGL3, Direct3D9, Direct3D11 - all works. I won't test OpenGL ES since I trust you that it works.

    1. Yaron Cohen-Tal author

      And thank you for the most thorough code review I've had in my life... I really appreciate it. I believe that in future PR-s you'll have less comments, as I now understand better what u expect to c.

      Plz tell me if and when u want me to merge the changes to the "v0" and "default" branches.

      1. Lukas Meindl

        Merging your changes into v0 should pose barely any issues if at all, merging them into default may cause some problems so it would be great if you could do that since you will be able to test it better than I can (I have no experience with OpenGL ES and this whole setup for it). However, lets first wait till this one is merged, which should happen some point tomorrow.

  6. Lukas Meindl

    I am merging it, I will apply the last suggested changes myself. Please update your repository once I have applied my changes. Feel free to make more PRs. If possible make a branch for each set of changes forming a PR (when you do the first commit open a new branch) which should also allow you to open multiple PRs from one fork.

    1. Lukas Meindl

      Edit: for each set of changes not for each commit, would be crazy do to a branch per commit ;)

      Edit2: Some of the newer comments I added were bogus, I was a bit tired yesterday when I wrote them and marked too many lines of code - I removed those comments. The only ABI break as far as I see is the one related to s3tc bool so I will fix that.