segross / UnrealImGui

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

Added support for netImgui. #39

Closed sammyfreg closed 4 years ago

sammyfreg commented 4 years ago

Note I renamed the filea ImGuiImplemention.cpp/.h to ThirdPartyBuildImGui.cpp/.h to match ThirdPartyBuildNetImgui.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

segross commented 4 years ago

Ok, so first nuances that will be easy to fix or modify.

  1. A minor backward compatibility issue with the FApp::GetProjectName(). It is only below 4.19, so not event sure if it is worth fixing. And I can do this (if necessary) as I have an older engine version.
  2. I'll check that later again but I'm pretty sure that there is a non-unity compile error.
  3. "ThirdPartyBuild..." names. TBH I don't like them - Sorry about that :) I looked at the files before looking at your comment and somehow linked Build with Build.cs, which confused me at first. But I agree with the motive, so maybe something needs to be done. I used the "implementation" for two reasons: because it is housing an implementation code and because it provides an interface to that implementation for the remaining part of the module (needed for instance for hot-reloading). Maybe it makes sense to move both files to a separate ThirdParty directory and name them ImGuiSomething and NetImGuiSomething. Now the "something" would be? ;) I'd say that "something" == "implementation" (because of the above) but I understand you disagree ;)
  4. About moving all options to different Build.cs files. I didn't use ImGuiLibrary.Build.cs in my first tests and only added that later to be able to see it in Visual Studio. I treat that library as a repository with the original code hidden behind the ImGui module. And since the ImGui is the only one exposed for users, I prefer to have configs there. But some configs, especially those not needed for the users can be in other places, so that is fine. Although maybe at some point, I would be tempted to move "enabled" property to the ImGui config as you could then use it to also make sure that the external module is only on the dependency list when used.
  5. Exposing settings should be easy. I can handle that, but I might have some questions. There already is a settings panel and since the ImGui module is the one that is built and loaded, it should also handle options.
  6. I see the comment about adding the command. Yeah, that would be something to think about. That and a function in the module interface.

Now to issues that I think are more problematic. I'm thinking about creating a special merge branch to be able to investigate them and help more with the innards of this plugin.

I see the connection and that works, but the results are different: Comparison

I might be missing something but it looks to me that the remote code calls only multi-context debug events. This means that any code registered in the world delegates and code form the update functions will not work. The world delegates can be fixed, but I'm less sure about the tick functions. Additionally, the multi-context events are called twice, so debug that relies on one call per frame can break. Wouldn't it be possible and make to only use one or the other? So, if PIE 1 is bound to a remote context then only update that and mirror the results. There probably would be some issues with that approach but at least the whole debug would be handled and no debug would be called twice. Implementation-wise, I think that the proxy will be a better place for that integration. The proxy could track whether it is bound to remote or local context and depending on that update correct one.

I use git but I'm still not that familiar with GitHub and its merging. I usually did squash and merge as it is simple. I will check whether I can find some more advanced options that would allow me to make a branch from this request and go from that.

sammyfreg commented 4 years ago
  1. A minor backward compatibility issue with the FApp::GetProjectName(). It is only below 4.19, so not event sure if it is worth fixing. And I can do this (if necessary) as I have an older engine version.

This is used for the name displayed remotely, you could just assign "UE4" for older engine version, or find another meaningful name.

  1. "ThirdPartyBuild..." names. TBH I don't like them - Sorry about that :) ...

No problem, it's your plugin, feel free to change anything to what fit your vision better. Could be a 'Private/ThirdParty' folder, which then keeps the same filenames ImGuiImplementation.cpp and add NetImguiImplementation.cpp.

  1. I see the comment about adding the command. Yeah, that would be something to think about. That and a function in the module interface.

Also, like I mentionned in the code, user being able to tell if they are local or remote drawing is really usefull, to modify content based on that. Not sure if you'd prefer through a module function, exposing netImgui api or some delegate parameter. I'll leave that decision to you.

Now to issues that I think are more problematic. I'm thinking about creating a special merge branch to be able to investigate them and help more with the innards of this plugin.

Makes sens, I realized I should also have done that, since as soon as I checkin code, it gets added to Pull request at the moment.

I see the connection and that works, but the results are different: Comparison

I notice that I have a disconnect UI issue I'll have to look at the netImgui server code. You can see it on your screenshot to, with way too many tabs.

I might be missing something but it looks to me that the remote code calls only multi-context debug events. This means that any code registered in the world delegates and code form the update functions will not work. The world delegates can be fixed, but I'm less sure about the tick functions. Additionally, the multi-context events are called twice, so debug that relies on one call per frame can break. Wouldn't it be possible and make to only use one or the other? So, if PIE 1 is bound to a remote context then only update that and mirror the results. There probably would be some issues with that approach but at least the whole debug would be handled and no debug would be called twice. Implementation-wise, I think that the proxy will be a better place for that integration. The proxy could track whether it is bound to remote or local context and depending on that update correct one.

Mmm, I did not add any call to delegates, the behavior should be fairly the same (like you ask, I either draw in proxy context or netImgui context, not both). All I did was add a NewFrame/EndFrame of the netImgui::Context into the ContextManager update function (so they are always called, my first attempt was in the proxy, but they don't get called when the world is inactive, so the netImgui::context was not updated). And then proxy assign the remote context if it detects it should be remote drawing. I did see different content when testing, by that was one was showing the 'Editor' context context, and the other one was showing the 'PIE' context content. You can see that there are 2 actives contexts (outside my netImgui Context) when clicking on the netImgui menu entry. Would you mind sharing your test delegate setup so I can test it too?

segross commented 4 years ago

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.

Mmm, I did not add any call to delegates,

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: ImGuiDebugOrderTest.zip

sammyfreg commented 4 years ago

Here's the Game running This is the game running stand alone (no editor) and without a connection to netImgui Server ImGuiPlugin_Game

Here's with netImgui Server This is the game running stand alone (no editor) with a connection to netImgui server. The game screen display only the game. ImGuiPlugin_Server

I believe the differences you are seeing, are between the 'Editor' proxy context and the 'PIE' proxy context and these were there before netImgui. You can toggle between the 2 (in MainMenu bar displayed in netImgui) and you will see they swap ImGui content. Perhaps before, they were both outputed to the same viewport when using FImGuiContextProxy::UpdateDrawData, and only displayed the last rendered (PIE).

Wouldn't it be possible and make to only use one or the other? So, if PIE 1 is bound to a remote context then only update that and mirror the results.

As mentionned in previous post, I did not affect anything regarding how you allow user to draw, nor added delegate calls, nor added any new proxy context. My changes are limited to :

  1. In FImGuiContextProxy::BeginFrame/EndFrame, skip calling the related ImGui functions, if proxy context tied to remote
  2. In FImGuiContextProxy::SetAsCurrent/IsCurrentContext, modified to use NetImgui context if proxy tied to remote
  3. In FImGuiContextManager::Tick, start by updating the remote Context (connection/EndFrame/StartFrame/Draw Special Menu)

So, with/without netImgui, you are updating 2 proxy context and I'm always picking the first context by default (user can switch them in the mainMenu bar displayed in netImgui). Maybe you want to only activate the PIE proxy context when it's active, and netImgui should also bind itself to it. If the user want to mirror the netImgui content in their game windows, we can easily do that (with caveat that screensize won't match and not input, like my dual output mirror demo).

segross commented 4 years ago

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 :)

As mentionned in previous post, I did not affect anything regarding how you allow user to draw, nor added delegate calls, nor added any new proxy context.

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. But it, of course, makes sense to send it. In the past, I was doing a similar thing but additionally hiding it after pressing the play button. I got used to that and probably also why I assumed that I see the PIE. Later I might add a similar option. I will need to review the whole editor context and make sure that it behaves correctly. There is something wrong with the order of events but calling only multi-context ones seems fine in this case, although it should be possible to change if necessary. And as you mentioned, the order issue will be nothing new, I expect that it will be in the proxy or manager (not many more places to look at ;).

I'll test it again and come back here. And I will try to see how to branch from a pull request.

segross commented 4 years ago

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? Also, will you mind if I have some questions later on? I'd like to know what would you like to see exposed, what in the settings, etc. And on the other hand I might have a question about mirroring, etc. But first I will play with this, update docs and check those issues with the editor context that showed up.

sammyfreg commented 4 years ago

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.

Also, will you mind if I have some questions later on?

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)

I'd like to know what would you like to see exposed, what in the settings, etc. And on the other hand I might have a question about mirroring, etc.

For compile settings I already exposed the 2 important things I believe (which port to use). Other than that, we have :

  1. I need to actually code the UE4 socket code :) right now it's only winsock, so won't work on other platforms.
  2. Mirroring option. I can quickly add that for you, and you decide if you want to keep it or not.
  3. Dual Context (allow different ImGui locally and remotely). This would be a more 'advanced' feature user might want, but do-able without too much problem.
  4. Way for programmer to know if drawing remote or not. Right now, people can call NetImgui::IsDrawingRemote() to know the current status and decide to change ImGui content at runtime. How to expose this to programmer.
  5. Where/how to distribute the netImgui server. I was also thinking of having a config file with the default local game/editor connection client configured. Will need to let user know about adding console too.
  6. For commands user might need, there's no much I believe, maybe manually initiating the connection, but auto wait for connect on start seems fine too.
  7. I select the proxy context 0 by default. In editor, should we associate PIE by default? Is there any point to expose proxy context switching in the UI?
sammyfreg commented 4 years ago

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.

segross commented 4 years ago

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).

Yes, that is great. I can also add them to the settings panel here, unless you think that something doesn't make sense to be exposed from some reaspon: Settings

I was thinking to make a separate category and add properties like port numbers, whether to auto connect on play, whether to switch to PIE on play, maybe whether to mirror, etc.

Ad 7. I wouldn't worry about context switching at this moment.

On a different note, there is a slight delay as I want to fix that non-unity error right after I take the build. This is only a missing core header and some win wrappers, so I already did it, just need to prepare for quick update after the merge. But I should do that later today. (EDIT: It is already later and I just finished my work, so I'll see how that goes today.)

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?

Yes and no. Not sure if I fully understood the point. If I recall correctly, delegates used to be tied more closely to the proxy or at least module. But this was causing issues in code that was trying to access them before loading the module and during hot reloading. To free other code form caring about that I moved delegates to a separate container. But in a result, I have to find a right delegate for the world, which is not a huge deal. I realise that there is a handful of delegates, but there is a reason behind:

  1. In my previous company we agreed that depending on a task, static or object delegates might be more useful. So what the code does is to set the right context for a given world and then call delegates. Static (multi-context) delegates call the same code for every world, so if you have a menu bar that you would like to add to every context, world stats etc. you can use that. The world delegates are not the same, those are per-world delegates that are called only for that given the world to allow its object add data.
  2. I wanted to be able to add headers and footers so I doubled number of multi-context delegates and for symmetry also world delegates. In result four delegates should be called for every world and in particular order: setting the world context -> multi-context (static) early delegates -> world early delegates -> world update -> regular world delegates -> regular multi-context delegates... going to the next world context (if this is PIE)

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?

Again not sure if I get it. The editor context was a dummy. But if you start a session with two or more PIE, then each world gets its own context. So in every viewport you will see the debug code for that world.

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.

But isn't there usually one viewport per game world? The context of dedicated server was indeed never visible. I expected to integrate the Remote ImGui at some point, but I never did. In my previous company we used to have one context for each world + one for editor and one remote client that was binding to all those context (there was no separate context for the remote client). The editor and dedicated server context were the only ones that needed remote client, while other contexts could be visible through their viewports or remote client. There was a flag to show or hide debug in the game but not per viewport. I don't see a problem to have that flag back (now with the remote it will be almost necessary). I also don't see a problem to make it per viewport (or at least an option for that). Do you think it is useful to have it per viewport?

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)

I myself don't particularly need separate contexts or dual debug. But surely it can be an option. Do you use that? I was actually thinking to keep one one context per world and in case of remote connection either reuse it or replace it. But I need to play with it a bit before discussing it further. But of course it is interesting to see how other people use it and what can be done. And one of the reasons why I'm keeping it away from viewport is that it is easier to plug and play which. In my previous company I had it more linked to the viewport (although I also had a widget) but here I was trying to keep pit away from it, so it can work without any extra steps.

sammyfreg commented 4 years ago

For compile settings I already exposed the 2 important things I believe (which port to use).

Yes, that is great. I can also add them to the settings panel here, unless you think that something doesn't make sense to be exposed from some reaspon:

Yes the plugin settings makes perfect sens to store some options, like port number.

Again not sure if I get it. The editor context was a dummy. But if you start a session with two or more PIE, then each world gets its own context. So in every viewport you will see the debug code for that world.

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.

Again not sure if I get it. The editor context was a dummy. But if you start a session with two or more PIE, then each world gets its own context. So in every viewport you will see the debug code for that world.

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 myself don't particularly need separate contexts or dual debug. But surely it can be an option. Do you use that?

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.

And one of the reasons why I'm keeping it away from viewport is that it is easier to plug and play which.

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.

segross commented 4 years ago

Ok, so delegate are...

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 feel that you look at this more like an editor extension, which is very fine and might need to be addressed. It just hasn't got enough traction so far as the focus was to give each game world a debug context to draw. More about that later.

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.

I might be mistaken, but I think it does create a world. I will need to check it.

so your Imgui content is going to be the same for main editor windows and viewer window

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.

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?

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. Now it looks like a good time to look at it.

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.

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 ;)

Ah, you can have 2 PIE going on? Didn't know about that.

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...

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.

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.

sammyfreg commented 4 years ago

I feel that you look at this more like an editor extension, which is very fine and might need to be addressed.

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 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.

I might be mistaken, but I think it does create a world. I will need to check it.

I detect no new proxy context in netImgui, when opening new Windows (model viewer, material editor, ...)

... but I wanted to allow immediate debug from world update what complicated it a bit

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.

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 ;)

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).

I mentioned that before but I'm thinking about refactoring it to allow context per widget which you can use as you like.

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.

segross commented 4 years ago

... more handling than straight single context situation...

I detect no new proxy context in netImgui, when opening new Windows (model viewer, material editor, ...)

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.

so just trying to make sure I'm integrating netImgui to be in line with your design

Integration is fine. Undoubtedly, some more things will be possible now and some things will show up (already shows up) that might require handling.

I've always relied on callbacks to draw Imgui UI.

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.

... 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).

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.

sammyfreg commented 4 years ago

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.

sammyfreg commented 4 years ago

Added support of Dual Rendering and fixed NoUnity build warning. There was also a bit of refactoring to be less intrusive in the Proxy context class, and code in ThirdPartyBuildNetImgui.cpp has been split in multiple functions for clarity. U9RTe8f2WF

Programmer now can know know if current Dear ImGui drawing is for local or remote destination. In the Screenshot, it can be seen at the end of each delegate's section (with different text for game and netImgui server).

It was done adding these lines in the ImguiActor : (The check IsRemoteConnected() only prevents displaying text when not connected, IsRemoteDrawing() works just fine without it)

if( FImGuiModule::Get().GetProperties().IsRemoteConnected() )
    ImGui::TextUnformatted(FImGuiModule::Get().GetProperties().IsRemoteDrawing() ? "Drawing: Remote" : "Drawing: Local");

It will be up to you to decide how to configure/expose these various options (dual display, remoteconnected, remotedrawing, establish connection). I did a base implementation that I think is satisfactory, but feel free to change anything to your own prefererences.

sammyfreg commented 4 years ago

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.

segross commented 4 years ago

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.

👍 That is really nice. Great thanks again.

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.

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.

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.

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.

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.

No need, turns out it was pretty easy. All I had to do was to create a new _netimgui 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 _netimgui 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.

sammyfreg commented 4 years ago

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.

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.

No need, turns out it was pretty easy. All I had to do was to create a new net_imgui branch ...

👍

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.

segross commented 4 years ago

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.

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.

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.

👍

segross commented 3 years ago

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 NetImgui::Shutdown(); and proxy stepping on each other toe when releasing resources. It doesn't happen when I move NetImgui::Shutdown(); after destruction of the manager.

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:

>   ntdll.dll!00007fff103191b2()    Unknown
    ntdll.dll!00007fff103215e2()    Unknown
    ntdll.dll!00007fff103218ea()    Unknown
    ntdll.dll!00007fff1032a8a9()    Unknown
    ntdll.dll!00007fff102626ee()    Unknown
    ntdll.dll!00007fff10260790()    Unknown
    ntdll.dll!00007fff1025fb91()    Unknown
    ntdll.dll!00007fff1026a7e7()    Unknown
    ntdll.dll!00007fff1026a76e()    Unknown
    ntdll.dll!00007fff102c62a9()    Unknown
    ntdll.dll!00007fff1025fb91()    Unknown
    ucrtbase.dll!00007fff0d5214cb() Unknown
    UE4Editor-SSL.dll!00007ffeb0d2b099()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d01fdf()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d01d74()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d02104()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d01ec0()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d02104()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d01ec0()    Unknown
    UE4Editor-SSL.dll!00007ffeb0d01ccf()    Unknown
    UE4Editor-SSL.dll!00007ffeb0c5e580()    Unknown
    UE4Editor-SSL.dll!00007ffeb0c5fa23()    Unknown
    UE4Editor-Core.dll!00007ffec5e054d5()   Unknown
    UE4Editor-Win64-DebugGame.exe!00007ff714d7af37()    Unknown
    UE4Editor-Win64-DebugGame.exe!00007ff714d7bb07()    Unknown
    UE4Editor-Win64-DebugGame.exe!00007ff714d8e3a0()    Unknown
    UE4Editor-Win64-DebugGame.exe!00007ff714d9146a()    Unknown
    kernel32.dll!00007fff0f577bd4() Unknown
    ntdll.dll!00007fff1028ce51()    Unknown

I'd assume this will be something with the socket. Do you get the same crashes when exiting editor?

sammyfreg commented 3 years ago

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.

>   [Inline Frame] UE4Editor-ImGui.dll!ImGui::MemFree(void * ptr) Line 3055 C++
    UE4Editor-ImGui.dll!ImFontAtlas::ClearTexData() Line 1543   C++
    [Inline Frame] UE4Editor-ImGui.dll!ImFontAtlas::Clear() Line 1556   C++
    UE4Editor-ImGui.dll!ImFontAtlas::~ImFontAtlas() Line 1508   C++
    UE4Editor-ImGui.dll!FImGuiContextManager::~FImGuiContextManager() Line 84   C++
    UE4Editor-ImGui.dll!FImGuiModuleManager::~FImGuiModuleManager() Line 75 C++
    UE4Editor-ImGui.dll!FImGuiModule::ShutdownModule() Line 140 C++
    [Inline Frame] UE4Editor-Core.dll!FModuleManager::UnloadModule(const FName) Line 582    C++
    UE4Editor-Core.dll!FModuleManager::UnloadModulesAtShutdown() Line 712   C++
segross commented 3 years ago

Yes, that will be probably the first issue that I described. Moving NetImgui::Shutdown(); after the manager dtor seems to prevent net and proxy stomping on each other and fixes the issue - I need to confirm that but it seems to be the case. Anyway, that should be hopefully an easy to fix.

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.

sammyfreg commented 3 years ago

I will take a look during the weekend and try to fix it.

sammyfreg commented 3 years ago

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.