Dav1dde / glad

Multi-Language Vulkan/GL/GLES/EGL/GLX/WGL Loader-Generator based on the official specs.
https://glad.dav1d.de/
Other
3.79k stars 447 forks source link

Defined GLAD_GL_IMPLEMENTATION to GLFW example #464

Closed makichiis closed 6 months ago

makichiis commented 7 months ago

Patch for issue #463 which can cause portability issues in the future by defining GLAD_GL_IMPLEMENTATION before glad import in GLFW example.

This patch does not stress nor attempt to fix any issues with the library itself, but edits the example in question so consumers don’t make the mistake of not defining the required macro.

An alternative solution might be to include GLFW before glad, but I haven’t had the chance to confirm. Don’t take my word for it.

Also added .vscode to .gitignore. If this is an issue please let me know so I can remove it before the pull.

Thank you @Dav1dde for helping me troubleshoot the version checking issue.

makichiis commented 7 months ago

Sorry, finger slipped

Dav1dde commented 7 months ago

Thanks for the patch, looks good, also adding vscode is fine! I am a bit surprised though, glad should return 0 in this case not 1. I'll have to dig into why it returns 1.

makichiis commented 7 months ago

Hi @Dav1dde,

To my knowledge and experience using GLAD for a couple years, I assumed the gladLoadGL return code was supposed to be a success indicator. At least, I've always used it that way, as have examples that use your code. It would be funny if the root of this was a misconception, but it seems fine. Unless I am misunderstanding the bug?

Also, I don't have merge permissions, so if the patch is sufficient would you mind merging when you have the chance?

Thank you, Sarah

makichiis commented 7 months ago

Oh, I just understood what you meant. If I get the chance I'll look into why glad is being set when the version is not set.

It could be that the loader runs anyway whilst the version checker doesn't? I'll take a gander when I have the chance. Thanks for the insight.

makichiis commented 7 months ago

Hi,

Quick follow up: I updated the fork, because GLFW_INCLUDE_NONE should be included before importing GLFW3 in order to avoid redeclarations of the OpenGL function macros.

Thank you for reading.

Dav1dde commented 7 months ago

Quick follow up: I updated the fork, because GLFW_INCLUDE_NONE should be included before importing GLFW3 in order to avoid redeclarations of the OpenGL function macros.

This is not necessary as long as you include glad before glfw. So I'd not include it in the example because it is not necessary.

To my knowledge and experience using GLAD for a couple years, I assumed the gladLoadGL return code was supposed to be a success indicator.

It is, the previous version used 0 and 1, glad2 uses 0 and some value bigger than 0 but not 1, which means the boolean check still works but you can also extract the loaded OpenGL version from it. Hence why glad shouldn't ever return 1.

Also, I don't have merge permissions, so if the patch is sufficient would you mind merging when you have the chance?

I will, but it might be open for a few days, I want to take the opportunity to figure out why glad returns 1 in the first place.

makichiis commented 7 months ago

Hi @Dav1dde,

I see. Thank you for the follow up. I was trying to reproduce the return 1 issue but I seemed to have failed to documented my reproduction steps. I used the headeronly library when troubleshooting and I'm using it now, but when I undefine GLAD_GL_IMPLEMENTATION it (rightfully) fails to link. I haven't figured out how to reproduce the issue.

Also, I checked glfw3.h again, and confirmed your mentioning that GLFW_INCLUDE_NONE is not necessary when __gl_h_ is defined. Sorry for that oversight, I'll revise this in my next commit.

Thank you.

Dav1dde commented 6 months ago

Thanks for the PR! I had a quick look again my best guess is somehow maybe it linked with an older version of glad, didn't have more time to investigate yet.

makichiis commented 6 months ago

Good morning @Dav1dde,

I'm thinking the same thing must have happened on my end, as well. I could not reproduce the issue, but in trying to do so I actually learned a lot about the library :)

Thanks for keeping up with this branch/issue