-
Notifications
You must be signed in to change notification settings - Fork 610
[linux] Add GLFW-based proc resolving #184
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
[linux] Add GLFW-based proc resolving #184
Conversation
c020315
to
11870e5
Compare
It seems like this is patching over an issue with the engine fix for #135. @chinmaygarde any thoughts on why some configurations are still not working without this? Can it be fixed in the engine? |
library/linux/src/embedder.cc
Outdated
// Resolves the address of the specified OpenGL or OpenGL ES | ||
// core or extension function, if it is supported by the current context. | ||
static void* GLFWProcResolver(void *user_data, const char *name) { | ||
return (void*) glfwGetProcAddress(name); |
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.
If we do end up needing this fix on the embedding side, this needs to be static_cast<void*>(...)
. This project follow's Google's C++ style guide, which does not use C-style casts.
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 only could get it to work with reinterpret_cast
. But please double check what I'm doing because I'm not very familiar with C++.
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'll need to look at this again when I have more time. Having looked at the types, this is reinterpret-casting a function pointer to a void*, which I believe violates the spec—in which either the engine API is problematic, or there's a missing step here.
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 dug into this more to see what Flutter code this was ultimately interacting with and how they handle the mismatch. It turns out that it eventually comes down to dlsym
, so this is the issue described in the Rationale section here. Pushing the requirement for the non-standards-compliant conversion up to this abstract layer, rather than keeping it closer to the dlsym
that requires POSIX, does seem like an engine API problem.
However, since the bug is breaking a bunch of people, I'm going to go ahead and land this, and then follow up with the Flutter team separately about whether there should be more changes here at the engine level. Thanks for tracking down the issue!
11870e5
to
524feb8
Compare
Fixes #170
The default symbol resolver that is used when no specific resolver is given doesn't resolve according to OpenGL features available in the GLFW context. This causes errors for some users.