SFML / imgui-sfml

Dear ImGui backend for use with SFML
MIT License
1.15k stars 172 forks source link

Better naming and folder structure #80

Open eliasdaler opened 5 years ago

eliasdaler commented 5 years ago

Right now, there's a bit of inconsistency in naming. The repo is named "imgui-sfml", the files are "imgui-SFML.h" and "imgui-SFML.cpp", the CMake target is "ImGui-SFML" Plus, the repo doesn't follow good C++ lib project structure. What I propose is this layout:

include/
    imgui-sfml/
        imgui-sfml.h
        ...
src/
     imgui-sfml.cpp

The CMake target will still be "ImGui-SFML".

In your code, you'll need to do this:

#include <imgui-sfml/imgui-sfml.h>

Please share your thoughts in the thread.

pinam45 commented 5 years ago

I think it's a good idea, this project structure is also followed by SFML, it will be more consistent in the includes.

But we can't have full mandatory consistency, ImGui itself have no special structure. Some CMake can add one to also have a subfolder include (e.g. #include <imgui/imgui.h>) but as imgui-SFML.cpp uses #include <imgui.h>, we still need to have the include without subfolder available.

eliasdaler commented 5 years ago

Good point, that's a pretty difficult thing to figure out...

I can make it so that during install, you'll get this layout (and use <imgui/imgui.h> in imgui-sfml.cpp):

/usr/include/
     imgui/
          imgui.h
    imgui-sfml/
         imgui-sfml.h

Another possibility (and use <imgui.h> in imgui-sfml.cpp):

/usr/include/
    imgui-sfml/
         imgui-sfml.h
         imgui.h

And the worst (and use <imgui.h> in imgui-sfml.cpp):

/usr/include/
     imgui-sfml/
         imgui-sfml.h
    imgui.h

What's happening now is this:

/usr/include/
     imgui.h
     imgui-sfml.h
pinam45 commented 5 years ago

The problem putting imgui.h in /usr/include/ is that someone installing ImGui or another binding wight override it with a future/past unsupported version. Putting imgui.h in /usr/include/imgui/ can lead to the same problem, if someone have the same idea.

The better is maybe

/usr/include/
    imgui-sfml/
         imgui-sfml.h
         imgui.h

to control the ImGui version used. But still if someone install ImGui or another binding with imgui.h in /usr/include/, with #include <imgui.h> there is a file name conflict and it migth resolve to /usr/include/imgui.h or /usr/include/imgui-sfml/imgui.h. So maybe use #include <imgui-sfml/imgui.h> even if it is in the same folder to solve the problem.

eliasdaler commented 5 years ago

@ocornut Any thoughts on where imgui.h should be stored when installing ImGui-SFML?

ocornut commented 5 years ago

I think imgui shouldn't be installed for variety of reason including some highlighted by @pinam45 above. There you have my answer.

eliasdaler commented 5 years ago

So, I guess ImGui-SFML should install without imgui.h and require users to add it to their build manually? That might make it problematic if ImGui-SFML was built with ImGui v1.XX, but then user attempted to use it with ImGui v1.YY and there was ABI change there.

eliasdaler commented 5 years ago

In scope of this, I think it'll be good to start producing 'libimgui-sfml.so' and 'libimgui-sfml.a', not 'libImGui-SFML.so' and 'libImGui-SFML.a'.

ocornut commented 5 years ago

Would be good to stop producing .so files :D

eliasdaler commented 5 years ago

Why? Also, that's an option - the static library is built by default, but there's an option to build a shared one.

ocornut commented 5 years ago

Dear ImGui is not ABI forward nor backward compatible. In addition it is a library that typically involve dozen of thousands of function calls every frame, building as DLL is only going to add extra overhead.

eliasdaler commented 5 years ago

I agree, but it's for the user to decide if they want to build it as DLL or not. IMGUI_API are sprinkled around the code for a reason. ;)

ChrisThrasher commented 1 year ago

ImGui-SFML v3 is a great place to make such a breaking change. I'd love to remove some of those capital letters. They look pretty awkward. I support pretty much everything Elias recommended so long as we don't touch the path to imgui.h since we don't own that header. These changes will be no trouble to implement. I'll likely write this up in the near future.