Skip to content
This repository has been archived by the owner on Feb 28, 2024. It is now read-only.

Various Logging Fixes #86

Merged
merged 5 commits into from
Mar 11, 2023
Merged

Various Logging Fixes #86

merged 5 commits into from
Mar 11, 2023

Conversation

zkxs
Copy link
Collaborator

@zkxs zkxs commented Mar 11, 2023

This does the following:

  • Minor logging cleanup
  • Capture stack traces at top level vs doing frame-skipping arithmetic. I think this is actually mandatory as I was seeing some strange logging behavior on Windows Headless that I can only explain if .NET was inlining method calls, which obviously would ruin my "skip 4 frames" style of arithmetic.
  • Stop intercepting internal .NET calls to AppDomain.GetAssemblies(). It turns out that .NET calls this a lot internally, and my logic that was supposed to be intercepting FrooxEngine calls was also messing with internal .NET calls not initiated by FrooxEngine. I've implemented a smarter stack frame search to solve this.

ljoonal
ljoonal previously approved these changes Mar 11, 2023
Comment on lines 74 to 78
if (Logger.IsDebugEnabled())
{
string actualPlugins = plugins.Keys.Join(delimiter: ", ");
Logger.DebugInternal($"Found {plugins.Count} actual plugins: {actualPlugins}");
}
Copy link
Member

@ljoonal ljoonal Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huumh, didn't we have debug methods just take in a closure that returns the debug string, and only gets called if the debug level is enabled...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... that's a good idea. Silly me for not seeing it.

@@ -527,7 +527,7 @@ internal void Save(bool saveDefaultValues = false, bool immediate = false)
{

Thread thread = Thread.CurrentThread;
NeosMod? callee = Util.ExecutingMod();
NeosMod? callee = Util.ExecutingMod(new(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new(1) is a stack trace? 0.o .NET syntax is weird, I assume this has just omitted the StackTrace part and it's implied...?

Copy link
Collaborator Author

@zkxs zkxs Mar 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the compiler knows a StackTrace is the only type that can go there, so we can omit the explicit type in the constructor call. This is equivalent to new StackTrace(1).

@zkxs zkxs requested a review from ljoonal March 11, 2023 11:36
@zkxs zkxs merged commit c5bb9ae into master Mar 11, 2023
@ljoonal ljoonal deleted the logging-fixes branch March 11, 2023 11:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants