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

Build system overhaul #332

Merged
merged 17 commits into from
Sep 7, 2024
Merged

Build system overhaul #332

merged 17 commits into from
Sep 7, 2024

Conversation

Vultraz
Copy link
Contributor

@Vultraz Vultraz commented May 31, 2022

Edit by Rotonen: I've ended up hijacking this and this fell victim to massive scope creep - the below is an edit from me for clarity and context.

  • Submodule Imgui and continue to vendor it in

    • Use upstream Imgui and not the Anura specific fork
    • Move the Anura specific extensions of Imgui to the Anura repo and use the appropriate extension mechanism
  • Cleanups

    • Chuck truly unused parts (iOS, Android, old external libs, old config, various build system bits and bobs) out
    • Chuck a now-broken OpenGL ES 2.0 renderer (does not compile anymore) out
  • Build system changes on Linux

    • Release builds /w maximum performance
    • Debug builds /w maximum debuggability
  • Migrate Linux build system to CMake

    • Map all warnings emitted in a way they can be easily peeled off over time
    • Prevent new warnings from being introduced
    • Dynamic Builds against OS provided libraries
    • Static builds against vcpkg provided libraries for SteamOS
  • CI

    • Informative builds for dynamic Linux builds against popular distributions

@DDR0
Copy link
Contributor

DDR0 commented Jun 4, 2022

Where does imgui come from for Linux, which remains (stubbornly, perhaps) off the vcxpkg manager?

@DDR0
Copy link
Contributor

DDR0 commented Jun 4, 2022

Wait, found it. It's libimgui-dev.

@DDR0
Copy link
Contributor

DDR0 commented Jun 4, 2022

We'll have to clean up the makefile a bit probably, here, but it should work. 🤔 This will be good to have, having imgui as a submodule was such a stumbling block.

@DDR0
Copy link
Contributor

DDR0 commented Jun 4, 2022

Hm, so I'm compiling with USE_IMGUI=no make here, to disable the stuff in the makefile since you removed the flags from code, and I'm getting errors like

src/kre/imgui_impl_sdl_gl3.h:11:10: fatal error: imgui.h: No such file or directory
   11 | #include <imgui.h>

The examples from imgui say #include "imgui.h", and I've got the file on disk at /usr/include/imgui/imgui.h more or less as expected. Any ideas?

@Vultraz Vultraz force-pushed the unvendor-imgui branch 4 times, most recently from d4a5841 to f9b6280 Compare December 28, 2022 11:05
@DDR0
Copy link
Contributor

DDR0 commented May 3, 2023

Just to update this, it works perfectly fine for me right now.

@DDR0
Copy link
Contributor

DDR0 commented May 3, 2023

Actually, never mind. I'm getting a segfault when I try to run the game. (trunk works fine)

GL_NV_fog_distance GL_NV_half_float GL_NV_light_max_exponent GL_NV_packed_depth_stencil
GL_NV_primitive_restart GL_NV_shader_atomic_int64 GL_NV_texgen_reflection GL_NV_texture_barrier
GL_NV_texture_env_combine4 GL_NV_texture_rectangle GL_NV_vdpau_interop GL_OES_EGL_image
GL_OES_read_format GL_S3_s3tc GL_SGIS_generate_mipmap GL_SGIS_texture_border_clamp
GL_SGIS_texture_edge_clamp GL_SGIS_texture_lod GL_SUN_multi_draw_arrays

Thread 1 "anura" received signal SIGSEGV, Segmentation fault.
0x0000555555aceef0 in stbrp__skyline_find_min_y (first=first@entry=0x555557791298, x0=511, 
    width=width@entry=0, pwaste=pwaste@entry=0x7fffffffc9ac, c=0x555557791260) at src/kre/stb_rect_pack.h:275
275        STBRP_ASSERT(node->next->x > x0); // we ended up handling this in the caller for efficiency
(gdb) bt
#0  0x0000555555aceef0 in stbrp__skyline_find_min_y (first=first@entry=0x555557791298, x0=511, 
    width=width@entry=0, pwaste=pwaste@entry=0x7fffffffc9ac, c=0x555557791260) at src/kre/stb_rect_pack.h:275
#1  0x0000555555acf96f in stbrp__skyline_find_best_pos (height=0, width=0, c=0x555557791260)
    at src/kre/stb_rect_pack.h:331
#2  stbrp__skyline_pack_rectangle (height=0, width=0, context=0x555557791260) at src/kre/stb_rect_pack.h:414
#3  stbrp_pack_rects (context=0x555557791260, rects=0x555557792d90, num_rects=2)
    at src/kre/stb_rect_pack.h:544
#4  0x0000555556545990 in ImFontAtlasBuildPackCustomRects(ImFontAtlas*, void*) ()
#5  0x0000555556549401 in ImFontAtlasBuildWithStbTruetype(ImFontAtlas*) ()
#6  0x00005555565472a5 in ImFontAtlas::GetTexDataAsAlpha8(unsigned char**, int*, int*, int*) ()
#7  0x0000555556547347 in ImFontAtlas::GetTexDataAsRGBA32(unsigned char**, int*, int*, int*) ()
#8  0x0000555555adb89b in ImGui_ImplSdlGL3_CreateFontsTexture () at src/kre/imgui_impl_sdl_gl3.cpp:220
#9  0x0000555555adc581 in ImGui_ImplSdlGL3_CreateDeviceObjects () at src/kre/imgui_impl_sdl_gl3.cpp:335
#10 0x0000555555adc7c5 in ImGui_ImplSdlGL3_NewFrame (window=0x555556cab820)
    at src/kre/imgui_impl_sdl_gl3.cpp:426
#11 0x0000555555b842d6 in KRE::SDLWindow::clear (f=KRE::ClearFlags::ALL, this=0x555556c46fa0)
    at /usr/include/c++/12/bits/shared_ptr_base.h:1665
#12 KRE::SDLWindow::clear (f=KRE::ClearFlags::ALL, this=0x555556c46fa0) at src/kre/WindowManager.cpp:300
#13 KRE::SDLWindow::createWindow (this=0x555556c46fa0) at src/kre/WindowManager.cpp:282
#14 0x0000555555b80e5a in KRE::WindowManager::createWindow (this=this@entry=0x7fffffffd2d0, wnd=...)
    at src/kre/WindowManager.cpp:701
#15 0x0000555555a6a94d in main (argcount=<optimized out>, argvec=<optimized out>) at src/main.cpp:1050

@DDR0
Copy link
Contributor

DDR0 commented May 3, 2023

At some point, we seem to have broken compiling like OPTIMIZE=no make. I've filed this as #355. It's not a blocker for this branch, but it has delayed investigations into the error above for sure.

@DDR0
Copy link
Contributor

DDR0 commented May 3, 2023

OK, so after quite some jiggling about with the drawers and rummaging around in the cupboards here, I have determined that STBRP_ASSERT(node->next->x > x0); is segfaulting because node->next is 0, in src/kre/stb_rect_pack.h:stbrp__skyline_find_min_y(). node is passed in as first to the function. So it looks like it's just flat-out not handling the case where we're passed a linked list of length 1.

This is in a loop in src/kre/stb_rect_pack.h:stbrp__skyline_find_best_pos() line 329, which goes while (node->x + width <= c->width) {…}, but I have no idea why it's doing what it's doing. Adding a check for node->next in the loop condition causes STBRP_ASSERT(node->next->x > x0); to be triggered later, so that's no good. This code doesn't seem to deal with resource exhaustion that I can find.

Taking a step back, I think this is all caused because we've vendored ANOTHER file into our codebase, imgui_impl_sdl3.cpp. It seems to be ... this has something to do with how you use imgui. You put a backend into your codebase. The imgui_impl_sdl3.cpp backend was added experimentally in 1.89.3 three weeks ago, and I've got libimgui-dev 1.86+ds-1+b1 here from Debian's apt, so presumably a fix for this was implemented sometime in the last three versions. Comparing the version here with the version in trunk, the one here exposes ImGui_ImplSdlGL3_RenderDrawLists... which seems pretty minor, really. So I'm not sure what's wrong, or how to fix it other than blindly trying to upgrade my imgui library version.

@DDR0
Copy link
Contributor

DDR0 commented Jul 14, 2023

OK, I think we've got everything working on the Linux side of things now.

@Rotonen Rotonen force-pushed the unvendor-imgui branch 2 times, most recently from 4bc11ea to 496c521 Compare July 18, 2023 17:35
@Rotonen
Copy link
Contributor

Rotonen commented Jul 18, 2023

New plan here: we will allow three use cases.

  1. Everything from vcpkg (Linux, Windows, macOS)
  2. Everything from distro packages (Debian, Ubuntu, maybe openSUSE Leap)
  3. Imgui via a git submodule, but not the anura fork, the upstream one (Fedora, probably openSUSE Leap)

The only amendments I expect to need here have to do with amending the commit which drops the submodule to change its origin and then touching a couple of the per PR pipelines to use the appropriate method to for obtaining imgui.

This way we also remain easily packageable on Debian.

This currently being red is fully expected and I'll need to amend both a couple of the commits and also a few of the pipelines themselves.

@Rotonen Rotonen force-pushed the unvendor-imgui branch 8 times, most recently from e917132 to b246807 Compare July 19, 2023 05:36
@Rotonen
Copy link
Contributor

Rotonen commented Jul 19, 2023

Seems some ld are pickier than others. On Debian based distributions it would seem if you define -lname then name.o, name.so and name.a are all fair game for the linker to find on the LD search path. On Fedora and SUSE it'd seem only name.so and name.a are fair game. For an imgui we build from a submodule we produce an imgui.o.

Additionally, some api change of imgui is getting us, but somewhat scarily that only seems to trip clang up and g++ just happily compiles it anyway. For this kind of stuff I eventually want to peel off all the -Wno- stuff and compile on many compilers so we get better early warnings like this (if we'd have a flag silencing warnings for implicit conversions this'd not blow up here - this blowing up here is very good).

@Rotonen Rotonen force-pushed the unvendor-imgui branch 2 times, most recently from 11704ab to 47baab9 Compare July 19, 2023 18:20
@Rotonen
Copy link
Contributor

Rotonen commented Jul 2, 2024

Descoping Windows and macOS off this PR for now and adding the new distro versions which have popped up in the meanwhile.

@Rotonen Rotonen marked this pull request as ready for review July 2, 2024 12:22
@Rotonen Rotonen enabled auto-merge July 2, 2024 12:28
@Rotonen Rotonen requested a review from DDR0 September 7, 2024 15:53
Copy link
Contributor

@DDR0 DDR0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have built the project, and it runs fine for me now. A massive thank you to everyone involved. 🙏

@Rotonen Rotonen added this pull request to the merge queue Sep 7, 2024
Merged via the queue into trunk with commit 4c93f9d Sep 7, 2024
74 checks passed
@Rotonen Rotonen deleted the unvendor-imgui branch September 7, 2024 16:21
@DDR0
Copy link
Contributor

DDR0 commented Sep 8, 2024

end of an era

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

Successfully merging this pull request may close these issues.

3 participants