blizzless / blizzless-diiis

Fully-functional open-source server implementation for Diablo 3
GNU Affero General Public License v3.0
1.37k stars 352 forks source link

From today the server is not cross-platform #106

Closed goremykin closed 1 year ago

goremykin commented 1 year ago

I afraid that this commit breaks linux and macos support + potential dockerization by importing windows dlls: https://github.com/blizzless/blizzless-diiis/commit/6325e825021e5fe2cf851b2a30dd934457a31c1a

[DllImport("kernel32.dll", ExactSpelling = true)]
[DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]

I am working from linux and even planned to wrap the server into docker, so any user without experience will be able to run the server without downloading VS. So it would be nice if we replace these imports with something cross-platform

iamdroppy commented 1 year ago

this is only called on windows platforms.

Em sex., 27 de jan. de 2023 19:23, Stepan Goremykin < @.***> escreveu:

I afraid that this commit breaks linux and macos support + potential dockerization by importing windows dlls: 6325e82 https://github.com/blizzless/blizzless-diiis/commit/6325e825021e5fe2cf851b2a30dd934457a31c1a

[DllImport("kernel32.dll", ExactSpelling = true)] [DllImport("user32.dll", CharSet = CharSet.Auto, SetLastError = true)]

— Reply to this email directly, view it on GitHub https://github.com/blizzless/blizzless-diiis/issues/106, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXQB5CUOYM5GBUFPI6L5R3WURDETANCNFSM6AAAAAAUJHNIYE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

goremykin commented 1 year ago

@iamdroppy

[DllImport("kernel32.dll", ExactSpelling = true)]

static extern IntPtr GetConsoleWindow();
static IntPtr ThisConsole = GetConsoleWindow();

The last line of code is called for any platform and crashes

iamdroppy commented 1 year ago

Agreed. Will be fixed.

iamdroppy commented 1 year ago

Done. Freel free to reopen the issue if there's any more issues relating to cross-platform issues, even if it's not a recent issue.

goremykin commented 1 year ago

@iamdroppy, Thanks! Do I understand the problem correctly, that you would like to somehow increase the space for the information we display?

iamdroppy commented 1 year ago

@goremykin check this configuration in your settings, you'll understand why a greater console area is beneficial:

[ConsoleLog]
Enabled = false
Target = Console
IncludeTimeStamps = true
MinimumLevel = Debug
MaximumLevel = Fatal

[AnsiLog]
Enabled = true
Target = Ansi
IncludeTimeStamps = true
MinimumLevel = Trace
MaximumLevel = Fatal

Basically it uses AnsiConsole (from https://spectreconsole.net/) to display logging, and other data. It has a clearer view even though it's not recommended for hard-debugging.

goremykin commented 1 year ago

@iamdroppy, I see. Maybe we should investigate how such projects like lazydocker built adaptive UI in console. Maybe even this spectreconsole supports adaptability?

iamdroppy commented 1 year ago

it does support adaptability. I just thought it would be better to expand the context for a better clear view on the logs.

LazyDocker seems impressive by the way

Have a go with spectreconsole, it's not long since I learned the library - so there's a lot of room for improvement.

To be quite honest, it's been a while I want to remake this logging system. It would be a good issue to solve. It's currently a mess and the AnsiTarget is another one - I was just thinking about new ways we could improve the read of data.

The console size was just a small change, can be undone if requested or PR (I'll immediately approve). But I wonder why not expand the area you are most likely to watch.

Anyhow, we could discuss the logging remake, if you have discord, please join us - my username is Droppy (Droppy#5264).

goremykin commented 1 year ago

The console size was just a small change, can be undone if requested or PR (I'll immediately approve). But I wonder why not expand the area you are most likely to watch.

I understand your point and fully agree with you, but usually console applications don't know anything about windows and their sizes. For example you can connect to the server via ssh and of course the app won't know about ssh session and can't access to the terminal size on my machine.

So probably we have to make our app more flexible...

Anyhow, we could discuss the logging remake, if you have discord, please join us - my username is Droppy (Droppy#5264)

At the moment, I don't have time to do any "major" changes (for example, redo logging), but I sometimes have a little time to make minor changes without diving deep into the project... I will contact you on discord when I have a little more time, okay?

iamdroppy commented 1 year ago

That's why the changes were meant for windows, yes, I totally understand and agree that console sizes shouldn't change - but they won't and the Logging changes doesn't require a certain width/height. But then, again, why show the console (when you can) at a desirable size? It shouldn't matter. If you are logging in to SSH that's fine, no changes there and the console will be based on TTY size. But if you are on a specific condition/server, if it can look better maximized, then why not?

In regards to discord, everyone has their business outside the project, but pass-by, drop a hello! Your opinion and expertise matters.

Have a nice weekend!

me thinking I made the console too colorful, THAT is not cool.

goremykin commented 1 year ago

@iamdroppy, I have a new idea. So my first suggestion was just to implement adaptive console UI (like lazydocker). This can be challenging.

But what if we implement GUI for regular users and will support not adaptive cli? In the case of GUI we have much more freedom in terms of adaptivity, sizes, etc

iamdroppy commented 1 year ago

A GUI would be nice - not to mention cof windowsforms cof - perhaps a ASP.NET Core (Blazor or whatever) running - we could implement a console logging mechanism and have control over everything, and remotely

A pro side is to switch the HTTP handler/parser done from scratch in /REST (so these controllers are public and this-RCON-GUI for allowed IPs)....