BrutPitt / imGuIZMO.quat

ImGui GIZMO widget - 3D object manipulator / orientator
BSD 2-Clause "Simplified" License
378 stars 41 forks source link

Unflexible include path of imgui (imGuIZMO.quat v3.0) #5

Open moritz-h opened 4 years ago

moritz-h commented 4 years ago

This library makes the assumption that the imgui headers are within an imgui directory.

#ifdef IMGUIZMO_USE_ImGui_FOLDER
    #include <ImGui/imgui.h>
    #include <ImGui/imgui_internal.h>
#else
    #include <imgui/imgui.h>
    #include <imgui/imgui_internal.h>
#endif

I think that is a very unflexible assumption. The imgui library itself does not have any default given in how and where to install the header files, also the official examples does not assume an imgui directory (https://github.com/ocornut/imgui/blob/v1.75/examples/example_glfw_opengl3/main.cpp). It would be nice to be able to use this library without assuming this directory structure. And as you can see you already added some kind of define wrapper, because there is no default. I think it would be more clear to let the build system define where the imgui headers are located by defining the correct include path. Then there would be no define needed and you can just do #include <imgui.h>.

Otherwise if you like to keep the current state it would be at least nice to add an extra define to choose no imgui directory in the include path:

#ifdef IMGUIZMO_USE_NO_FOLDER
    #include <imgui.h>
    #include <imgui_internal.h>
#elif defined(IMGUIZMO_USE_ImGui_FOLDER)
    #include <ImGui/imgui.h>
    #include <ImGui/imgui_internal.h>
#else
    #include <imgui/imgui.h>
    #include <imgui/imgui_internal.h>
#endif
BrutPitt commented 4 years ago

Nothing to object, this is more flexible.. mostly if you have imgui directory in the INCLUDE search path: I'll the defines also in the vConfig file.

Already there is a CMakeFile.txt "internal" variable (not yet exported), I'll export it for user settings.

BrutPitt commented 4 years ago

I was thinking of making it even more flexible and less invasive, by specifying the path directly with:

// without quotes and with final slash
#define IMGUIZMO_IMGUI_FOLDER imgui/

And in the code (but is transparent for users):

#define JOIN(S) S
#define HASH_STR(S) #S
#define STR(S) HASH_STR(S)
#define PATH(A, B) STR(JOIN(A)JOIN(B))

#include PATH(IMGUIZMO_IMGUI_FOLDER,imgui.h)
#include PATH(IMGUIZMO_IMGUI_FOLDER,imgui_internal.h)

what do you think about it? It seems works (MSVC, CLang GCC), but I need to get more feedbacks ... but so the path is relative (between "path")

moritz-h commented 4 years ago

I see no big advantages in this solution. This feels a little bit like reimplementing the compiler include path functionality. So either I need to add -Ipath/to/imgui or -DIMGUIZMO_IMGUI_FOLDER=path/to/imgui (or similar, however this looks in the chosen build system). Also the code becomes much more less readable with this macros.

Perhaps the only reason I can think of would be, with this solution you can maintain backwards compatibility, when the default remains "imgui".

But I have no strong opinion on this. I am happy with any flexible solution that can be configured from the outside within my build system.

BrutPitt commented 4 years ago

I had actually taken a somewhat winding road... i can get the absolute path simply:

#define GET_PATH(B) B
#define INC_PATH(A) <GET_PATH(IMGUIZMO_IMGUI_FOLDER)A>

#include INC_PATH(imgui.h)
#include INC_PATH(imgui_internal.h)

This feels a little bit like reimplementing the compiler include path functionality. So either I need to add -Ipath/to/imgui or -DIMGUIZMO_IMGUI_FOLDER=path/to/imgui (or similar, however this looks in the chosen build system). Also the code becomes much more less readable with this macros.

Yes, via compiler parameters it seems a re-implementation of -I flag

Perhaps the only reason I can think of would be, with this solution you can maintain backwards compatibility, when the default remains "imgui".

The imgui (without capitalization) is the path that is created when you call git clone command, or add dear ImGui inside your repo as sub-module, or fork it.

Also with your previous solution the compatibility with previous versions would be maintained, and also easier to implement... just this solution would have even more flexibility.

So an acceptable compromise could be: default folder: imgui/ when IMGUIZMO_IMGUI_FOLDER is undefined

#if !defined(IMGUIZMO_IMGUI_FOLDER)
    #define IMGUIZMO_IMGUI_FOLDER imgui/
#endif

If you don't want to use path, just use: -DIMGUIZMO_IMGUI_FOLDER= These two cases are the most important, for the others or for a more complex/personal path, it could be configured in vConfig.h, with the convenience to do it only once (for all personal projects).

//------------------------------------------------------------------------------
// imGuiZmo.quat - v3.0 and later - (used only inside it)
//
//      used to specify where ImGui include files should be searched
//          #define IMGUIZMO_IMGUI_FOLDER  
//              is equivalent to use:
//                  #include <imgui.h>
//                  #include <imgui_internal.h>
//          #define IMGUIZMO_IMGUI_FOLDER myLibs/ImGui/
//              (final slash is REQUIRED) is equivalent to use: 
//                  #include <myLib/ImGui/imgui.h>
//                  #include <myLib/ImGui/imgui_internal.h>
//          Default: IMGUIZMO_IMGUI_FOLDER commented/undefined
//              is equivalent to use:
//                  #include <imgui/imgui.h>
//                  #include <imgui/imgui_internal.h>
//
// N.B. Final slash to end of path is REQUIRED!
//------------------------------------------------------------------------------
// #define IMGUIZMO_IMGUI_FOLDER ImGui/

So, also the old ImGui/ capitalized folder is contemplated. (but it was a my old habit to associate ImGui namespace to path)

moritz-h commented 4 years ago

Unfortunately just defining IMGUIZMO_IMGUI_FOLDER does not help. When I define it without value it expands to 1, so it will try to include 1imgui.h. Therefore I need to explicitly use IMGUIZMO_IMGUI_FOLDER=./. Then it works for me.

BrutPitt commented 4 years ago

Yes, you are right, my mistake: I wanted to write: -DIMGUIZMO_IMGUI_FOLDER= (an assignment is required, also empty, otherwise it's 'obiously' 1, without '=')

Corrected also above.

eliasdaler commented 3 years ago

This is not a conventional way of doing includes, I'm afraid...

Things like this should be handled via include search paths and not via macros. At the very least IMGUIZMO_IMGUI_FOLDER should be not used by default (e.g. I won't need to set it to "empty" value to be able to use it).

For people who stumble upon this. This is what worked for me in CMake:

add_library(imguizmo_quat
    "imGuIZMO.quat/imGuIZMOquat.cpp"
)
target_compile_definitions(imguizmo_quat PUBLIC -DIMGUIZMO_IMGUI_FOLDER=) # this "=" is needed for it to work