pthom / hello_imgui

Hello, Dear ImGui: unleash your creativity in app development and prototyping
https://pthom.github.io/hello_imgui
MIT License
604 stars 91 forks source link

Improve HELLOIMGUI_WIN32_NO_CONSOLE implementation #99

Closed xuboying closed 3 months ago

xuboying commented 3 months ago

Version: 0576001764e38c1e3b04fceeb30f02ec0b19f866 Platform: Win10 + MSVC + clang tool chain File: hello_imgui_add_app.cmake

Current implantation manually adds link option for MINGW/non-MINGW target which is not necessary, it's also causing a bug for MSVC + clang tool chain as clang could not understand "/subsystem:WINDOWS".

The fix is very simple, CMake supports WIN32 flag in add_executable (https://cmake.org/cmake/help/v3.10/command/add_executable.html) Just PREPAND the option to app_sources before add_executable. BTW, it's also possible to APPEND HelloImGui_WinMain.cpp to app_sources before add_excutable so the entire fix could be very clean:

    #############################################################################
    # If windows, and if the user wants to, we can make this an app without console
    # and provide a WinMain entry point
    if (WIN32)
        if (HELLOIMGUI_WIN32_NO_CONSOLE)
            # Make this an app without console, and use HelloImGui_WinMain.cpp
            list(PREPEND app_sources WIN32)
        endif()
        if (HELLOIMGUI_WIN32_AUTO_WINMAIN)
            list(APPEND app_sources ${HELLOIMGUI_CMAKE_PATH}/HelloImGui_WinMain.cpp)
        endif()
    endif()

    if (ANDROID)
        add_library(${app_name} SHARED ${app_sources})
    else()
        add_executable(${app_name} ${app_sources})
    endif()

    hello_imgui_prepare_app(${app_name} ${assets_location})
pthom commented 3 months ago

Hello,

Thanks for your suggestion! Would you want to provide a PR for this, so that you are cited in the contributors?

Also, I guess you tried this modification with clang. Can you confirm this, and tell me if you had time to also test it with msvc and/or mingw?

xuboying commented 3 months ago

sure, I will try all your suggestions

xuboying commented 3 months ago

Hi again I tried the MSVC and MSVC with Clang tool chain on Win10 VS Code, they are fine.

MSVC version:
[proc] Executing command: C:\usr\cli\cmake\bin\cmake.EXE --no-warn-unused-cli -Wno-dev -DCMAKE_BUILD_TYPE:STRING=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS:BOOL=TRUE -SC:/repos/hello_imgui -Bc:/repos/hello_imgui/build/Debug -G "Visual Studio 17 2022" -T host=x64 -A x64
[cmake] Not searching for unused variables given on the command line.
[cmake] -- Selecting Windows SDK version 10.0.22621.0 to target Windows 10.0.19045.
[cmake] -- The C compiler identification is MSVC 19.38.33130.0
[cmake] -- The CXX compiler identification is MSVC 19.38.33130.0

I would like to try Mingw, however I have no experience on that. I encounter several new issues like cmake platform unknown issue, Makefile multiple target bug(caused by windows driver) , and finally stopped at opengl32 linking problem. I also realized there are many variants of mingw, making troubleshooting ever harder.

So, is it possible to simply testing the mingw result in your CI pipeline?

I have pushed change to my fork: https://github.com/xuboying/hello_imgui/commit/5dce5cc9793628e97a7800b6e4d7feb333a5cd9e

pthom commented 3 months ago

You are right, testing on the multiple platform / compilers is one of the main difficult things in this project.

So, is it possible to simply testing the mingw result in your CI pipeline?

Yes, please submit a PR, and I'll run the pipeline on it. If it does not work on MingW, I'll diagnose it on my side. You did the test I could not do (i.e. with MSVC + clang).

Thanks!

xuboying commented 3 months ago

Thanks!

pthom commented 3 months ago

The mingw pipeline passed. Many thanks for your contribution!