RobertBeckebans / RBDOOM-3-BFG

Doom 3 BFG Edition source port with updated DX12 / Vulkan renderer and modern game engine features
https://www.moddb.com/mods/rbdoom-3-bfg
GNU General Public License v3.0
1.44k stars 251 forks source link

Missing imgui.h #733

Closed runlevel5 closed 1 year ago

runlevel5 commented 1 year ago
# last commit 2c6b593191ac06db0ae18d584aecf660a590bf1c

$ uname -ar
Linux toolbox 6.2.0-0.rc1.14.fc38.ppc64le #1 SMP Mon Dec 26 16:44:33 UTC 2022 ppc64le ppc64le ppc64le GNU/Linux

$ gcc --version
gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ cd ..
$ rm -rf build
$ mkdir build
$ cd build
$ cmake -G "Unix Makefiles" -DCMAKE_BUILD_TYPE=Release -DUSE_VULKAN=OFF -DFFMPEG=ON -DBINKDEC=OFF -DUSE_SYSTEM_ZLIB=ON -DUSE_SYSTEM_LIBPNG=ON -DUSE_SYSTEM_LIBJPEG=ON -DUSE_SYSTEM_LIBGLEW=ON -DUSE_SYSTEM_RAPIDJSON=ON ../neo
$ make
...
# 2 errors about missing imgui/imgui.h

Patches to fix them are:

diff --git a/neo/renderer/NVRHI/RenderBackend_NVRHI.cpp b/neo/renderer/NVRHI/RenderBackend_NVRHI.cpp
index 1b19436a..213839af 100644
--- a/neo/renderer/NVRHI/RenderBackend_NVRHI.cpp
+++ b/neo/renderer/NVRHI/RenderBackend_NVRHI.cpp
@@ -35,7 +35,7 @@ If you have questions concerning this license or the applicable additional terms
 #include "../RenderCommon.h"
 #include "../RenderBackend.h"
 #include "../../framework/Common_local.h"
-#include "../../imgui/imgui.h"
+#include "../../libs/imgui/imgui.h"
 #include "../ImmediateMode.h"

 #include "nvrhi/utils.h"
diff --git a/neo/tools/imgui/util/Imgui_IdWidgets.cpp b/neo/tools/imgui/util/Imgui_IdWidgets.cpp
index a4d2d9e4..50a7d241 100644
--- a/neo/tools/imgui/util/Imgui_IdWidgets.cpp
+++ b/neo/tools/imgui/util/Imgui_IdWidgets.cpp
@@ -30,7 +30,7 @@ If you have questions concerning this license or the applicable additional terms
 #include "precompiled.h"
 #pragma hdrstop

-#include "../imgui/imgui.h"
+#include "../../../libs/imgui/imgui.h"

 #include "Imgui_IdWidgets.h"
RobertBeckebans commented 1 year ago

Is this resolved with the current master branch?

SRSaunders commented 1 year ago

Had a quick look and it seems that the -DUSE_SYSTEM_<*>=ON settings in the above config parameters are somehow exposing this latent issue. Without them the issue is not evident. Strange as none of them directly include imgui.h.

I can duplicate the second compile failure (i.e. in Imgui_IdWidgets.cpp) by using the above settings in the current master branch on macOS (except for -DUSE_SYSTEM_RAPIDJSON=ON which is not installed for me).

The proposed changes look fine, but I think using #include "libs/imgui/imgui.h" vs. relative paths in both cases may be more resilient and consistent with imgui.h includes elsewhere in the code. Tested on macOS and working fine.

Good catch @runlevel5 !

(FYI: -DUSE_VULKAN=OFF is now forced ON for the NVRHI stream, and -DUSE_SYSTEM_LIBGLEW=ON is now a no-op given the mandatory NVRHI Vulkan back-end on linux/macOS -- both of these options are effectively ignored)

coldtobi commented 1 year ago

The proposed changes look fine, but I think using #include "libs/imgui/imgui.h" vs. relative paths in both cases may be more resilient and consistent with imgui.h includes elsewhere in the code. Tested on macOS and working fine

Please dont. This will make it harder to use any imgui by distributions. I'd suggest to change it to "imgui/imgui.h" and make sure that libs is in the include path. (I've recently got a bug report about that in Debian, https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1028279, however this is about cross compilation but still to the topic… This bug has also a patch attached (which targets the Debian package, so this patch will create a patch file, if you wonder why it looks that way…)

The "make sure that libs is in the include path" is one part of this PR: https://github.com/RobertBeckebans/RBDOOM-3-BFG/pull/657 (the changes to CMakeLists.txt)

SRSaunders commented 1 year ago

I thought the whole point of Dear ImGui is to be a small, efficient source lib that is statically linked to an executable. It is quite different than ffmpeg, openal, zlib, libpng, libjpg, etc that are sizeable and officially distributed as shared libs. Do Debian's code copy policies apply to small source utils like ImGui that are not distributed in shared object form?

coldtobi commented 1 year ago

Do Debian's code copy policies apply

Yes they apply. A longer explanation.

SRSaunders commented 1 year ago

Thanks Tobias for the links. The main points I see are: a) a reasonable desire to eliminate convenience copies of code where other shareable dependency options are available, and b) a desire to promote shared libs over static linking. While these are laudable goals in many cases, ImGui may not be the best candidate. As far as I am aware, only some distros include ImGui as a shared lib, and the project itself encourages configuration-specific customization with static linking to an executable - in order to freeze the API and behaviour for a target app. See https://github.com/ocornut/imgui/issues/2084 for an (older) discussion re Debian packaging with the project owner.

Nonetheless, I am certainly not an expert in distro packaging and policy rules and the thinking may have evolved since then. Perhaps the best way forward would be to fix the specific #include defects raised by @runlevel5. And if you want to pursue the general problem further with Robert, you could rebase your PR #657 against the current master branch and open a new discussion around that.

coldtobi commented 1 year ago

a) and b) are merely results/effects of avoiding convenience copies. The footnote of the Debian policy states a few reasons:

"Having multiple copies of the same code in Debian is inefficient, often creates either static linking or shared library conflicts, and, most importantly, increases the difficulty of handling security vulnerabilities in the duplicated code."

And security should be a concern… Take look for example at the bundled libpng -- it is at the (ancient) version, 1.2.49 , which has several known security issues -- look at http://www.libpng.org/pub/png/libpng.html , however note that security issues for 1.2.x might not be listed completely: as upstream says: "(Greg no longer bothers to list either series here; enough's enough, folks. Update those apps now!) ". If RBDoom3BFG wants to be a platform for modders and game designers, this problem is real and also not limited to libpng (cough jpeg6b cough)

you could rebase your PR https://github.com/RobertBeckebans/RBDOOM-3-BFG/pull/657

I can't rebase #657 atm, because master does not compile for me (it does not find "DXC_SPIRV_EXECUTABLE", whatever that is; Nvhri is currently not packaged for Debian (AFAIK), but the vendor copy is modified anyway so the problem space just got bigger… I'm not complaining, don't get me wrong and was clearly my decision to package it for Debian, I get it that for Robert distributions (and Linux in general maybe) are not a priority; Currently Debian is approaching the freeze for its next release, its just ENOTIME to take a deeper look. Debian 12 will have rbdoom3bfg 1.4.

SRSaunders commented 1 year ago

Would it help if the defect was fixed by the following method:

  1. Add include_directories("libs/imgui") to CMakeLists.txt
  2. Change all */imgui.h includes to look like #include "imgui.h"

FYI from the README regarding DXC_SPIRV_EXECUTABLE:

Go to https://github.com/microsoft/DirectXShaderCompiler and download the DXC binaries for Linux and put them into your local PATH.

RobertBeckebans commented 1 year ago

Bundling Imgui and other libraries is the best way to make sure the software works as intended. The Imgui part is even extended with imgui_stdlib.{h/cpp} which is a special id Tech 4 adapted version because the original code wouldn't compile with the new Imgui based Articulated Figure editor.

There are several gamedev projects going on based on RBDOOM-3-BFG and those have a higher priority than Linux support and we can't allow that distribution policies are holding back the development. Almost all development happens on Windows and we just make sure that Linux users can still run the engine. Unfortunately updating all used libraries isn't easy because some libraries are really a big mess like libjpeg. Also the jpeg code id Software shipped with was customized so error messages get printed properly to the console. I think there are similar changes with the recently added Ogg Vorbis code.

Concerning DXC: We did a ton of changes with the NVRHI backend which reduces the maintenance costs of this engine and allows to develop more GPU driven rendering algorithms featuring Raytracing and even more id Tech 7 oriented bindless rendering. Also Microsoft's DXC compiler is a huge gift for every graphics programmer and even the LLVM team considers the adoption of the SPIR-V compile target. We cannot do without it.

If you are just concerned to run DOOM-3-BFG on Linux then version 1.4 is perfectly fine.

The original game assets never ever trigger the jpeg or png related code paths because that is something that only happens when running mods. All images are already DXT compressed and shipped as .bimage files similar to .dds.

coldtobi commented 1 year ago

Robert, thanks for your message and being upfront.

In Software development are many ways to archive things, tons of bike-shedding opportunities… I understand that you want to advance the project, however, those many options we have in SW development does not make it mutual exclusive to have progress and be considerate to downstreams as the same time. For example, that idstr <-> std::string conversion could have been done on engine side without the need to patch imgui. But thats my bikeshed color. The same with libjpeg. The ancient version as well as the modern libjpeg-turbo allows to redirect error messages without even patching it. In the long run, patching third party libraries does make a mess and inhibits easy upgrade paths. Again, my bikeshed color only.

I explicitly acknowledge your bike shed colors, they are ok. I understand your motivation to patch stuff and also to choose a easier way is absolutely OK… The only thing I can do is show you my colors and hope that you will like them.

At least I tried that the PRs I've sent (thanks for merging those) were always designed to blend in the project while archiving those little improvements for downstream distributions.

My goal for packaging rbdoom3bfg was not only to provide the ability to play the game, but to give people access to the engine and I was thrilled to see that it gets some momentum here. So while 1.4.0 will fulfill the first goal, staying at this version will not do good to the other. Eventually the 1.4.0 will bit rot and counting that would mean that eventually I'd run a fork, which I simply would not be able to do so.

The the Developers reference rightfully recommends to have a healthy relationship with upstream, and I always tried to do so and you were also always nice to me, but sometimes it is also a valid realization that those bike shed color are just too different to be sustainable.

Honestly, thanks for RBDOOM-3-BFG, I've enjoyed maintaining it.

DXC / NVRHI

Just to avoid misunderstanding, I don't have a problem with DXC/NVRHI. It is free software and Microsoft has opened up a lot towards the community. Same with NVRHI, even if nvidia is doing pseudo-FOSS on other fronts. Packaging both would for sure be beneficial for distributions, and I probably would have done it just for rbdoom3bfg alone.)

-- tobi