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 · 11 comments
Open

Better naming and folder structure #80

eliasdaler opened this issue May 13, 2019 · 11 comments

Comments

@eliasdaler
Copy link
Owner

@eliasdaler eliasdaler commented May 13, 2019

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

This comment has been minimized.

Copy link

@pinam45 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

This comment has been minimized.

Copy link
Owner Author

@eliasdaler eliasdaler commented May 13, 2019

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

This comment has been minimized.

Copy link

@pinam45 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

This comment has been minimized.

Copy link
Owner Author

@eliasdaler eliasdaler commented May 15, 2019

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

@ocornut

This comment has been minimized.

Copy link

@ocornut 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

This comment has been minimized.

Copy link
Owner Author

@eliasdaler eliasdaler commented May 15, 2019

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

This comment has been minimized.

Copy link
Owner Author

@eliasdaler eliasdaler commented May 20, 2019

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

This comment has been minimized.

Copy link

@ocornut ocornut commented May 20, 2019

Would be good to stop producing .so files :D

@eliasdaler

This comment has been minimized.

Copy link
Owner Author

@eliasdaler 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

This comment has been minimized.

Copy link

@ocornut 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

This comment has been minimized.

Copy link
Owner Author

@eliasdaler eliasdaler commented May 28, 2019

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. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.