-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve GLENUM names/macro/printing #71
Open
heyx3
wants to merge
12
commits into
JuliaGL:master
Choose a base branch
from
heyx3:cleanup-macro
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a99e012
Simplify glConstants definitions, improve @GenEnums macro, allow hard…
heyx3 bc44091
Update project version
heyx3 d6f4a3c
Merge branch 'v4.6' into cleanup-macro
heyx3 340b11a
Merge branch 'v4.6' into cleanup-macro
heyx3 4f8bc31
Simplify the custom GLENUM overloads, add tests for them
heyx3 103af35
Update readme
heyx3 caa73d3
Add OpenGL max version to readme
heyx3 94567d9
Remove accidental new dependency in Project file
heyx3 b3ac5a2
Add more missing constants
heyx3 eb3724a
Add buffer constants
heyx3 6e1fd96
Fix incorrect GLsizei alias
heyx3 91e6ed0
Remove redundant type parameter and force @nospecialize, to reduce JI…
heyx3 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sendGLENUM
to C. Wouldn't external libraries want plainGLenum
values, rather than this package's special wrapper?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimonDanisch Sorry, I think I misunderstood your original comment. I assumed you were talking about my new implementation of the
GLENUM
constructor, dispatching onVal(UInt32(i))
. But it sounds like you were talking about the first type parameter ofGLENUM{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 plainGLenum
.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 forVal{1}
, another forVal{0}
, and then the catchall for any otherVal
.TL;DR I'll cut out the
Sym
type parameter fromGLENUM
, and also throw@nospecialize
in there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofGLENUM
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.