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

Better naming and folder structure #80

Open
eliasdaler opened this issue May 13, 2019 · 12 comments
Open

Better naming and folder structure #80

eliasdaler opened this issue May 13, 2019 · 12 comments

Comments

@eliasdaler
Copy link
Contributor

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
Copy link
Contributor

pinam45 commented May 13, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

pinam45 commented May 14, 2019

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
Copy link
Contributor Author

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

@ocornut
Copy link

ocornut commented May 15, 2019

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

@eliasdaler
Copy link
Contributor Author

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 eliasdaler added this to the 3.0 milestone May 20, 2019
@eliasdaler eliasdaler removed the v3.0 label May 20, 2019
@eliasdaler
Copy link
Contributor Author

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
Copy link

ocornut commented May 20, 2019

Would be good to stop producing .so files :D

@eliasdaler
Copy link
Contributor Author

eliasdaler commented May 20, 2019

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

@ocornut
Copy link

ocornut commented May 28, 2019

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
Copy link
Contributor Author

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
Copy link
Member

ChrisThrasher commented Sep 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants