northwood-studios / LocalAdmin-V2

Second version of LocalAdmin for SCP: Secret Laboratory game.
MIT License
22 stars 20 forks source link

Added support for input transfer during logging #92

Open AlexanderK666 opened 3 months ago

AlexanderK666 commented 3 months ago

Now when logging commands in the console, input transfer works, which improves user interaction with the server console, where logs are written in a thread and begin to interfere with viewing the current input.

Tested on LocalAdmin v. 2.5.14 and Windows.

AlexanderK666 commented 3 months ago

Oh, I uploaded the changes from the wrong folder, I'll re-upload them now

AlexanderK666 commented 3 months ago

Oh, GitHub Desktop broke the commit.

zabszk commented 3 months ago

Hi,

Unfortunately this PR makes LA unusable in environments that support only stdin, stdout and stderr, just like Pterodactyl (which is the most popular way of launching SCP:SL servers). It doesn't support ReadKey. I just tested your PR there and it was not possible to run any command in Pterodactyl. Furthermore the output is completely broken there. I attached a few screenshots below.

2024-07-19_17-57-57 2024-07-19_17-58-44 2024-07-19_18-01-04

Additionally I have noticed a significant negative performance impact.

I tested it on regular Ubuntu as well. It works, but it only prints output when you press enter key. I had to press enter key 9 times just to get this output.

2024-07-19_17-56-48

iamalexrouse commented 3 months ago

Hi,

Unfortunately this PR makes LA unusable in environments that support only stdin, stdout and stderr, just like Pterodactyl (which is the most popular way of launching SCP:SL servers). It doesn't support ReadKey. I just tested your PR there and it was not possible to run any command in Pterodactyl. Furthermore the output is completely broken there. I attached a few screenshots below.

2024-07-19_17-57-57

2024-07-19_17-58-44

2024-07-19_18-01-04

Additionally I have noticed a significant negative performance impact.

I tested it on regular Ubuntu as well. It works, but it only prints output when you press enter key. I had to press enter key 9 times just to get this output.

2024-07-19_17-56-48

lmao.

This is pretty much why these things need to be tested on both Windows & Linux before they're marked as ready for review.

zabszk commented 3 months ago

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

iamalexrouse commented 3 months ago

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

Is it possible to implement an os dependent ReadKey()?

zabszk commented 3 months ago

The primary issue isn't lack of testing (bugs can be fixed, of course before merging the PR), but the fact that some environments don't support ReadKey() and other used features.

Is it possible to implement an os dependent ReadKey()?

It's not an OS issue. For example pterodactyl doesn't support "all interactions". It only supports stdin and stdout (and likely stderr, I'm not sure). You can't press a key there. You can only type a line of text and send it as stdin. So it's not an OS limitation (linux supports it), but specific environment limitation.

Just like LA doesn't pass key presses to SCP:SL process itself. It only reads stdout and stderr. So if you for some reason call ReadKey() in SCP:SL itself (no LA), it will never be triggered, even if you press a key in LA.

AlexanderK666 commented 3 months ago

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

iamalexrouse commented 3 months ago

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

While you can add an argument, the change would have to be communicated with people using pterodactyl who decide to get the latest localadmin, so I'd prefer the auto detection, but how?

AlexanderK666 commented 3 months ago

Hmm, I think we can add a check to see if the environment supports ReadKey. If not, we should use the regular ReadLine. Additionally, we should add an argument to the startup options to forcefully change the operating mode.

While you can add an argument, the change would have to be communicated with people using pterodactyl who decide to get the latest localadmin, so I'd prefer the auto detection, but how?

Yes, I meant to implement automatic detection and, in addition to that, a startup argument.

AlexanderK666 commented 3 months ago

implement automatic detection

So, I think I found a way to implement this automatic detection. Screenshot of code

Console.IsInputRedirected is True when running through Docker Compose; I think it will be the same in Pterodactyl. I temporarily commented it out to see if there is another way to implement this. And yes, the catch works successfully. If there is no exception (for example, on Windows), we need to remove the key press wait; for this, I used .Wait(50).

Docker compose launch without `Console.IsInputRedirected` handling

What do you think?

AlexanderK666 commented 3 months ago

I read the Dotnet Runtime code, and it seems that performing additional checks besides Console.IsInputRedirected is unnecessary.

zabszk commented 3 months ago

I didn't see an exceptions when running in pterodactyl, but the entire output was broken, so I can't tell for sure (and there could be some try/catch somewhere in the code).

Of course this will not catch all the edge cases, but the most important is to make sure it works in docker in pterodactyl.

We should decide whether this feature should be opt-in (disabled by default) or opt-out (enabled by default).

AlexanderK666 commented 3 months ago

I didn't see an exceptions when running in pterodactyl, but the entire output was broken, so I can't tell for sure (and there could be some try/catch somewhere in the code).

Of course this will not catch all the edge cases, but the most important is to make sure it works in docker in pterodactyl.

We should decide whether this feature should be opt-in (disabled by default) or opt-out (enabled by default).

I'm thinking of making this feature enabled by default in environments that support the use of Console.ReadKey, while adding the option to disable this via the configuration file or the startup argument. In environments that do not support the use of Console.ReadKey turn it off.

AlexanderK666 commented 3 months ago

I didn't see an exceptions when running in pterodactyl

I think this is because the input is happening in a thread, and there's no try-catch block there.

AlexanderK666 commented 3 months ago

I didn't see an exceptions when running in pterodactyl

I think this is because the input is happening in a thread, and there's no try-catch block there.

In Visual Studio, with debugger: In Visual Studio, with debugger screenshot

In runtime, without debugger: In runtime, without debugger screenshot

zabszk commented 3 months ago

Is it possible for you to test it in linux and in pterodactyl? If not, I can test it after you commit fixes to this PR.

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

AlexanderK666 commented 3 months ago

Is it possible for you to test it in linux and in pterodactyl? If not, I can test it after you commit fixes to this PR.

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

Yes, I have the ability to test in Linux, but I haven't figured out how to work with Pterodactyl yet. I have Docker Compose and Portainer, which theoretically should work in a similar way. Additional testing would definitely be useful.

AlexanderK666 commented 3 months ago

Don't forget about the issue that exists on linux (outside pterodactyl) that I mentioned (the one that requires to press enter key to get new output).

It seems I managed to fix it. To solve this, I added a check for Console.KeyAvailable.

Screenshot

AlexanderK666 commented 3 months ago

So far, I found that the exit command on Windows does not close the window, but I haven't been able to figure out why yet.

AlexanderK666 commented 3 months ago

I did the build via GitHub Actions on my fork repository. P.S.: workflow dispatch is not necessary to contribute to the main repository, it was needed for the fork.

iamalexrouse commented 3 months ago

So far, I found that the exit command on Windows does not close the window, but I haven't been able to figure out why yet.

If you CTRL + C, does it quit the application?

AlexanderK666 commented 3 months ago

If you CTRL + C, does it quit the application?

This closes SCPSL.exe, but the LocalAdmin window in Windows has to be closed manually. On Linux, everything closes correctly.

iamalexrouse commented 3 months ago

If you CTRL + C, does it quit the application?

This closes SCPSL.exe, but the LocalAdmin window in Windows has to be closed manually. On Linux, everything closes correctly.

Console apps are designed by Microsoft to terminate if there is no valid code to process meaning somewhere in your changes, your preventing the console from reaching a termination state.

You’ll need to pause the application state after entering the state and discover what the JIT compiler is struggling to figure out.