UE4SS-RE / RE-UE4SS

Injectable LUA scripting system, SDK generator, live property editor and other dumping utilities for UE4/5 games
http://docs.ue4ss.com/
MIT License
1.38k stars 188 forks source link

Update ImGui ini location; make os window position and size persistent across runs #541

Closed narknon closed 6 months ago

narknon commented 6 months ago

Add imgui userdata for backend window intended to scale into use for injected window as well

Needs to be changed to save less often. Either on exit or if moved and every 5 seconds or similar prior to merging.

UE4SS commented 6 months ago

You should use the static getter UE4SSProgram::get_program().get_debugging_ui() instead of making non-static member variables of DebuggingGUI static. If I'm misunderstanding why you're changing variables to static, my apologies, and could you explain ?

UE4SS commented 6 months ago

When using UE4SSProgram::get_program().get_debugging_ui(), it's fine to change the visibility of members (but it's preferred to create public getter/setters in DebuggingGUI) if the members are private and you're trying to access them outside the class.

narknon commented 6 months ago

Thank you! I was a bit overwhelmed by the code there and just trying to get something working before going back and figuring out if I accessed stuff correctly, so I was hoping someone might take a look while it was still a draft. I'll swap

Buckminsterfullerene02 commented 6 months ago

Works great for me, awesome work :)

I wonder if it would be difficult to also save the position of the depth of the property details window? This part image

Wouldn't be bothered if too annoying but would be an extra teeny boost of QoL in this

Buckminsterfullerene02 commented 6 months ago

Actually, there is a bug.

How to reproduce:

  1. Make sure there is no imgui.ini already in working directory
  2. When the game/ue4ss is initialising, you can do one of the following: a. Close the GUI window (then closing the game any time after initialising) b. Close the game
  3. See that a garbage file is created in the root directory, and no imgui.ini file is made in the working directory

image

narknon commented 6 months ago

My best guess is this is related to how or when ue4ssprogram sets the working directory?

narknon commented 6 months ago

My best guess is this is related to how or when ue4ssprogram sets the working directory?

@UE4SS thoughts on this? Can we setup paths earlier or some other solution?

UE4SS commented 6 months ago

Actually, there is a bug.

How to reproduce:

1. Make sure there is no imgui.ini already in working directory

2. When the game/ue4ss is initialising, you can do one of the following:
   a. Close the GUI window (then closing the game any time after initialising)
   b. Close the game

3. See that a garbage file is created in the root directory, and no imgui.ini file is made in the working directory

I'm unable to reproduce the problem. It shouldn't be possible for the path to not be set by the time the GUI tries to create imgui.ini because the path is set long before any the GUI thread starts. When this happens, does any of the paths in UE4SS.log look strange ?

I guess you can try making a variable instead of altering a temporary in the params. Meaning this:

auto ini_file = to_string(StringType{UE4SSProgram::get_program().get_working_directory()} + STR("\\imgui.ini"));
ImGui::SaveIniSettingsToDisk(ini_file.c_str());

instead of this:

ImGui::SaveIniSettingsToDisk(to_string(StringType{UE4SSProgram::get_program().get_working_directory()} + STR("\\imgui.ini")).c_str());

Remember that this path is created in three different places so you'll have to try this at all of those places. This probably shouldn't be how we do it, we should set the path in one place and just refer to that after.

narknon commented 6 months ago

Its happened to me at times imgui shouldn't even be writing, which makes me wonder if it's unrelated to this.

Buckminsterfullerene02 commented 6 months ago

When I load the game with the debugger attached I instantly get an exception on this line, something about the handler being invalid or null, but not familiar enough with cpp to figure out what's going on here. m_os_backend->create_window(settings.Pos_X, settings.Pos_Y, settings.Size_X, settings.Size_Y); image

This is what happens if I don't have debugger attached: image

I have no idea why this is happening now but not yesterday. But I verified that I don't get this issue on main, so it is definately this branch. *Also tried merging main into the branch since its based on a slightly older commit, but same outcome.

Edit 42: Now I'm confused, I just changed two lines in CrashDumper.cpp to fix its dump location to working directory, and switched to this branch to test it as I can always get crash dumps generated from this issue but... wtf, I can't replicate now... how... I force rebuilt everything several times and had deleted all xmake cache between this point (due to updating MSVC version and having an issue with that, which turned out to not be anything to do with xmake anyway), so it's not like I was on some old/broken dll versions... all my logs had the same commit hash at the to (the one for the commit on this branch) p, so it was built on the same commit every time.

Buckminsterfullerene02 commented 6 months ago

Actually, there is a bug. How to reproduce:

1. Make sure there is no imgui.ini already in working directory

2. When the game/ue4ss is initialising, you can do one of the following:
   a. Close the GUI window (then closing the game any time after initialising)
   b. Close the game

3. See that a garbage file is created in the root directory, and no imgui.ini file is made in the working directory

I'm unable to reproduce the problem. It shouldn't be possible for the path to not be set by the time the GUI tries to create imgui.ini because the path is set long before any the GUI thread starts. When this happens, does any of the paths in UE4SS.log look strange ?

I guess you can try making a variable instead of altering a temporary in the params. Meaning this:

auto ini_file = to_string(StringType{UE4SSProgram::get_program().get_working_directory()} + STR("\\imgui.ini"));
ImGui::SaveIniSettingsToDisk(ini_file.c_str());

instead of this:

ImGui::SaveIniSettingsToDisk(to_string(StringType{UE4SSProgram::get_program().get_working_directory()} + STR("\\imgui.ini")).c_str());

Remember that this path is created in three different places so you'll have to try this at all of those places. This probably shouldn't be how we do it, we should set the path in one place and just refer to that after.

Not sure what the third place is, I only found it twice.

Anyway I looked at the value of ini_file at any point it's saved and I never saw it with a weird path. however, when I am debugging the code, and close the GUI window during initialisation, the ini file is for some reason created both in the root directory and the working directory. This doesn't happen when not debugging. So my thinking is, maybe the code is having some thread based issue where it's saving the file to the disk and before it has time to actually fetch the right file path from the UE4SSProgram. I think that perhaps it defaults to the current directory (the root directory) and the garbage filename is just whatever it finds in that memory address at the time.

Also, in the debugger, this breakpoint does get hit, and the path looks fine. But for some reason this log is never shown in the console, but the imgui.ini file is still created in both root and working directories. Edit: the log does show in the console if I let the game load fully without closing the GUI window before initialisation is over. image

This log does show however, and it shows the correct path in the ue4ss log. image

UE4SS commented 6 months ago

@Buckminsterfullerene02 There's one in the screenshot you showed, and then there are two in DebuggingGUI::setup. Search for imgui.ini, you'll find them all. Maybe we're not waiting for the GUI thread to stop before we destruct UE4SSProgram ? That could cause this problem. In the UE4SSProgram destructor, we're telling events to stop processing and then we exit immediately without waiting. Can you try adding a call to stop_render_thread() in ~UE4SSProgram() ? That might fix the problem.

Buckminsterfullerene02 commented 6 months ago

@Buckminsterfullerene02 There's one in the screenshot you showed, and then there are two in DebuggingGUI::setup. Search for imgui.ini, you'll find them all. Maybe we're not waiting for the GUI thread to stop before we destruct UE4SSProgram ? That could cause this problem. In the UE4SSProgram destructor, we're telling events to stop processing and then we exit immediately without waiting. Can you try adding a call to stop_render_thread() in ~UE4SSProgram() ? That might fix the problem.

Hmm, still happens. Also, the third thing you're talking about is the load settings from file, but not sure this is relevant?

UE4SS commented 6 months ago

@Buckminsterfullerene02 There's one in the screenshot you showed, and then there are two in DebuggingGUI::setup. Search for imgui.ini, you'll find them all. Maybe we're not waiting for the GUI thread to stop before we destruct UE4SSProgram ? That could cause this problem. In the UE4SSProgram destructor, we're telling events to stop processing and then we exit immediately without waiting. Can you try adding a call to stop_render_thread() in ~UE4SSProgram() ? That might fix the problem.

Then I have no idea what the problem is. Why do we even have to set the ini location three times ? Why isn't one time in the setup function enough ?

UE4SS commented 6 months ago

@Buckminsterfullerene02 Try adding a new variable to DebuggingGUI and store the full path to imgui.ini in there and set it in the setup function and use that variable instead of constantly querying UE4SSProgram.

Buckminsterfullerene02 commented 6 months ago

@Buckminsterfullerene02 There's one in the screenshot you showed, and then there are two in DebuggingGUI::setup. Search for imgui.ini, you'll find them all. Maybe we're not waiting for the GUI thread to stop before we destruct UE4SSProgram ? That could cause this problem. In the UE4SSProgram destructor, we're telling events to stop processing and then we exit immediately without waiting. Can you try adding a call to stop_render_thread() in ~UE4SSProgram() ? That might fix the problem.

Then I have no idea what the problem is. Why do we even have to set the ini location three times ? Why isn't one time in the setup function enough ?

As I say, there is only two times it is saved. The "second one in setup()" is loading the file from the location.

Anyway, I removed the save file to disk in setup() but kept the one in the on_update() and the imgui.ini file only appeared in the root directory. If I remove the one in on_update() but keep the one in setup(), the imgui.ini file appears only in the working directory sometimes and in both directories other times. This likely reinforces the idea that there is some kind of thread problem.

@Buckminsterfullerene02 Try adding a new variable to DebuggingGUI and store the full path to imgui.ini in there and set it in the setup function and use that variable instead of constantly querying UE4SSProgram.

I tried this too (absolute path hardcoded into the variable) and the imgui.ini files still get saved to both directories.

UE4SS commented 6 months ago

This likely reinforces the idea that there is some kind of thread problem.

I don't see how that's possible. If we're waiting in UE4SSProgram for the GUI to stop when you close the game, there should be no race condition. If the path is stored locally instead of querying UE4SSProgram, this also removes any possible race condition after the initial setup. The only way you're hitting a race condition then is if you time it perfectly so that you hit 'X' to close at exactly the right time during init, and at that point how are you even consistently reproducing this problem ?

UE4SS commented 6 months ago

You could try to hard-code the path just to test if it still puts the file in both places.

Buckminsterfullerene02 commented 6 months ago

You could try to hard-code the path just to test if it still puts the file in both places.

I did hardcode it already and it still does it.

Buckminsterfullerene02 commented 6 months ago

This likely reinforces the idea that there is some kind of thread problem.

I don't see how that's possible. If we're waiting in UE4SSProgram for the GUI to stop when you close the game, there should be no race condition. If the path is stored locally instead of querying UE4SSProgram, this also removes any possible race condition after the initial setup. The only way you're hitting a race condition then is if you time it perfectly so that you hit 'X' to close at exactly the right time during init, and at that point how are you even consistently reproducing this problem ?

oh yeah true. also it started happening now without me evening having to close the GUI while initialising. I can just leave it to fully initialise and both imgui files started getting saved anyway. even weirder, I no longer can replicate getting those garbage files.

Buckminsterfullerene02 commented 6 months ago

@UE4SS I pushed my testing changes to ImGuiSettingsTestBug branch

UE4SS commented 6 months ago

@UE4SS I pushed my testing changes to ImGuiSettingsTestBug branch

I don't see anything wrong there, and since I'm unable to reproduce the problem I can't do any testing.

Buckminsterfullerene02 commented 6 months ago

For Nark: the imgui.ini being replicated in both working and root dir thing was a me problem, I accidentally overwrote some code somehow that added that new bug, so you can disregard that.

However, what we did figure out is that I could not replicate the bug on debug build, but only on shipping. Going to bed now but muppet will try to replicate on shipping as he did not try that before.

UE4SS commented 6 months ago

I've reproduced the random file problem.

UE4SS commented 6 months ago

I've fixed the problem. It turned out to be a lifetime problem with the temporaries created for the params. We're now storing it in DebuggingGUI as a member variable instead.

I also renamed the window_settings struct to WindowSettings for consistency.

Buckminsterfullerene02 commented 6 months ago

Cannot replicate the issue anymore 👍

narknon commented 6 months ago

Nice, this should be ready for approve and merge then if someone wants to do that.

narknon commented 6 months ago

Storing the size of the split boxes can be another PR later now that we use this file.

UE4SS commented 6 months ago

Needs to be changed to save less often. Either on exit or if moved and every 5 seconds or similar prior to merging.

Are we happy with how often it saves ? Also, do we really need to constantly save ? Saving on exit, or when a change is detected (like when the window has moved) seems a lot better to me than randomly saving.

UE4SS commented 6 months ago

After looking at the code again, I see now that we're not constantly saving, the timer is actually a cooldown on how often it's allowed to save.

narknon commented 6 months ago

Yeah, I had already updated the code to only save if a change and greater than 5 seconds, similarly to how imgui does it generally.