Skip to content
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

Defined GLAD_GL_IMPLEMENTATION to GLFW example #464

Merged
merged 4 commits into from
Apr 7, 2024

Conversation

makichiis
Copy link
Contributor

@makichiis makichiis commented Apr 3, 2024

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.

Also added .vscode to .gitignore. If this is
an issue please let me know so I can remove
it before the pull.
@makichiis
Copy link
Contributor Author

Sorry, finger slipped

@Dav1dde
Copy link
Owner

Dav1dde commented Apr 3, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

makichiis commented Apr 3, 2024

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
Copy link
Owner

Dav1dde commented Apr 3, 2024

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
Copy link
Contributor Author

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 Dav1dde merged commit 5dd44f1 into Dav1dde:glad2 Apr 7, 2024
1 check passed
@Dav1dde
Copy link
Owner

Dav1dde commented Apr 7, 2024

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants