Logging improvements

[Exzap]

Hi, first time contributor here. I am interested in making the game compile with MSVC (if thats ok?). But to get there, some posix-only code has to be replaced with cross platform code. I don't want to add even more #ifdef blocks to the codebase and instead I believe it would be better to use the opportunity and upgrade the relevant parts of the code to use cross-platform C++ API. This PR is a small step in that direction. Eventually I also want to contribute features, optimizations and bugfixes once I have a better understanding of the codebase.

Anyway, this PR changes the following:

• Replace some posix-only API in logging.cpp with cross platform code
• Added a utility function mkdir_single to substitute posix mkdir()
• Replaced SDL_LockMutex with std::mutex + std::lock_guard since it's a bit more compact and less error prone
• If logging level is debug_verbose, the timestamp for the debug messages will also show the milliseconds. I added this because it may be useful for measuring performance before/after code rewrites or when working on optimizations.

Also a question: It seems that C++11 is being targeted. Any considerations to raise that up to C++17? Being able to use std::filesystem would simplify a lot of file related code, although it would require some wrappers for interoperability with the .c code.

[pjbroad]

Hi @Exzap , thanks for the PR and welcome to EL dev! I'm one of the committers for the code base but Radu has the final say on major changes. For the Windows build we currently use MSYS see the build methods repo for the details. Before going down that path I had a look a using MSVC but found it pretty poor and not standard compliant. That was a few years ago so things may have changed for the better. As I'm sure you know, POSIX, by it very definition is a portable set of APIs, even windows is meant to be POSIX compliant though that was one of the issues with the MSVC compiler, it was not POSIX compliant. We moved to C++11 a few years ago, held back from a newer standard by support on some of the platforms (I can't remember the details). The client is build on Linux, Windows, OSX and Android, they all need to be checked for compatibility before we move again. Also, when making changes to the client code, we have to ensure the Map Editor build is not broken and that builds with some of the alternative #defs (which do need pruning) don't break either. The EL code base has old roots that are fragile in places, but that does not stop us moving it forwards. I'll take a closer look at your patch when I have time, perhaps at the weekend, but thanks again! Please don't take anything I've said as discouraging your ideas and contribution.

[gvissers]

Disclaimers: I'm not really involved with EL anymore, so take anything I say with a grain of salt. But this PR caught my attention. Also, I just read through the patch, I haven't tested anything.

I fully support using standard C++, dan I think the basic idea is good. However, there's one change that I think might be annoying: in the current code, when the log level is not verbose, anything logged within debug marks will be wiped from the log file when the debug mark is left.[*] If I'm not mistaken, your patch will leave all the data written within debug marks in the log file (including the "Entering/Leaving debug mark" messages). This might add a lot of clutter to the log file.

I'm guessing the debug mark code was mostly added to debug crashes, and in normal operation you don't need the logged data. I'm not saying the current implementation of writing and wiping data is perfect, but neither is filling the log up with debug data.

[*] If the amount of data logged is greater than 1024 bytes at least? Don't ask me the reason, I didn't write the code.

[Exzap]

Yeah I was unsure what to do about the debug mark pruning since it relies on file truncation which ofstream does not support. I could replicate the original behavior by delaying the printing of debug marks until at least one message is printed inside them. But I found that there isn't really that much extra clutter considering a majority of marks are not empty to begin with (at least from what I saw during testing on my end). Anyway, if the original behavior is desired I will change it back. And yeah I couldn't make sense of the +1024 thing either.

Regarding the points brought up by @pjbroad I generally agree and understand. MSVC unfortunately still has incomplete POSIX support. At any rate, I might have given the wrong impression that I want to replace POSIX code only as means to get MSVC compatibility. That's not the case. I think there is a lot of value in slowly migrating old code to STL in general, like having consistent type-safety and other mechanisms which prevent mistakes, even when not using classes. It can avoid some of the fragility that has been mentioned. MSVC compatibility would be a bonus on top of that.

[gvissers]

Yeah I was unsure what to do about the debug mark pruning since it relies on file truncation which ofstream does not support.

Hmmm, yeah, this is a really unfortunate shortcoming of the standard library. C++17 has std::fs::resize_file(), but I think the client is currrently using C++11.

I could replicate the original behavior by delaying the printing of debug marks until at least one message is printed inside them.

That doesn't fully replicate the original behaviour. Even when messages are printed in a debug mark, they are removed if the debug mark is left successfully (if log level is less than verbose). Also, I'm not sure delaying the writing of log messages is advisable, in case of a crash you might want that information to be written to the log.

But I found that there isn't really that much extra clutter considering a majority of marks are not empty to begin with

All log messages within debug marks are removed in the old code when leaving a debug mark if the log level is less than verbose.

[Exzap]

I will see if I can change it back to the original truncation behavior.

C++17 has std::fs::resize_file()

It takes a file path as a parameter and calling it on a file that is being written to probably doesn't work.

[Exzap]

Alright, I reverted the file API and logging behavior changes. For the purpose of MSVC support (IF that were to happen) it is quite trivial to just provide the missing 2-3 posix APIs via platform.h or some solution similar to that. I still believe that using C or C++ standard API is preferable over pure posix because of readability and guaranteed cross-platform support but neither support file truncation so for the existing logging behavior they are out of the question.

Whats left in this PR is:

• The git ignore update for temporary files created by clion IDE
• Using std::lock_guard instead of manual SDL_Lock/Unlock
• Milliseconds in timestamp if log level is debug_verbose
• Some smaller changes to make the code more compact and readable
• The reusable mkdir_single utility function

[pjbroad]

Looks good. The only change I'd perhaps make is to use snprintf() when appending the milliseconds. I know there's enough buffer allocated but it would not hurt. Perhaps also add a newline to the end of the .gitignore file.

I've tested Android with C++17 and it compiles and runs fine (I checked for support of std::filesystem), Linux and MSYS for Windows works too. I'm seeking guidance on MacOS, and if we can confirm that, we could move to C++17.

[pjbroad]

Thanks for do the additional changes.