Did a review of my own code to make a todo list.

Did a quick skim through the code, attached some notes.

When I first saw this patch I was worried that abstracting the interfaces in such a hardcoded way might not be such a good idea because sometimes different OpenGL extensions have subtle differences that need a different coding model (for instance, ARB_shader_objects is quite different from core OpenGL). Also, one cannot easily check at runtime what function is really called in the code. For instance, what would happen if two extensions interact in different ways and the abstraction code chose one over the other?

Also, if we really are going to use so many levels of extension loading code (GLEW, multi-context glew, unloading functions we do not need) it gets a bit too complicated just to do one thing: Check for OpenGL support based on platform. I start thinking if having a dedicated extension loader here might be better. Essentially different OpenGL are different renderers. Maybe we should design them like that?

If we really want to abstract OpenGL calls in the GPU module, I wonder if such abstractions will really help us in the end, since the GPU code will be more or less concentrated in one place and we can abstract platform differences "easily" anyway.

Generally I'm still not convinced if this patch should be used.

I am now slowly reading through the whole viewport branch, and given that there is already a level of abstraction in shim_init, I would be interested in hearing the detailed rationale for this actually.

OK, there's a reason to prefer defines rather than shims actually. If we use multiple contexts, then the shim will only have the pointer of the first context applied.

In D699#11, @Antonis Ryakiotakis (psy-fi) wrote:

I am now slowly reading through the whole viewport branch, and given that there is already a level of abstraction in shim_init, I would be interested in hearing the detailed rationale for this actually.

The code was copied from shim_init into mx when I realized it needed to be part of the context and needed to be separate from Blender.

The only thing left in snim_init should be some code for abstracting the use of vertex buffers.

Here is a more detailed rationale for this code:

If we are using desktop OpenGL then it is very likely, if not guaranteed, that if a feature is available as part of standard OpenGL (glFoo) and it used to be an extension (glFooARB) then you are safe to look up GL_ARB_foo and use glFooARB and it will work fine.

This is thrown out the window when using OpenGL ES!

Since OpenGL ES might have incorporated a feature that was never an ARB extension then looking up GL_ARB_foo will fail and you will not utilize a feature that is actually available.

There are also standard features, such as 3D textures and memory mapping, that are only available to ES as extensions.

Before, in previous GSoCs, we had a set of functions called gpu_glFoo which implemented these 'feature to extension' mappings. But I realized that just using the un-suffixed name was probably cleaner. That is why I added code to remove and redefine the symbols from glew.h

In D699#10, @Antonis Ryakiotakis (psy-fi) wrote:

Did a quick skim through the code, attached some notes.

When I first saw this patch I was worried that abstracting the interfaces in such a hardcoded way might not be such a good idea because sometimes different OpenGL extensions have subtle differences that need a different coding model (for instance, ARB_shader_objects is quite different from core OpenGL). Also, one cannot easily check at runtime what function is really called in the code. For instance, what would happen if two extensions interact in different ways and the abstraction code chose one over the other?

Also, if we really are going to use so many levels of extension loading code (GLEW, multi-context glew, unloading functions we do not need) it gets a bit too complicated just to do one thing: Check for OpenGL support based on platform. I start thinking if having a dedicated extension loader here might be better. Essentially different OpenGL are different renderers. Maybe we should design them like that?

If we really want to abstract OpenGL calls in the GPU module, I wonder if such abstractions will really help us in the end, since the GPU code will be more or less concentrated in one place and we can abstract platform differences "easily" anyway.

Generally I'm still not convinced if this patch should be used.

The only real difference I could find was that ARB_shader_objects takes a couple of parameter names that are illegal in OpenGL 2.0 (and not used by Blender). I could check again by reading over the specs and the standard, but I think the possibility of subtle differences causing a problem is unlikely. If there is a problem it would be with ARB_shader_objects, but I think saying it is "quite" different might be an exaggeration. The differences seem simple.

I'm not sure what you mean by losing the ability to check for OpenGL support based on platform. This patch centralizes all of that checking into a single place. Other code simply doesn't need to know, for example, if 3D textures are an extension or not.

If we did have our own custom extension loader it would look a lot like what I've written here except we'd have to duplicate a lot of code from glew. This code just reorganizes what glew found into something less verbose to use.

All this really does is check, once, which entry point to use for a particular feature, and then stores that result in a structure for use later.

In D699#12, @Antonis Ryakiotakis (psy-fi) wrote:

OK, there's a reason to prefer defines rather than shims actually. If we use multiple contexts, then the shim will only have the pointer of the first context applied.

My main reason for going with defines is that you do not have to remember to use some weird name like gpu_glFoo or _mx_context->Foo.

Also, glew.h uses defines.

I admit that the list of functions that need to be defined/undefined is long, but it isn't complicated. They are just copied from the specs.

I am checking at this again.

I am feeling that this adds too much complexity with too little benefits at the moment. We have to keep track of various versions of external libraries and undefine certain functions and definitions in our own code. A good question at this point is: Why not write our own wrapper instead of GLEW if we are to do so much work redefining and reloading stuff? It will be much simpler to keep track of, instead of looking through 5 headers at a time trying to figure out what a symbol tracks back to, and we can maintain it better. I think this kind of abstraction, if needed, can certainly wait for later for when we have GLEW support in blender, but I would not go for it now.

In D699#12708, @Antonis Ryakiotakis (psy-fi) wrote:

I am checking at this again.

I am feeling that this adds too much complexity with too little benefits at the moment. We have to keep track of various versions of external libraries and undefine certain functions and definitions in our own code. A good question at this point is: Why not write our own wrapper instead of GLEW if we are to do so much work redefining and reloading stuff? It will be much simpler to keep track of, instead of looking through 5 headers at a time trying to figure out what a symbol tracks back to, and we can maintain it better. I think this kind of abstraction, if needed, can certainly wait for later for when we have GLEW support in blender, but I would not go for it now.

How is it complex?
There are two types of complexity, incidental and essential.
Incidental complexity is complexity introduced by a bad solution. You could also call it accidental complexity.
Essential complexity is the actual complexity of the problem.

We have a about dozen features that are implemented by about three dozen GL versions and extentions. This solution chooses which version or extention to use once so that the code is not littered with compile time and runtime branches that would be even more complex than this.

This solution centralizes the decision so that it is not spread out through the code and makes it less likely a
maintainer will miss something.

This solution is actually not very complex, just verbose.

What external libraries are we keeping track of? If you mean OpenGL then unfortunately I do not think we can outsource understanding GL's history to somebody else. Using extentions and supporting multiple versions is our job.

The undef's are a convenience to remind programmers not to use extensions directly. They are a lot like the GPU_deprecated.h; if they seem like a nuisance they could be removed.

The definitions for the "canonical" functions are straightforward really. How is it difficult to keep track of? If you wanted to add support for another feature it would be simple.

Writing our own wrapper would be redundant. This is only a small number of functions compared to what GLEW loads. This does not "reload" anything, it just chooses which pointers to use for each feature.

Also, this is building on top of GLEW by using it to load the extention pointers and determine what is available. All this code does is decide in a single place which entry points to use.

These decisions have to be made anyway! CentralIzing it makes sense. This complexity is not going away, it is essential. I've just concentrated it in one place and you are only balking because you couldn't see it before.

(Admittedly supporting core and es adds some more essential complexity)

A remake of GLEW would be just as complicated as GLEW. I'd rather build a custom GLEW that only contains the exact stuff we need (which is possible), but making our own would be odd, since we don't have to in order to get the benefits.

Looking through 5 headers is unfortunate. MSVC takes me straight to the correct symbol. My question would be why do you need to? Anybody working on Blender GL directly should know which functions have been canonicallized if they are working that deeply.

I'm not sure what you mean by "GLEW support in Blender".

One thing about this code is that it is based on OpenGL's history. History does not change.

Finally, I do not have a solution to the problem this code solves except lots of conditionals at compile and runtime. But that would be a discusting mess.

When I read Blender's OpenGL code I find a lot of mistakes made because somebody did not look up exactly where an entry point is defined. There is a failure to check for the right version or extention or to even check at all.

There is also a failure to make sure that all possibilities have been exhausted for accessing a feature.

With adding support for OpenGL core and es this task gets even more complex. This code simplifies maintainance for most contributors because they no longer need to worry about being an expert in OpenGL's 25 year history.

This code is also moderately more efficient because it directly calls the required function instead of choosing between alternatives every time.

Aaargh, I meant GLES, not GLEW, sorry.

Centralization is desirable and should be done (everything will indeed be done in the GPU module).

When I say external libraries I am referring to GLEW itself - different platforms or even distributions may use different versions of the library. I don't think it will be such an issue in this case because the abstracted functionality is old, but for GLES the GLEW headers can change as far as I understood?

What is of concern is what information you hide too. Not everyone uses MSVC and you require programmers to also become familiar with how blender uses OpenGL in addition to just know how OpenGL works "out of the box". Having one code path instead of many is good, but with the possible exception of GLES, we can just agree to use one set of extensions without hiding it behind a second layer. Besides I'd say it's good time to define that OpenGL 2.1 at least for desktop should be minimum for blender soon so most if cases should go away anyway. So does adding the code here really solve something crucial?

What I find peculiar, is the following chain:

• GLEW
• Add extra wrapper on top of GLEW for multiple contexts (essential if we want correctness)
• Add extra wrapper on top of the wrapper to disallow functions in the rest of blender (nice but not essential)
• Add extra wrapper to abstract functions that share same functionality (not essential and makes GL interface different than default)
• Add missing definitions for different GLEW versions. (not essential and not desirable)
• Use OpenGL

I am sure this can be simplified. If we really have to add this much code in blender redefining stuff just so we don't drop away GLEW, I seriously suggest we write a loader of our own. There are so many lines redefining things that it's almost certain that those could be used for a custom loader. And in that case, things are even simpler. Undefined functions don't even exist, that is, you get a compile time error unless you define them, and you can define them in any way you want using just one abstraction layer. And we get rid of GLEW as a dependency. For new functions, again, anyone who wants to add new functionality can do so, and the system is much more transparent to people who have working knowledge of standard OpenGL.

In any case, this is just my opinion, if others think that this is a good solution then I will agree to this. If this is truly blocking vertex streaming we should get more eyes here and get a to final conclusion.

Getting rid of GLEW would actually be pretty cool.

I just wanted to avoid it because it seemed like reinventing the wheel, but at the same time writing a Python script to grab the spec from opengl.org and only generate exactly what Blender needs to a "GPU_gl.h" does seem like a similar amount of effort as "patching" GLEW at runtime.

Mainly, it has the conceptual advantage of doing everything in one place instead of spread between extern/glew, intern/glew-mx, and source/blender/gpu.

So, I guess I see your point.