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

NetImgui Update 1_3 #44

Merged
merged 17 commits into from
Feb 26, 2021
Merged

NetImgui Update 1_3 #44

merged 17 commits into from
Feb 26, 2021

Conversation

sammyfreg
Copy link

@sammyfreg sammyfreg commented Feb 14, 2021

Updating Dear Imgui to 1,80 (with table support) and NetImgui to 1.3.

Tested on Unreal 4.26

Note: There only 2 changelist for the latest change. Other changes listed are from previous changes.

sammyfreg and others added 14 commits September 6, 2020 19:46
TODO: fix issue with editor not drawing until PIE activated once
TODO: Implement UE4 sockets (instead of using Winsock directly)
TODO: Figure out how to expose options to user, and netImgui server exe
Added: IsRemoteDrawing() and IsRemoteConnected() to FImGuiModuleProperties
Removed : Intrusive code in FProxyContext
Fixed: NoUnityBuild
…platform supported by Unreal should now work with remote connection.
Update to latest UnrealImgui
Updating DearImgui and NetImgui libraries to latest version.
"CanContainContent": false,
"IsBetaVersion": false,
"Installed": false,
"Modules": [
{
"Name": "ImGui",
"Type": "Developer",
"Type": "DeveloperTool",
Copy link
Author

Choose a reason for hiding this comment

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

This was done to get rid of compiling warning that 'Developer' is deprecated. This can be left intact for older UE release if you prefer.

Copy link
Owner

Choose a reason for hiding this comment

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

It is intentionally left as "Developer" because "DeveloperTool" doesn't work with anything older than 4.24, if I remember correctly, while on the other hand right now "Developer" only gives a warning that is pretty understandable and easy to fix for those that care. I locally also use "DeveloperTool". But in this branch, we can leave "DeveloperTool".

@@ -0,0 +1,27 @@
{
Copy link
Author

Choose a reason for hiding this comment

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

Decided to pack the server with the plugin. Can be removed if you prefer.

@@ -15,13 +15,19 @@ ImGui version: 1.74

Supported engine version: 4.25*

\* *Plugin has been tested and if necessary updated to compile and work with this engine version. As long as possible I will try to maintain backward compatibility of existing features and possibly but not necessarily when adding new features. Right now it should be at least backward compatible with the engine version 4.15.*
\* *Plugin has been tested and if necessary updated to compile and work with this engine version. As long as possible I will try to maintain backward compatibility of existing features and possibly but not necessarily when adding new features. When it comes to bare-bone ImGui version it should be at least backward compatible with the engine version 4.15. For NetImgui it needs to be determined.*
Copy link
Author

Choose a reason for hiding this comment

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

Before Starting my changes, I integrated the master branch into the net_imgui branch.

@@ -11,7 +11,11 @@

#include <imgui.h>


#include "Fonts/Roboto_Medium.cpp"
Copy link
Author

Choose a reason for hiding this comment

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

This is optional. In the UnrealNetImgui plugin, I added support for the fonts shipping in Dear ImGui. Brought the same changes here.

@@ -225,8 +223,9 @@ void FImGuiContextProxy::BeginFrame(float DeltaTime)
{
if( NetImGuiCanDrawProxy(this) )
{
ImGuiIO& IO = ImGui::GetIO();
IO.DeltaTime = DeltaTime;
SetAsCurrent();
Copy link
Author

Choose a reason for hiding this comment

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

DrawDebug (on line 200) can leave the ImGui context set to the one created for NetImgui making sure we are using the proper context of this proxy.

@@ -59,5 +59,5 @@ void FImGuiDrawList::TransferDrawData(ImDrawList& Src)

// ImGui seems to clear draw lists in every frame, but since source list can contain pointers to buffers that
// we just swapped, it is better to clear explicitly here.
Src.Clear();
Src._ResetForNewFrame();
Copy link
Author

Choose a reason for hiding this comment

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

API change to latest Dear ImGui

@@ -241,6 +241,23 @@ void FImGuiModule::ToggleShowDemo()
}
}

bool FImGuiModule::IsRemoteDrawing() const
Copy link
Author

Choose a reason for hiding this comment

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

Thought it made more sens to have directly under module instead of properties

@@ -24,10 +44,14 @@ void NetImguiPreUpdate_Connection()
// Could also support reaching server directly if we had a provided IP
if (ImGui::GetCurrentContext() && !NetImgui::IsConnected() && !NetImgui::IsConnectionPending())
{
// Using a separate Imgui Context for NetImgui, so ContextProxy can be easily swapped to any active (PIE, Editor, Game, ...)
spNetImguiContext = ImGui::CreateContext(ImGui::GetIO().Fonts);
Copy link
Author

Choose a reason for hiding this comment

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

NetImgui doesn't handle creating its own Imgui context internally anymore, must create one for it if we don't want it to rely on the current active context (because UnrealImgui handle mutliple contexts)

@@ -12,6 +12,19 @@
class FImGuiModule : public IModuleInterface
{
public:
enum class eFont
Copy link
Author

Choose a reason for hiding this comment

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

Optional, part of exposing the new loaded fonts.

/**
// Helper class that change the font and automatically restore it when object is out of scope
*/
struct ImGuiScopedFont
Copy link
Author

Choose a reason for hiding this comment

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

Usefull struct to auto push/pop the font in code.
Example :

void ShowText()
{
    {
        ImGuiScopedFont scopedFont(FImGuiModule::eFont::kProggyClean); 
        ImGui::Text("Lorem ipsum dolor sit");
    }
    {
        ImGuiScopedFont scopedFont(FImGuiModule::eFont::kDroidSans); 
        ImGui::Text(" amet, consectetur adipiscing elit");
    }
}

@sammyfreg sammyfreg changed the title Update 1 3 Update 1_3 Feb 14, 2021
@sammyfreg sammyfreg changed the title Update 1_3 NetImgui Update 1_3 Feb 14, 2021
@Frigerius
Copy link

Frigerius commented Feb 16, 2021

It seems this change introduces a new crash when cooking:
Need to note I upgraded to 1.81 hope this doesn't contain this crashing code ^^'
Looking at the code, Shutdown seems to get called with a nullptr context
I found a fix, will comment the part in the review


  LogWindows: Error: === Critical error: ===
  LogWindows: Error:
  LogWindows: Error: Fatal error!
  LogWindows: Error:
  LogWindows: Error: Unhandled Exception: EXCEPTION_ACCESS_VIOLATION reading address 0x00000000000000a8
  LogWindows: Error:
  LogWindows: Error: [Callstack] 0x00007ff92af0b36b UE4Editor-ImGui.dll!ImGui::Shutdown() [U:\Projects\Yggdrasil\Plugins\UnrealImGui\Source\ThirdParty\ImGuiLibrary\Private\imgui.cpp:4099]
  LogWindows: Error: [Callstack] 0x00007ff92af0bece UE4Editor-ImGui.dll!FImGuiModule::ShutdownModule() [U:\Projects\Yggdrasil\Plugins\UnrealImGui\Source\ImGui\Private\ImGuiModule.cpp:137]
  LogWindows: Error: [Callstack] 0x00007ff946989b45 UE4Editor-Core.dll!FModuleManager::UnloadModulesAtShutdown() [U:\UnrealEngine\Engine\Source\Runtime\Core\Private\Modules\ModuleManager.cpp:731]
  LogWindows: Error: [Callstack] 0x00007ff78fdc00c7 UE4Editor-Cmd.exe!FEngineLoop::Exit() [U:\UnrealEngine\Engine\Source\Runtime\Launch\Private\LaunchEngineLoop.cpp:4216]
  LogWindows: Error: [Callstack] 0x00007ff78fdc0bf7 UE4Editor-Cmd.exe!GuardedMain() [U:\UnrealEngine\Engine\Source\Runtime\Launch\Private\Launch.cpp:182]
  LogWindows: Error: [Callstack] 0x00007ff78fdc0c4a UE4Editor-Cmd.exe!GuardedMainWrapper() [U:\UnrealEngine\Engine\Source\Runtime\Launch\Private\Windows\LaunchWindows.cpp:137]
  LogWindows: Error: [Callstack] 0x00007ff78fdd4e1d UE4Editor-Cmd.exe!WinMain() [U:\UnrealEngine\Engine\Source\Runtime\Launch\Private\Windows\LaunchWindows.cpp:268]
  LogWindows: Error: [Callstack] 0x00007ff78fdd6eb2 UE4Editor-Cmd.exe!__scrt_common_main_seh() [d:\agent\_work\63\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288]
  LogWindows: Error: [Callstack] 0x00007ff9b5087034 KERNEL32.DLL!UnknownFunction []
  LogWindows: Error: [Callstack] 0x00007ff9b6b7d241 ntdll.dll!UnknownFunction []

{
#if NETIMGUI_ENABLED
NetImgui::Shutdown(true);
ImGui::DestroyContext(spNetImguiContext);

Choose a reason for hiding this comment

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

You should check here, if the context you want to release is != nullptr, since otherwise we will crash, or you might release GImGui, so the current context, which also might lead to other side effects.

Copy link
Author

Choose a reason for hiding this comment

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

I did not see the crash while cooking (launched demo game from editor with cooked on the fly off) and probably just didn't have the right condition for the problem. Adding a null check makes sens, so it is now there.

@@ -46,6 +46,7 @@ static FImGuiContextHandle ImGuiContextPtrHandle(ImGuiContextPtr);
#include "imgui_demo.cpp"
#include "imgui_draw.cpp"
#include "imgui_widgets.cpp"
#include "imgui_Tables.cpp"

Choose a reason for hiding this comment

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

clang is case sensitive, so has to be imgui_tables.cpp

Choose a reason for hiding this comment

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

Since I can't mark it, can you also fix the include for #include "ThirdPartyBuildImgui.h" and change it to #include "ThirdPartyBuildImGui.h"

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for spotting this. I compile NetImgui against VSToolchain and Clang but haven't tried to figure out setting up UE4 build against clang, so plugin is only tested against VSToolchain for now,

@segross
Copy link
Owner

segross commented Feb 23, 2021

I'll just merge it without checking as I don't have right now any time. But I've seen a discussion above and I believe it works, so all good.

@segross segross merged commit 297e09d into segross:net_imgui Feb 26, 2021
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.

None yet

3 participants