Skip to content

remove non-existent function calls#358

Open
cone-forest wants to merge 4 commits intonmwsharp:masterfrom
cone-forest:master
Open

remove non-existent function calls#358
cone-forest wants to merge 4 commits intonmwsharp:masterfrom
cone-forest:master

Conversation

@cone-forest
Copy link

@cone-forest cone-forest commented Sep 4, 2025

ImGui_ImplOpenGL3_DestroyFontsTexture and
ImGui_ImplOpenGL3_CreateFontsTexture were removed on 04.06.2025

If glm is already a target then no GLM_ENABLE_EXPERIMENTAL define might be specified for it. A more robust way is to define it explicitly

Lets say we have 3 projects: A, B, C. A depends on B and C, both of which use stbi. If stb implementation is not marked as static then you get multiple definition errors during link time because stb implementation symbols are exposed in a compiled library

`ImGui_ImplOpenGL3_DestroyFontsTexture` and
`ImGui_ImplOpenGL3_CreateFontsTexture` were removed on 04.06.2025
@cone-forest cone-forest force-pushed the master branch 3 times, most recently from 63394d8 to f325c67 Compare September 7, 2025 20:31
@cone-forest
Copy link
Author

@nmwsharp This PR fixes compilation/linking errors. Would appreciate your review and merge 😊

@nmwsharp
Copy link
Owner

Hi! Thanks for submitting this. Pardon the delay.

ImGui_ImplOpenGL3_DestroyFontsTexture

Right now this library uses ImGui 1.91.9b, which looks to be before this change. Next time we bump the dependency I will certainly remove those, but I don't think it makes sense to remove them before bumping the dependency.

GLM_ENABLE_EXPERIMENTAL

It took me a minute to understand this. The problem is related to doing the define in CMake, which we currently do, right? Adding the define to the source directly makes sure our functions compile even if the target was defined elsewhere? I think that makes sense to me, thanks for the fix.

STB_IMAGE_STATIC

Good catch, thanks.

@nmwsharp
Copy link
Owner

For now I'll remove the ImGUI change so I can merge this PR with the other two.

@nmwsharp
Copy link
Owner

Hmmmm, note the build failures from the CI. I think to use STB_IMAGE_STATIC we'd need to also switch to including the implementation in every file which uses it, right?

#define STB_IMAGE_STATIC
#define STB_IMAGE_IMPLEMENTATION
#include "stb_image.h"

It's been 2 years since I had to think about C++ symbol visibility, so I might be missing something basic here.

@cone-forest
Copy link
Author

@nmwsharp

Sorry, for the delay on my end too!

The problem with STB is indeed present. The fix I proposed here worked for me because I had a binary linking to both polyscope and stb (by including it). Therefore I had some double definition errors. When I removed STB from my dependencies (moved to wuffs), I got the no-definition linker errors. Not sure how to fix this then

Since it's been half a year since I opened this PR, I no longer have the context in my head ;( I used CPM package manager and I believe the imgui and glm were downloaded by it and had latest versions. polyscope's cmake has find_package I believe so it picked them up. I personally would encourage to use a package manager but since I am the only one with these issues (and I am using a pm), I guess it's my problem :)

The glm fix is harmless and useful though, lets only keep this one. And yes, you understood everything correctly. The define in cmake only happens if glm isn't present on the system. But if it is, then there is no define propagated to it.

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