segross / UnrealImGui

Unreal plug-in that integrates Dear ImGui framework into Unreal Engine 4.
MIT License
666 stars 212 forks source link

Fixed build errors when the plugin is placed in the engine directory. #19

Closed kkawachi closed 5 years ago

kkawachi commented 5 years ago

It compiles both in the engine directory and project directory.

segross commented 5 years ago

Hey, sorry that I didn’t notice your request earlier and thank you for your contribution. It is good to know that it can be complied as the engine plug-in.

Unfortunately, after moving pre-compiled headers, this will either not compile or compile with warnings, depending on the engine versions and possibly set-up. This is because, if used, pre-compiled headers should be always included at the beginning. May I ask what is the purpose of that reordering?

Also, does adding categories to FImGuiKeyInfo help with anything, given that this structure already has its own property type customisation?

To compile it as the engine plug-in, is it enough to change those 11 lines that include engine headers?

kkawachi commented 5 years ago

Hi, Thank you for reviewing my pull request.

The purpose of my PR is to compile the plugin as an engine plugin. When the plugin is placed in Engine/Plugins directory, we can see the plugin won't build because of three different kinds of errors. I will describe the details of these errors at the end of this comment.

I tested the modified plugin code with UE-4.21.2 and UE-4.22.3. It compiles and works fine both as engine/project plugins on Visual Studio 2017 in Development_Editor / Win64 config, but I want to improve my PR by decreasing incompatibility with existing code and projects if any.

1. Categories of properties

When the plugin is placed in Engine/Plugins of vanilla UE4 source tree, UnrealHeaderTool produces the following errors. This can be fixed by adding categories to FImGuiKeyInfo line by line.

1>------ Build started: Project: UE4, Configuration: Development_Editor x64 ------
1>Creating makefile for ShaderCompileWorker (no existing makefile)
1>Creating makefile for UE4Editor (no existing makefile)
1>Creating makefile for UnrealHeaderTool (no existing makefile)
1>Parsing headers for UE4Editor
1>  Running UnrealHeaderTool UE4Editor "C:\UnrealEngine-4.22.3-release\Engine\Intermediate\Build\Win64\UE4Editor\Development\UE4Editor.uhtmanifest" -LogCmds="loginit warning, logexit warning, logdatabase error" -Unattended -WarningsAsErrors
1>C:/UnrealEngine-4.22.3-release/Engine/Plugins/UnrealImGui-UE_4.22/Source/ImGui/Private/ImGuiModuleSettings.h(21) : LogCompile: Error: An explicit Category specifier is required for any property exposed to the editor or Blueprints in an Engine module.
1>C:/UnrealEngine-4.22.3-release/Engine/Plugins/UnrealImGui-UE_4.22/Source/ImGui/Private/ImGuiModuleSettings.h(24) : LogCompile: Error: An explicit Category specifier is required for any property exposed to the editor or Blueprints in an Engine module.
1>C:/UnrealEngine-4.22.3-release/Engine/Plugins/UnrealImGui-UE_4.22/Source/ImGui/Private/ImGuiModuleSettings.h(27) : LogCompile: Error: An explicit Category specifier is required for any property exposed to the editor or Blueprints in an Engine module.
1>C:/UnrealEngine-4.22.3-release/Engine/Plugins/UnrealImGui-UE_4.22/Source/ImGui/Private/ImGuiModuleSettings.h(30) : LogCompile: Error: An explicit Category specifier is required for any property exposed to the editor or Blueprints in an Engine module.
1>C:/UnrealEngine-4.22.3-release/Engine/Plugins/UnrealImGui-UE_4.22/Source/ImGui/Private/ImGuiModuleSettings.h(33) : LogCompile: Error: An explicit Category specifier is required for any property exposed to the editor or Blueprints in an Engine module.
(- snip -)
1>Done building project "UE4.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 2 up-to-date, 0 skipped ==========

2. Order of included header files

After adding categories, UnrealHeaderTool still produces errors as follows. I guess this is related to IWYU rules of the UE4 engine, and the errors can be fixed by reordering #include.

1>------ Build started: Project: UE4, Configuration: Development_Editor x64 ------
1>Creating makefile for UE4Editor (no existing makefile)
1>Parsing headers for UE4Editor
1>  Running UnrealHeaderTool UE4Editor "C:\UnrealEngine-4.22.3-release\Engine\Intermediate\Build\Win64\UE4Editor\Development\UE4Editor.uhtmanifest" -LogCmds="loginit warning, logexit warning, logdatabase error" -Unattended -WarningsAsErrors
1>Reflection code generated for UE4Editor in 12.4912993 seconds
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Private\Editor\ImGuiEditor.cpp(1): error : Expected ImGuiEditor.h to be first header included.
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Private\Editor\ImGuiKeyInfoCustomization.cpp(1): error : Expected ImGuiKeyInfoCustomization.h to be first header included.
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Private\Utilities\DebugExecBindings.cpp(1): error : Expected DebugExecBindings.h to be first header included.
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Private\Utilities\WorldContext.cpp(1): error : Expected WorldContext.h to be first header included.
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Private\Widgets\SImGuiCanvasControl.cpp(1): error : Expected SImGuiCanvasControl.h to be first header included.
(- snip 30 lines - )
1>Done building project "UE4.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 2 up-to-date, 0 skipped ==========

3. Path of included header files

After reordering #include, the Visual Studio C++ compiler produces the following errors, which can be fixed by adding folder to #include like → <Modules/ModuleManager.h>.

1>------ Build started: Project: UE4, Configuration: Development_Editor x64 ------
1>Using Visual Studio 2017 14.16.27023 toolchain (D:\program_files\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.16.27023) and Windows 10.0.17763.0 SDK (C:\Program Files (x86)\Windows Kits\10).
1>Building 6 actions with 8 processes...
1>  [1/6] PCH.ImGui.cpp
(snip)
1>C:\UnrealEngine-4.22.3-release\Engine\Plugins\UnrealImGui-UE_4.22\Source\ImGui\Public\ImGuiModule.h(9): fatal error C1083: Cannot open include file: 'ModuleManager.h': No such file or directory
(snip)
1>Done building project "UE4.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 2 up-to-date, 0 skipped ==========
segross commented 5 years ago

Hey, Thanks for your response.

Ad 1. I wasn’t aware of that but sure, if this is causing errors then of course it needs to be changed. I guess that UHT has no way to tell that there is a property type customisation and whether it is going to be used or not :) Ad 3. Also nothing to dispute there.

The problem is only with the issue 2 as moving PCH includes causes errors in the older engine versions and warnings in the latest ones. You are right that it will be related to the IWYU that they introduced at some point. I’d like to maintain backward compatibility, but I sort of expected that sooner or later I may have issues connected with that change.

But it would be good to allow compile it as an engine plug-in, especially that it doesn’t require that much changes. I will check, maybe it is possible to get rid of pre-compiled header and mitigate that problem or if not fully possible, then use it only in the older builds.

segross commented 5 years ago

Hey, an update regarding the issue number 2. I don't specify PCH usage in the Build.cs file. I get similar errors, if I put a following line there:

PCHUsage = PCHUsageMode.UseExplicitOrSharedPCHs;

and I get it fixed back after changing that to:

PCHUsage = PCHUsageMode.UseSharedPCHs;

I suppose that because PCH usage is not specified, UBT can use whatever is the default and it may be different for the engine and for the project. Instead of reordering all those includes, I would try to put above to the build script and see if that works.

Later I will try to switch to IWYU, but for now it would be an easy and nice fix... if it worked.

One note is that PCHUsage would need to be guarded by a version check (not sure when it became available). Something like the following example taken from the Build.cs file, line 26:

#if UE_4_21_OR_LATER
    PrivatePCHHeaderFile = "Private/ImGuiPrivatePCH.h";
#endif
kkawachi commented 5 years ago

Thank you for your deep investigation on the 2nd issue! I've reverted the order of included header files and added PCHUsage to ImGui.Build.cs as you pointed out. Now it compiles and runs fine both on 4.21 and 4.22 as an engine plugin. I would appreciate it if the updated changeset could be accepted.

segross commented 5 years ago

Hi, a bit late response but I just came back from holiday. I'll try to review and merge it this week. I have guests right now, so do not worry, if nothing changes for a while. From a quick look it looks good. The only thing is, that I would use a different version for PCHUsage, but I can amend it myself.

Thank you for that change and for the effort to apply suggestions.

Does it matter for you how I merge it? Because I was thinking about 'Squash and merge'.

segross commented 5 years ago

Hey, your request is merged using 'Squash and merge'. I removed redundant commends and added notes from this discussion to keep in history why those changes were necessary. Turns out that PCHUsage is available even in UE 4.15, which I claim to support, so I move it outside of the conditional block and amended.

Thanks for your changes.

kkawachi commented 5 years ago

Hi, I'm sorry for not responding sooner. Thank you for your cleanup and merger of the PR. I've checked out the merged code and confirmed that it compiles and runs as expected. Many Thanks!