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

Include custom OpenGL loader header from define. #4649

Closed
wants to merge 2 commits into from
Closed

Include custom OpenGL loader header from define. #4649

wants to merge 2 commits into from

Conversation

avostrik
Copy link

In case of Windows and custom loader file, no OpenGL API is visible to OpenGL3 implementation.
It is needed to include loader header that provides it.

Can be provided via CMake for example:

target_compile_definitions(target PRIVATE IMGUI_IMPL_OPENGL_LOADER_CUSTOM="custom_opengl3_loader.h")

In case of Windows and custom loader file, no OpenGL API is visible to OpenGL3 implementation.
It is needed to include loader header that provides it.

Can be provided via CMake for example:

target_compile_definitions(target PRIVATE IMGUI_IMPL_OPENGL_LOADER_CUSTOM="custom_opengl3_loader.h")

Signed-off-by: Andrey Vostrikov <andrey.vostrikov@cogentembedded.com>
@ocornut
Copy link
Owner

ocornut commented Oct 13, 2021

I think this may be a breaking change and in some situation (e.g. unity build #4646) this may be undesirable.
Maybe there should be a separate optional define such as IMGUI_IMPL_OPENGL_LOADER_CUSTOM_INCLUDE ?

@avostrik
Copy link
Author

How does it compile if no GL header is included (in case of defined IMGUI_IMPL_OPENGL_LOADER_CUSTOM)?

On Windows setup with OpenGL backend and custom loader generated with gl3w that is not part of imgui it fails on basic GL types like GLuint and so on inside imgui_impl_opengl3.cpp.

May be I am missing something and custom include can be provided by other means.

Signed-off-by: Andrey Vostrikov <andrey.vostrikov@cogentembedded.com>
@PathogenDavid
Copy link
Contributor

How does it compile if no GL header is included (in case of defined IMGUI_IMPL_OPENGL_LOADER_CUSTOM)?

With Unity builds you build everything as a single file. So you don't build imgui_impl_opengl3.cpp, you include it from your main file which should've already included an OpenGL loader.

On Windows setup with OpenGL backend and custom loader generated with gl3w that is not part of imgui it fails on basic GL types like GLuint and so on inside imgui_impl_opengl3.cpp.

Generally speaking, imgui_impl_opengl3.cpp should end up using Dear ImGui's baby OpenGL loader:

#include "imgui_impl_opengl3_loader.h"

Unless you have a specific need to, you should just let Dear ImGui use it. (IE: Don't define IMGUI_IMPL_OPENGL_LOADER_CUSTOM.)

@avostrik
Copy link
Author

Unless you have a specific need to, you should just let Dear ImGui use it. (IE: Don't define IMGUI_IMPL_OPENGL_LOADER_CUSTOM.)

This was on a first iteration, but re-generating it adding more and more GL API in evolving project is not convenient. (full API generated loader works)
Also, I can't use this repo intact as a git submodule with modified loader file. It is always possible to fork or embed ImGui in the project but this is not a right solution.

@PathogenDavid
Copy link
Contributor

You don't have to (and generally shouldn't) use the minimal Dear ImGui loader directly, it should only be used by the backend. You can continue to use Glad/GL3W/whatever as per usual for your own code.

@avostrik
Copy link
Author

You don't have to (and generally shouldn't) use the minimal Dear ImGui loader directly, it should only be used by the backend. You can continue to use Glad/GL3W/whatever as per usual for your own code.

Unfortunately, two gl3w wrappers in same project do not work together. That is already tried. Nevertheless of my project, if IMGUI_IMPL_OPENGL_LOADER_CUSTOM is enabled and ImGui used as usual with OpenGL backend it does not compile as no OpenGL API header provided.

@ocornut
Copy link
Owner

ocornut commented Oct 14, 2021 via email

@ocornut
Copy link
Owner

ocornut commented Oct 14, 2021

Unfortunately, two gl3w wrappers in same project do not work together. That is already tried.

I think you may be on a XY Problem here.

Two OpenGL loaders in same project does/should work. You should focus on solving this.

@avostrik
Copy link
Author

Two OpenGL loaders in same project does/should work. You should focus on solving this.

Does or should?

If it does, please point me to working example.

I could extract to small test app what does not work, if it will help you understand the issue better.

So far points:

  • ImGui with defined custom loader under windows and app provided gl3w does not compile. No GL API visible to ImGui opengl backend.
  • ImGui with its own gl3w loader doesn't link as app provided gl3w defines same functions implementations. (ok, lets keep ImGui intact, removing implementation from app, hoping that version is the same and implementation is compatible.
  • App crashes on its own GL API call with access error in the opengl driver, even though pointers to GL functions are resolved. (I don't have details at hand to add it now).

@ocornut
Copy link
Owner

ocornut commented Oct 14, 2021

Does or should?

It was already discussed in #4445 and it worked then and no changes have been made.

ImGui with defined custom loader under windows and app provided gl3w does not compile. No GL API visible to ImGui opengl backend.

If you use a custom loader for imgui_impl_opengl3.cpp it needs to expose a GL api, that's the point of a loader.
But you should NOT have to use a custom loader for imgui_impl_opengl3.cpp
The only reason I can currently think of where there is a need to use a custom loader for imgui_impl_opengl3.cpp is unity build (#4646)

ImGui with its own gl3w loader doesn't link as app provided gl3w defines same functions implementations. (

It should not, we discussed this in #4445 and fixed remaining bugs back then. We designed our gl3w fork to not have conflicting linking symbols with gl3w.
Please state exactly what are the conflicts, if there are more we'll fix them.
Note that you did NOT fill the requested issue template and therefore we don't know which version of the Dear ImGui codebase you are using, make sure you are on latest.

The last known fix was merged on Aug 24.

Either way we are discussing a XY Problem. Should solve this instead of using a custom loader for imgui_impl_opengl3.cpp

@avostrik
Copy link
Author

If you use a custom loader for imgui_impl_opengl3.cpp it needs to expose a GL api, that's the point of a loader.

That is exactly the purpose of my pull request. To provide GL API via custom loader header to backend when it is not used as a single translation unit (in case of Unity).

@avostrik
Copy link
Author

avostrik commented Oct 14, 2021

We designed our gl3w fork to not have conflicting linking symbols with gl3w.

This is a root cause with conflicting symbols, I used same scripts to generate custom app loader with extended API set.
May be generator needs to be adjusted to put generated code into dedicated ImGui namespace. So, headers generated by your fork script could produce non-conflicting loaders. And app generated API should go into app-specific namespace.

@ocornut
Copy link
Owner

ocornut commented Oct 14, 2021

Ah, that now explains it. You are using a second copy of our loader.
You should NOT use our loader. Use regular gl3w or anything else.

@ocornut
Copy link
Owner

ocornut commented Oct 14, 2021

The PR is sound if we were to accept that you'll need to use a different loader for imgui_impl_opengl3 in its own compilation unit, but right now there is no reason to do it. (*)

By accepting your use we'd be explicitly taking on the burden that our loader that wasn't designed for other users suddenly become something we should support. OpenGL ecosystem is enough of a mess we don't want our forked loader to be a project we'd provide maintenance for. We explicitely worked on this to stay as far as we could from the mess that is the GL loader ecosystem.

TL;DR; please use any of the million available loaders or even gl3w, but don't use imgl3w in your project please.

(*) And whenever we find another reason that this is useful we can merge this change, but right now I can't merge it for this reason.

@ocornut
Copy link
Owner

ocornut commented Nov 4, 2021

Closing this. Stance of #4649 (comment) stands, you should not use our custom loader for your code we cannot support it (you can use stock GL3W though). Sorry for that.

@ocornut ocornut closed this Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants