-
Notifications
You must be signed in to change notification settings - Fork 216
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
Added support for netImgui. #39
Conversation
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
Ad 1. Yes, I suppose that there will be just a different function for that. I had the same problem with folders names at some point and I think it was somewhere around 4.19. Ad 6. We'll see. I might have some questions but it should be fine. I would expect that there will be some wrapper functions + commands.
That is interesting :) There is a mechanism that is kicking in when you don't call delegates explicitly. I guess that is what is happening. I will debug that when I get a moment. Is that the editor context that I see in the remote connection? That would explain why I only saw debug from static functions. I would need to check the order though. I will check it and maybe clean. Also the proxy seems like a right place to keep all that. It was supposed to be some sort of "abstraction" on top of the proper context. But I only added what I needed at the time and if it didn't work for you it means that it need to be improved. I'll see what I can do with the branch, debug it a bot more and try to see how I would do the context. But I'm a bit loaded with lots of other stuff so please be patient with me. And here is the file with the example that you asked. Should be all that is needed but let me know if you find that something is missing: |
Oh yes, I only realised after your previous post that it may be the editor context. And I didn't notice your bar before - I somehow lost it in the previous post. So yes, it is all looking good :)
Yes, I believe that. It all makes sense after realising that this was the editor context. Sorry that I didn't see it before, but I forgot that it even existed. The editor context was just a dummy added to make sure that there is always at least one valid context and to this point, it was completely neglected in this plugin. So I now see better why it was causing you troubles - even more kudos that you decided to handle it. I'll test it again and come back here. And I will try to see how to branch from a pull request. |
The PIE works great. So to confirm, the whole misunderstanding was because I confused editor with PIE context. I'm merging that, not even sure whether it needs a separate branch. I still might create one for experiments but whatever is here is a great addition and should go to master. Thanks you for that. Do you have any preferences how I should credit you? Full name from the licence? |
👍
I guess my name and link to the netImgui depot would be nice. Glad it worked without too much fuss.
Certainly, I don't think the current integration is finalized yet. I'll be happy to continue providing support and anwser questions. You can reach me directly at my email address (see my netImgui git)
For compile settings I already exposed the 2 important things I believe (which port to use). Other than that, we have :
|
I just quickly added the 'mirror' option that's accessible in the netImgui mainmenu bar. One thing I realized is that your proxy context are not tied to particular delegate callbacks. They seem to each call all of the delegates? I am not sure I understand the purpose of having more than 1 context (in editor for example) when they are really tied to a particular Viewport either? In that case, then yes, it would make sens to create a netImgui context class that takes precedence over the regular proxy context, if there's only 1 context at a time. Would also allow to easily have dual Imgui display with distinct content (one locally and one remotely) When I implemented ImGui at my previous company, we had a context per Unreal Viewport, with a per viewport settings object also associated. This allowed us to toggle rendering flags per viewport and such. Not sure how desirable or feasible this would be for your plugin. |
Yes the plugin settings makes perfect sens to store some options, like port number.
Ok, so delegate are still tied indirectly to a certain Proxy context, because it checks the world. And you can have more than 1 active world outside the editor context. I'm not sure I fully understand when UE creates à new World, I thought the editor had 1 world and PIE had another one, so when viewing a model with model viewer, it doesn't creates a new World, just a new view, so your Imgui content is going to be the same for main editor windows and viewer window. Same for split screen. This was why we tied it per viewport, but I might be wrong on the behavior. I will leave the context switching option as is.
Ah, you can have 2 PIE going on? Didn't know about that. So, you are saying the Editor proxy context is a dummy, do you not expect user to be able to use ImGui inside the editor, normally?
I have no difficulty imagining user wanting full debug menu in the remote connection, but some basic things (like memory use, FPS, and other stats) locally. Also, Unreal Texture asset are not supported, since I need access to their data, which is not readily accessbible on the CPU.
Per viewport might complicate things a bit and not sure how easy it is to achieve through plugin. I believe you can already receive a callback when a viewport is created/destroyed? But in any case, if there hasn't been any feedback from user wanting things more viewport bound, I don't see why you should change how things are done. |
OK, I think that we might be looking at this from a bit different perspectives and in result missing each other points. I probably should update the readme file to better explain goals and design. There used to be more information there (at lest in my first prototypes) but in an attempt to reduce jibber-jabber, which I do, I might reduced it too far. So, the primary goal was to add an easy immediate debug the tool that you can call from any place in your game code (well, most of). The tool is, of course, the ImGui framework and the integration tries to make it possibly invisible for the game code how it is managed. And the rest is dealing with innards like hot-reloading, inputs, viewports attachments, etc.
I might be mistaken, but I think it does create a world. I will need to check it.
That is correct. But this is not something that cannot change and in fact is on the cards. Like before, I develop this slowly and it just didn't get enough traction.
Not exactly. In my previous office it was used as it was visible through the remote client. Here it was invisible so far but I still used it as a safety "fallback". I don't want somebody to call a debug code (accidentally or not) and get a crash because of a null GImGui.
Yes, I have no problem with that. In my last project, we used for that a custom non-ImGui overlay and ImGui for more interactive tools. We did have ImGui stats that you could turn on/off, but there was much more interactive tools for almost any system in the game. Even the remote client was bound to the same context that you could see in the game, so you could switch between the two as you saw fit or edit in one and record in the other. So I probably think about it differently, and that is why some things might be missing for you. But now when it is an option, why not? I'd like to think how to do it, though. Maybe the same delegates can be called twice with "Is Remote" flag on/off. Although I'd fear that unaware code might draw twice. Maybe a different callback for local context that are bound to remote? Although I would hope to be able to switch between the remote and a local one invisibly... meaning that I would disconnect invisibly and not see it. Or maybe I should shift the paradigm, who knows ;)
Yeah, that was the first thing that hit me after interaction. ImGui was one of the first thing I was doing after switching from CryEngine to UE so I was not aware about having multiple PIE, etc. I didn't think about any multi-context at the time so I thought that it will be as simple as creating context, dumping debug from game and make it visible...
I think I understand now better your attachment (pun intended ;) to veiwports but TBH I would thing that per-viewport might be hard or impossible from plugin. Especially, if you want to be able to just drop the code, compile and run. It is not that I looked at this much but generally you would end up modifying some core code, adding some hooks or at best implementing a custom class - and I want to avoid that requirement. The widget is attached to the viewport after on-viewport-created event. But the per world approach was added to also allow worlds without viewport (which I know was pointless to this moment). And as explained before I wanted this to be world-synced. If I restrainded myself to using delegates only it would be a different story but I wanted to allow immediate debug from world update what complicated it a bit (mainly in a way that when you allow that there is always somebody doing something weird at some point and asking what the hell). I think I understand now better your attachment (pun intended ;) to veiwports but TBH I would thing that per-viewport might be hard or impossible from plugin. Especially, if you want to be able to just drop the code, compile and run. It is not that I looked at this much but generally you would end up modifying some core code, adding some hooks or at best implementing a custom class - and I want to avoid that requirement. The widget is attached to the viewport after on-viewport-created event. But the per world approach was added to also allow worlds without viewport (what I know was pointless to this moment). And as explained before I wanted this to be world-synced. If I restrained myself to using delegates only, I think it would be a different story but I really wanted to allow immediate debug from world update and it complicated it a bit. I mentioned that before but I'm thinking about refactoring it to allow context per widget which you can use as you like. So while implementing in viewport might not be an option from the plugin, it should be always possible to attach a right widget. But even after that, I will want to keep this workflow where one of the options will automatically create a context for every game world that is playing. But I think that it should also work with editor windows as somebody is already trying that. |
I am not particularly looking at this as a editor extension, in fact, I expect it to be used mostly for in game debugging, but the multi windows Editor setup can require more handling than straight single context situation, so just trying to make sure I'm integrating netImgui to be in line with your design.
I detect no new proxy context in netImgui, when opening new Windows (model viewer, material editor, ...)
Yes, that's definitly different that previous approach have seen/done. It's nice that user can call ImGui drawing from anywhere (on the gamethread), making it easy for programmer to have their debug content inplace. I've always relied on callbacks to draw Imgui UI.
Currently, I prevent the selected proxy context from receiving the Imgui commands, and they get sent to netImgui instead. So no double drawing in a the same context. With your current multi proxy context, delegates already get called as many time as there are proxy context, so that behavior isn't changed either. Now, things could be done differently if you prefer, for example use 2 more parameters when adding a delegate, specifying if they support remote and if they do, should we still call them locally when connected. But this doesn't address the UObject::Tick drawing commands, which at the moment just happens to be sent to the last ImGui context assigned to ImGui (I think). That said, I think things can work nicely with current setup, with just the addition of letting programmer know if the delegate is drawing for remote content or not at the moment, and having dual output only possible with delegates, not UObject::Tick (or other gamethread functions).
How does that work for where things are rendered? You only support 1 context per viewport rendering, no? What happens with 2 widgets in same viewport? This doesn't affect netImgui, just curiosity on my part. |
Yes, because I suppress anything other than game worlds (with an exception for the one editor context). Like I said, by any means, it is not complete and I added what I mostly needed or found useful. That saying, there is no reason to not to add more and in fact some people are experimenting with that. There are also quite a few other things missing like blueprints integration.
Integration is fine. Undoubtedly, some more things will be possible now and some things will show up (already shows up) that might require handling.
Yes, we had some fierce discussion about that in my last studio. That saying, I would confirm that most of the time (significantly) relying on callbacks was the preferred and more convenient option. But on the other hand some things where only possible or easier during the world update and that is why I like to have it.
Yes, I think they can. And I don't think there is any reason to hide it. I would be the hypocrite insisting on the features described above and claimed that something else is not important. And I can see the benefits that you presented, so all in all it can prove a nice addition. I'm sure some approach will emerge with time but we might agree that delegates might be a way for that "lighter" debug view. I desperately want to finish a task before Friday, so not sure whether I merge it today, tomorrow or Saturday (depending how my task goes ;) but I will surely do it. It is not much work but I don't want to make it in rush. Sorry for the delay. |
Take your time, this is more or less a hobby :) In the meantime, I'll slowly implement the UE4 sockets support. |
return AddTextureEntry(Name, Texture, true); | ||
texIndex = AddTextureEntry(Name, Texture, true); | ||
#if NETIMGUI_ENABLED | ||
NetImgui::eTexFormat eFmt = SrcBpp == 8 ? NetImgui::eTexFormat::kTexFmtA8 : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8 and 32 or 1 and 4? Those are bytes per pixel, so it fails to send any texture. After changing this here, ImGuiModuleManager.cpp doesn't need following lines 111-113:
#if NETIMGUI_ENABLED
NetImgui::SendDataTexture(static_cast<uint64_t>(FontsTexureIndex), Pixels, Width, Height, NetImgui::eTexFormat::kTexFmtRGBA8);
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. I'll make de change. By the way, as can been seen in this file, I only handle raw texture, not UE texture asset (would need CPU access to their data)
if( sMirrorContent ) | ||
{ | ||
ImDrawData* pDrawData = NetImgui::GetDrawData(); | ||
// Because UpdateDrawData take ownership of the Imgui DrawData with a memory swap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The swap is not really necessary. We can remove it or conditionally inhibit, if it is causing problems here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you. By having it here, it prevents spreading the code handling netImgui in your regular implementation and it's not a big bother to handle, just had to find it :)
Added: IsRemoteDrawing() and IsRemoteConnected() to FImGuiModuleProperties Removed : Intrusive code in FProxyContext Fixed: NoUnityBuild
@@ -143,12 +144,20 @@ void FImGuiContextProxy::DrawEarlyDebug() | |||
if (bIsFrameStarted && !bIsDrawEarlyDebugCalled) | |||
{ | |||
bIsDrawEarlyDebugCalled = true; | |||
if( NetImGuiCanDrawProxy(this) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the best I could come up with to support calling delegates twice (when dual output enabled). At first, I thought I could just call all the delegates in NetImguiUpdate
but then I realized you can call these delegates from various engine callbacks. So for now, I call the delegate 2x (or only once if no dual output) in the various Draw functions of the active proxy context.
@@ -295,6 +303,23 @@ bool FImGuiTextureHandle::HasValidEntry() const | |||
return Index != INDEX_NONE && ImGuiModuleManager && ImGuiModuleManager->GetTextureManager().GetTextureName(Index) == Name; | |||
} | |||
|
|||
bool FImGuiModuleProperties::IsRemoteDrawing() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured that's where you would want this information accessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily :) But let's leave it there for now. In general, I have settings that are part of configuration and not exposed and properties that are exposed but not intended to be typically used. They contain various switches that typically have corresponding commands. When I will be adding option to hide ImGui from the game viewport it will be there. I'm pretty sure that mirroring and dual debug might find their own properties at some point (not yet, though). They are exposed for people that don't like default implementation or want own extension, viewport that would handle them directly, etc. but they are not meant for "typical" debug code. But for now this is fine. I'll need to think where I'd want that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it made for a long expression when writing FImGuiModule::Get().GetProperties().IsRemoteDrawing()
:) but saw functions in the module class were deprecated and moved to moduleproperty class. These 2 functions are not settings related, just information on current status.
@@ -231,5 +228,7 @@ void FImGuiModuleManager::AddWidgetsToActiveViewports() | |||
|
|||
void FImGuiModuleManager::OnContextProxyCreated(int32 ContextIndex, FImGuiContextProxy& ContextProxy) | |||
{ | |||
// Make sure that textures are loaded before the first Proxy Context is created. | |||
LoadTextures(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here, since I wanted netImgui to work as soon as Editor is open and connected. Previously, the first LoadTextures()
was called once PIE was launched, preventing to see results properly until then.
|
||
// Add other netImgui options here, like continue display locally | ||
ImGui::Separator(); | ||
ImGui::RadioButton("DualDisplay: None", &sDualUIType, kDualUI_None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can can choose how to name these options, change them, or available at all.
|
||
//SF Temporary code, prevents warning while we are still include Windows specific. Will be removed once using UE sockets | ||
#ifdef TEXT | ||
#undef TEXT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a havoc, at least when compiling with unity. Suddenly multiple engine and plugin files show: "error C3861: 'TEXT': identifier not found".
Not sure why but I don't actually see any warning when I remove that code (UE 4.25.1, MSVC 14.16.27023 and Windows 10.0.17763.0 SDK)... OK, I see it when compiling without unity.
So, this is best handled by wrapping relevant code in this:
#if PLATFORM_WINDOWS
#include <Windows/AllowWindowsPlatformTypes.h>
#endif // PLATFORM_WINDOWS
#if PLATFORM_WINDOWS
#include <Windows/HideWindowsPlatformTypes.h>
#endif // PLATFORM_WINDOWS
Nesting of that header is now allowed, so the best way in my opinion is to wrap all the "third party" cpps when including them in the ThirdPartyBuildNetImgui.cpp. See @ the ThirdPartyBuildImGui.cpp for example. I did actually mentioned that warning before and I was going to do just that.
Sadly, compiling with and without unity is necessary to catch all the errors and warnings because some people that do CI are quick to hit and come back with those issues.
On a related note, I think that the ThirdPartyBuildNetImgui.h used to miss CoreMinimal.h or HAL/Platform.h + Containers/Map.h, but that seem to be fixed after including other module's header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I finish implementing the UE4 sockets, I won't be including Windows specific header there, so that should solve the problem. On a side note, seems like I was compiling nounity differently, because I saw no compiling error in UE 4.25.3. I was adding bUseUnity = false;
to ImGui.Build.cs
inDevelopement Editor
configuration. Maybe because it enabled it only for the ImGui plugin. At my previous company, we were asking engine programmers to always compile with no unity (we added a build type if I remember correctly) before submitting code, to catch these real issues early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try to read my comment with a fresh eye and perhaps it was confusing.
- I only saw an error when compiling with unity (using undefined TEXT).
- The second part was about removing that undef. From your comment I figured that you had a warning but after I removed those lines I didn't see any warning. It was back after I switched to nounity.
In any case, all is good in the latest.
To switch, I use that in my target rules:
bUseUnityBuild = false; // or true
bUsePCHFiles = false; // or true
I didn't know that you can use it in Build.cs files.
At my previous company, we were asking engine programmers to always compile with no unity (we added a build type if I remember correctly) before submitting code, to catch these real issues early.
Yes, same here. We did that also and with time we got a pretty nice tools with a few try-commit options. I changed a company recently and I really miss that. It was pretty good in catching all sorts of errors at early stage as you said. Here I to things manually but I often forget something, either checking no-unity, no editor or add something that is not backward compatible.
…platform supported by Unreal should now work with remote connection.
With this latest checkin, netImgui integration is now feature complete. I am using UE4 socket code, so every platform UE ship to, will be supported. Only thing left will be fixing issue that may arise. I also also made sure the no unity build build work by disabling it on the entire engine (took 5hrs+ on my laptop :p ). It compiled fine minus some unrelated errors in GteGenerateMeshUV. The rest will be up to you to decide how to expose settings/features and shuffle things around to fit how you'd like things to work. I'll be happy to help some more and offer explanations. If you prefer, you might be able to create a new 'netImgui Integration' branch, and I could try to create a pull request to it, instead of the master branch. Up to you. |
👍 That is really nice. Great thanks again.
OMG. You compile the whole engine like this? If you want to add something later, you can always let me know and I can do it. It is fairly quick check for me and I will probably do it anyway. Still worth reminding me so I know and won't forget.
OK. I have already some ideas how I want to expose some things, so I will play with it and let you know. I might ask for opinion to make sure that it is also with a spirit of workflow that you added and that it all combines properly.
No need, turns out it was pretty easy. All I had to do was to create a new net_imgui branch and then I was able to modify this request to merge to that new branch. I'm not that familiar with GitHub but that was pretty straightforward. So this is already merged to net_imgui and soon I will merge that to master. The only downside is that I don't see you in the contributors list yet but that apparently will be fixed after merging that new branch to master. |
Well, I usually didn't but since I didn't have the issue you had, and thought it was without unity build, I did that as a last check. Didn't expect it to end up being 3x longuer than with unity. I didn't disabled PCH though, and I believe I am 1 UE version ahead.
👍 On my side, I might create a new barebone plugin to add netImgui + Dear ImGui for a very simple integration example, with no local display support. Should be a good starting point for people with their own Dear ImGui integration wanting remote connection, and with your plugin for a more in-depth demonstration. |
I can always do that. For the most of the time I have my testing project linked to a binary engine version, so when I disable unity it only affects that project. It is a small project, so it takes seconds/minutes rather than hours. I've never tried to compile it without unity with an engine that I've built from source code. As I said, it can be always raised to remind me to test it.
👍 |
I'm looking at merging that to the master but unfortunately I'm blocked by a bunch of crashes when exiting from editor. I cannot get consistently the same call-stack but one problem seemed to be with Another issue is a bit more consistent but the call-stack is cryptic to me. I might need to try engine with full source code, because right now when running DebugGame Editor I simply get this:
I'd assume this will be something with the socket. |
I am not seing any crash under UE4.25.3 while closing the editor and connected to netImgui. However, I did get a crash just now while closing the editor, and having connected/disconnected from netImgui before closing. I had this callstack, which seems to point to font release issue. I will take a look later.
|
Yes, that will be probably the first issue that I described. Moving But even if I do that or even if I do not release the manager at all to make sure that they don't try to free the same resources, I still get this crash with that call-stack above. I will test later the older revision using win socket. I used 4.25.1 but I have other engines versions to compare later. Oh, actually. If you got that crash as you posted, you may try what I did. Either moving net shutdown after the manager or not releasing the manager at all (that should be fine as it just leaks... fine for testing, I meant). That is how I noticed the second crash. |
I will take a look during the weekend and try to fix it. |
Found the problem and fixing it. Will spend the weekend to finish it and make it clean. Need to properly wait for socket to end connection before existing the module shutdown. |
Added support for netImgui. (segross#39)
Note
I renamed the filea
ImGuiImplemention.cpp/.h
toThirdPartyBuildImGui.cpp/.h
to matchThirdPartyBuildNetImgui.cpp/.h
. I think it makes it easier to figure out how things are build this way, but can be renamed to something you prefer.I added the configuration options (defines, modules/path includes) directly in the netImguiLibrary.cs, making it clear what this is about. Something similar could be done with ImGuiLibrary.cs I believe.
ToDo