Improve GLENUM names/macro/printing #71
Conversation
…-coded GLENUM values for special constants like 1=GL_TRUE and 0=GL_FALSE
|
One other small detail is that I specified the max OpenGL version of 4.6 in the Readme. |
|
Oh, and the macro also handles exporting now |
|
Nice! |
|
Interesting, but I don't really know anything about Julia's build system or LLVM in general. That may be a bit much for me to take on :P |
Both aren't really needed ;) It's really just about pointing that script to the OpenGL header files and then run it! |
|
Hi, I just noticed that Using Clang.jl seems interesting and I am trying to understand that area more, but in the meantime, would it be OK to merge this in to fix these issues in the short- and medium-term? I also have a subsequent branch adding some of of the ARB extensions, if you're interested. |
| @@ -19,7 +19,7 @@ const GLhalf = Cushort | |||
| const GLenum = Cuint | |||
| const GLboolean = Cuchar | |||
| const GLclampf = Cfloat | |||
| const GLsizei = Cssize_t | |||
| const GLsizei = Cint | |||
There was a problem hiding this comment.
Those have different sizes.. Was this really incorrect before?
There was a problem hiding this comment.
Ah, sorry...Didn't see your comment... Wow! That's interesting :-O
There was a problem hiding this comment.
@SimonDanisch the errors seemed to be that any time i passed in a pointer to GLsizei, for OpenGL to tell me the length of something (e.x. a uniform name string, or an error log string), it would sometimes return a ridiculously large size, on the order of hundreds of billions.
It was extremely infrequent when getting uniform names, but happened every time when trying to read the error log.
| end | ||
| struct GLENUM{Sym, T} | ||
| number::T | ||
| name::Symbol |
There was a problem hiding this comment.
I'm not sure if I like this to be so heavyweight, with a symbol field and the Sym type parameter...
We already have massive compilation latency problems and putting the name into the type parameter means, that all code using GLENUM will try to specialize on Sym.
Not sure how big the problem is considering that there isn't much to specialize on.. But it still seems excessive!
Maybe it would be easier to group the enums, so that in one group they have their own type, and therefore don't overlap? I don't really see any end user package using the overload infrastructure - this is rather something that should be set in stone by ModernGL itself, or how do you plan to use this?
But it does sounds confusing to me, that when you load some package your enums suddenly get renamed...
Also, does the symbol field actually work in all cases with the C interop? I'm not sure if there are cases where we need to pass an array of enums, or what other problems could arise.
Maybe we can use the implementation from https://github.com/JuliaInterop/CEnum.jl instead, and then put a bit of work into finding some groups, that we need to separate to not get name clashes?
There was a problem hiding this comment.
I was imagining that users might want to overload specific values to return whatever name is most relevant to the debugging work they're doing, but you're probably right that it's not a likely scenario in practice.
I'm not sure how much this increases the compilation times, as doesn't the compilation only happen after you first dispatch to it? I thought that in practice, this function is merely something you call once or twice in the REPL while debugging, or maybe 3-10 times when intercepting OpenGL errors. Did I miss a use-case?
I would guess that the project's huge compilation time comes from pre-emptively filling in the GLENUM lookup dictionary, because the GLENUM struct also has a type parameter for the Symbol, which means it's compiling hundreds of specializations upon loading the package. Speaking of which, is there a reason behind that, or would you be interested in a commit which removes it? It seems redundant anyway because it already keeps the Symbol as a field.
I'd be surprised if there are any cirumstances in which you'd send GLENUM to C. Wouldn't external libraries want plain GLenum values, rather than this package's special wrapper?
There was a problem hiding this comment.
@SimonDanisch Sorry, I think I misunderstood your original comment. I assumed you were talking about my new implementation of the GLENUM constructor, dispatching on Val(UInt32(i)). But it sounds like you were talking about the first type parameter of GLENUM{Sym, T}? That was already there before I made these changes, and as I said above I agree we could get rid of it.
I'm still a bit confused on the circumstances where you'd want to pass a GLENUM into C, as opposed to a plain GLenum.
Also, I just checked over the code, and I was mistaken about compilation times. The lookup dictionary for enums doesn't contain a bunch of GLENUM instances, merely the symbols, so there's no excessive compilation there. The main cost of compilation is most likely the sheer overhead of setting up so many functions and constants, which is probably hard to avoid.
On the note of my new GLENUM() constructor, I just discovered that Julia has a @nospecialize macro, which should guarantee that the compiler only makes 3 overloads -- one for Val{1}, another for Val{0}, and then the catchall for any other Val.
TL;DR I'll cut out the Sym type parameter from GLENUM, and also throw @nospecialize in there.
There was a problem hiding this comment.
After updating the code, it takes about 5 seconds to precompile ModernGL on my machine, and then starting up a window (which involves a number of GL calls, GLFW calls, and also my own code) altogether takes ~3 seconds.
There was a problem hiding this comment.
Oh and as for grouping the GL enums so you can see all the names for each value, that would be nice, but I think it would need extra engineering work. A naive implementation would lead to creating thousands of tiny dictionaries, just for a little debugging helper! I think you'd want some kind of sparse matrix. Replacing the current dictionary with something like that would probably merit its own branch.
For the record, this branch doesn't change how the dictionary lookup works; I just cleaned it up a bit by pulling its declaration out of the macro, and changed GLENUM() from having one method to having three.
There was a problem hiding this comment.
Sorry I forgot to answer. I did actually missunderstand your PR. I thought it was replacing the constants with const GL_CONSTANT = GLENUM(...), which would put a lot more pressure on the implementation of GLENUM...
There was a problem hiding this comment.
Ah, that makes sense now :D
Well we've got the important bug-fixes merged in, and there's already an experimental branch using Clang, so it's up to you if you still want the other stuff from this PR. I'll try to fix the merge conflicts tonight (this branch should steamroll over the other one).
Either way, do you want some ARB extensions from another branch of mine? I only added a few that were relevant to me, mainly ARB_bindless_textures.
What was the error? I'm wondering if this will fix a bug in Makie! |
const GL_ABC = convert(GLenum, 123). However, the only really important information to declare is 1) the enum's name, 2) the enum's type, and 3) the enum's value. So I changed the syntax to be the simplerGL_ABC::GLenum = 123, which the macro then expands out into the original statement. And the type is assumed to beGLenumby default, so most of the declarations are even simpler:GL_ABC = 123Base.print()andBase.show()for the GLENUM type so that it's a lot easier to read at a glance. When printing, it looks like "GLENUM(GL_TRUE, 1)". When using show(), it looks like "GL_TRUE<1>".