Polytonic / Glitter

Dead Simple OpenGL
http://polytonic.github.io/Glitter/
2.48k stars 418 forks source link

stb_image implementation #13

Closed metiulekm closed 9 years ago

metiulekm commented 9 years ago

Hello from the same guy who bugged you to support MinGW :smiley:

When I actually found time to go through OpenGL tutorials, I noticed one thing in your project. In glitter.hpp (which at least I noted as a file meant to be included in all GL-needing source files and simply grouping all the includes), you define STB_IMAGE_IMPLEMENTATION. This is required by stb_image, but only in one .cpp file (source), as it essentially unpacks the implementation of the library. You can check it yourself by creating another .cpp file and including glitter.hpp in it -- the project will fail to link with multiple redefinition errors. So that #define has to be moved into a .cpp file.

And if this behaviour is intended, then I think you should specify in readme glitter.hpp is only meant to be included in one .cpp file and otherwise it'll break the program.

Polytonic commented 9 years ago

Multiple redefinition errors are more likely related to me forgetting to add include guards. Can you try again with #pragma once?

If I remember correctly (and I'm a bit foggy on this one) #define STB_IMAGE_IMPLEMENTATION needs to go in the same C++ source file where stb_image is used. I'm not sure the preprocessor directive really belongs in main.cpp though.

metiulekm commented 9 years ago

I will look into it in a few hours, I'll be AFK right now. But unless I were really blind yesterday, the stb_image's header does say that this

define goes into cpp file.

17 wrz 2015 07:04 "Kevin Fung" notifications@github.com napisał(a):

Multiple redefinition errors are more likely related to me forgetting to add include guards. Can you try again with #pragma once?

If I remember correctly (and I'm a bit foggy on this one) #define STB_IMAGE_IMPLEMENTATION needs to go in the same C++ source file where stb_image is used. I'm not sure the preprocessor directive really belongs in main.cpp though.

— Reply to this email directly or view it on GitHub https://github.com/Polytonic/Glitter/pull/13#issuecomment-140970996.

Polytonic commented 9 years ago

No no you're very probably right on the define going in the cpp file.

I'm pretty sure your multiple redefinitions error I think is unrelated, and due to the fact that I forgot about include guards.

I won't have access to a computer that can test this until tomorrow night, but I'll poke at it then.

metiulekm commented 9 years ago

Yeah, I pulled the guards fix and it still didn't help. The link still fails after #including glitter.hpp in the second .cpp file (if you don't get the error when you will be testing you probably will have to rerun CMake in order to get the added file). Also, the #define must go before the include, I originally put it below. :frowning:

Polytonic commented 9 years ago

@metiulekm sorry for the delay. I've added a small block comment. Is this any better?

I'm trying to avoid lumping "FAQ" level stuff into the readme, since this is technically a requirement of STB, and I want people to go read the documentation for STB rather than turning this into a "here is a list of every problem ever with associated answer!"

metiulekm commented 9 years ago

Well, I think that's excellent, it explains it well. :smile: