Skip to content

Edit options when initialized, Fix cleanup when recompiling #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mcpiroman merged 6 commits into from
May 6, 2020
Merged

Edit options when initialized, Fix cleanup when recompiling #20

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20e9645
Allow editing options when initialized, DllManip has a separate Optio…
rogerbarton May 1, 2020
80ea4fe
Fix OnDestroy not being called when recompiling
rogerbarton May 1, 2020
247f2cd
Update scripts/Editor/DllManipulatorEditor.cs
rogerbarton May 6, 2020
698117a
Review changes
rogerbarton May 6, 2020
5091e0c
Small fix
rogerbarton May 6, 2020
50efd28
DllManip.Options private set
rogerbarton May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion scripts/DllManipulator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,11 @@ public partial class DllManipulator
/// If <see cref="DllLoadingMode.Preload"/> option is specified, loads all DLLs specified by these functions.
/// Options have to be configured before calling this method.
/// </summary>
internal static void Initialize(int unityMainThreadId, string assetsPath)
internal static void Initialize(DllManipulatorOptions options, int unityMainThreadId, string assetsPath)
{
// Make a deep copy of the options so we can edit them in DllManipulatorScript independently
Options = new DllManipulatorOptions();
options.CloneTo(Options);
_unityMainThreadId = unityMainThreadId;
_assetsPath = assetsPath;

Expand Down Expand Up @@ -123,6 +126,8 @@ public static void Reset()
_customLoadedTriggers?.Clear();
_customAfterUnloadTriggers?.Clear();
_customBeforeUnloadTriggers?.Clear();

Options = null;

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Wait a sec, what does it do?

@rogerbarton rogerbarton May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We delete the DllManip Options copy. This is also used as an indicator to whether the DllManip is initialized.

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I mean, how does it consume the options when they are null? Sry if I miss sth

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ah nvm, on the diff it blended in as though it was called in Initalize, not Reset, sorry

}

private static void RegisterTriggerMethod(MethodInfo method, ref List<Tuple<MethodInfo, bool>> triggersList, TriggerAttribute attribute)
Expand Down
66 changes: 53 additions & 13 deletions scripts/DllManipulatorScript.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ private void OnEnable()
DontDestroyOnLoad(gameObject);

if(EditorApplication.isPlaying || Options.enableInEditMode)
{
Initialize();

AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload;
}

// Ensure update is called every frame in edit mode, ExecuteInEditMode only calls Update when the scene changes
if(!EditorApplication.isPlaying && Options.enableInEditMode)
EditorApplication.update += Update;
Expand All @@ -80,16 +83,29 @@ private void OnEnable()
#endif
}

private void Initialize()
public void Initialize()
{
var initTimer = System.Diagnostics.Stopwatch.StartNew();

DllManipulator.Options = Options;
DllManipulator.Initialize(Thread.CurrentThread.ManagedThreadId, Application.dataPath);
DllManipulator.Initialize(Options, Thread.CurrentThread.ManagedThreadId, Application.dataPath);

initTimer.Stop();
InitializationTime = initTimer.Elapsed;
}

/// <summary>
/// Will reset the DllManipulator and Initialize it again.
/// Note: Unloads all Dlls, may be a dangerous operation if using preloaded
/// </summary>
public void Reinitialize()
{
DllManipulator.Reset();

#if UNITY_EDITOR

@mcpiroman mcpiroman May 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think if this is not going to actually perform the initialization again, then this method shouldn't be called in the first place (it doesn't actually reinitialize).

@rogerbarton rogerbarton May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've changed it although this requires that the Initialize function is public.

if(EditorApplication.isPlaying || Options.enableInEditMode)
#endif
Initialize();
}

/// <summary>
/// Note: also called in edit mode if Options.enableInEditMode is set.
Expand All @@ -110,25 +126,49 @@ public static void InvokeMainThreadQueue()
}

#if UNITY_EDITOR
private bool _isRecompiling;
/// <summary>
/// Called when Assemblies are reloaded due to recompilation.
/// Called before OnDisable.
/// </summary>
private void OnBeforeAssemblyReload()
{
_isRecompiling = true;
}

private void OnDisable()
{
if(!EditorApplication.isPlaying && Options.enableInEditMode)
if(_singletonInstance == this && !EditorApplication.isPlaying && Options.enableInEditMode)
{
EditorApplication.update -= Update;
AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload;

// When recompiling OnDestroy is not called by default (the object is not really destroyed)
// Manually trigger OnDestroy to clean up if we are disabled because of recompilation
if (_isRecompiling)
{
_isRecompiling = false;
Reset();
}
}
}
#endif

private void OnDestroy()
{
if (_singletonInstance == this)
{
//Note on threading: Because we don't wait for other threads to finish, we might be stealing function delegates from under their nose if Unity doesn't happen to close them yet.
//On Preloaded mode this leads to NullReferenceException, but on Lazy mode the DLL and function would be just reloaded so we would up with loaded DLL after game exit.
//Thankfully thread safety with Lazy mode is not implemented yet.
Reset();
}

if (DllManipulator.Options != null) // Check that we have initialized
DllManipulator.Reset();
_singletonInstance = null;
}
public void Reset()

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This one doesn't need to be public right?

@rogerbarton rogerbarton May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, its now private

{
//Note on threading: Because we don't wait for other threads to finish, we might be stealing function delegates from under their nose if Unity doesn't happen to close them yet.
//On Preloaded mode this leads to NullReferenceException, but on Lazy mode the DLL and function would be just reloaded so we would up with loaded DLL after game exit.
//Thankfully thread safety with Lazy mode is not implemented yet.

if (DllManipulator.Options != null) // Check that we have initialized
DllManipulator.Reset();
_singletonInstance = null;
}
}
}
111 changes: 81 additions & 30 deletions scripts/Editor/DllManipulatorEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace UnityNativeTool.Internal
public class DllManipulatorEditor : Editor
{
private static readonly string INFO_BOX_GUI_CONTENT =
"Mocks native functions to allow manually un/loading native DLLs. DLLs are always unloaded at OnDestroy.";
"Mocks native functions to allow manually un/loading native DLLs. DLLs are always unloaded at OnDestroy. Configuration changes below are always applied at OnEnable.";
private static readonly GUIContent TARGET_ALL_NATIVE_FUNCTIONS_GUI_CONTENT = new GUIContent("All native functions",
"If true, all found native functions will be mocked.\n\n" +
$"If false, you have to select them by using [{nameof(MockNativeDeclarationsAttribute)}] or [{nameof(MockNativeDeclarationAttribute)}].");
Expand Down Expand Up @@ -56,10 +56,23 @@ public class DllManipulatorEditor : Editor
private static readonly GUIContent UNLOAD_ALL_DLLS_IN_PLAY_PRELOADED_GUI_CONTENT = new GUIContent("Unload all DLLs [dangerous]",
"Use only if you are sure no mocked native calls will be made while DLL is unloaded.");
private static readonly GUIContent UNLOAD_ALL_DLLS_WITH_THREAD_SAFETY_GUI_CONTENT = new GUIContent("Unload all DLLs [dangerous]",
"Use only if you are sure no other thread will be call mocked natives.");
"Use only if you are sure no other thread will call mocked natives.");
private static readonly GUIContent UNLOAD_ALL_DLLS_AND_PAUSE_WITH_THREAD_SAFETY_GUI_CONTENT = new GUIContent("Unload all DLLs & Pause [dangerous]",
"Use only if you are sure no other thread will be call mocked natives.");
"Use only if you are sure no other thread will call mocked natives.");
private static readonly TimeSpan ASSEMBLIES_REFRESH_INTERVAL = TimeSpan.FromSeconds(5);

private static readonly GUIContent INITIALIZE_ENABLED_EDIT_MODE_GUI_CONTENT = new GUIContent(
"Apply Changes Now & Initialize",
"Start mocking native functions in edit mode immediately without waiting for OnEnable.");
private static readonly GUIContent REINITIALIZE_WITH_CHANGES_LAZY_GUI_CONTENT = new GUIContent(
"Unload, Apply Changes Now & Reinitialize",
"Changes made to the options above are only applied when play(/edit) mode is entered." +
" Use this to unload all Dlls and initialize with the new changes immediately.");
private static readonly GUIContent REINITIALIZE_WITH_CHANGES_PRELOADED_GUI_CONTENT = new GUIContent(
"Unload, Apply Changes Now & Reinitialize [Dangerous]",
"Changes made to the options above are only applied when play(/edit) mode is entered. " +
"Use this to unload all Dlls and initialize with the new changes immediately. " +
"Use only if you are sure no mocked native calls will be made while DLL is unloaded.");

private bool _showLoadedLibraries = true;
private bool _showTargetAssemblies = true;
Expand Down Expand Up @@ -93,12 +106,69 @@ public override void OnInspectorGUI()
EditorGUILayout.HelpBox(INFO_BOX_GUI_CONTENT, MessageType.Info);

DrawOptions(t.Options);

DetectOptionChanges(t);

EditorGUILayout.Space();

DrawCurrentState(t);
}

/// <summary>
/// Detects whether the <see cref="DllManipulatorScript.Options"/> have changed, both relative to the previous
/// options and the <see cref="DllManipulator.Options"/> if we are currently initialized.
/// </summary>
/// <param name="t">The OnInspectorGUI target</param>
private void DetectOptionChanges(DllManipulatorScript t)
{
// Set the target as dirty so changes can be saved, if there are changes
if (GUI.changed)
{
if (!t.Options.Equals(_prevOptions))
{
// If the options have changed then update the _prevOptions and notify there are changes to be saved
// CloneTo is used to ensure a deep copy is made
t.Options.CloneTo(_prevOptions);
EditorUtility.SetDirty(target);
}
}

// Allow Reinitializing DllManipulator if there are changes
if (DllManipulator.Options != null && !t.Options.Equals(DllManipulator.Options))
{
if (DllManipulator.Options.loadingMode == DllLoadingMode.Preload)
{
if (GUILayout.Button(REINITIALIZE_WITH_CHANGES_PRELOADED_GUI_CONTENT))
t.Reinitialize();
}
else if(GUILayout.Button(REINITIALIZE_WITH_CHANGES_LAZY_GUI_CONTENT))
{
t.Reinitialize();
Comment thread
rogerbarton marked this conversation as resolved.
}
}

// When enabling enableInEditMode for the first time, allow immediately initializing without waiting for OnEnable
if(DllManipulator.Options == null && t.Options.enableInEditMode && !EditorApplication.isPlaying &&
GUILayout.Button(INITIALIZE_ENABLED_EDIT_MODE_GUI_CONTENT))

@mcpiroman mcpiroman May 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would maybe move that button to be close to the 'enable in edit mode' option

@mcpiroman mcpiroman May 4, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I mean you enable it and this shows up right next to it so that you know you have to click it first for it to work, sth like warning button i guess

@rogerbarton rogerbarton May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think its reasonably clear when the button pops up. If the user immediately clicks the initialize button and then changes the options below they will have to unnecessarily reinitialize. Having the button at the bottom implies that the other options also affect its behaviour.

Also in the code it is clearer as the functionality about re/initializing is together in DetectOptionChanges.

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yup

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Although, it looks like Options in DllManipulator can now be private?

@rogerbarton rogerbarton May 6, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use it in the Script and Editor to check if the DllManip is initialized/there are changes to be applied. We could make the setter private?

@mcpiroman mcpiroman May 6, 2020

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

yes

{
t.Initialize();
}
}

/// <summary>
/// Draws GUI related to the current state of the DllManipulator.
/// Buttons to load/unload Dlls as well as details about which Dlls are loaded
/// </summary>
/// <param name="t">The OnInspectorGUI target</param>
private void DrawCurrentState(DllManipulatorScript t)
{
if (DllManipulator.Options == null) // Exit if we have not initialized DllManipulator
return;

var usedDlls = DllManipulator.GetUsedDllsInfos();
if (usedDlls.Count != 0)
{
if(t.Options.loadingMode == DllLoadingMode.Preload && usedDlls.Any(d => !d.isLoaded))
if(DllManipulator.Options.loadingMode == DllLoadingMode.Preload && usedDlls.Any(d => !d.isLoaded))
{
if (EditorApplication.isPaused)
{
Expand All @@ -118,7 +188,7 @@ public override void OnInspectorGUI()
if (EditorApplication.isPlaying && !EditorApplication.isPaused)
{
bool pauseAndUnloadAll;
if(t.Options.threadSafe)
if(DllManipulator.Options.threadSafe)
pauseAndUnloadAll = GUILayout.Button(UNLOAD_ALL_DLLS_AND_PAUSE_WITH_THREAD_SAFETY_GUI_CONTENT);
else
pauseAndUnloadAll = GUILayout.Button("Unload all DLLs & Pause");
Expand All @@ -132,9 +202,9 @@ public override void OnInspectorGUI()


bool unloadAll;
if((EditorApplication.isPlaying || t.Options.enableInEditMode) && t.Options.threadSafe)
if(DllManipulator.Options.threadSafe)
unloadAll = GUILayout.Button(UNLOAD_ALL_DLLS_WITH_THREAD_SAFETY_GUI_CONTENT);
else if ((EditorApplication.isPlaying && !EditorApplication.isPaused || t.Options.enableInEditMode) && t.Options.loadingMode == DllLoadingMode.Preload)
else if (DllManipulator.Options.loadingMode == DllLoadingMode.Preload && (EditorApplication.isPlaying && !EditorApplication.isPaused || DllManipulator.Options.enableInEditMode))
unloadAll = GUILayout.Button(UNLOAD_ALL_DLLS_IN_PLAY_PRELOADED_GUI_CONTENT);
else
unloadAll = GUILayout.Button("Unload all DLLs");
Expand All @@ -145,7 +215,7 @@ public override void OnInspectorGUI()

DrawUsedDlls(usedDlls);
}
else if(EditorApplication.isPlaying || t.Options.enableInEditMode)
else
{
GUILayout.BeginHorizontal();
GUILayout.FlexibleSpace();
Expand All @@ -154,25 +224,13 @@ public override void OnInspectorGUI()
GUILayout.EndHorizontal();
}

if((EditorApplication.isPlaying || t.Options.enableInEditMode) && t.InitializationTime != null)
if (t.InitializationTime != null)
{
EditorGUILayout.Space();
EditorGUILayout.Space();
var time = t.InitializationTime.Value;
EditorGUILayout.LabelField($"Initialized in: {(int)time.TotalSeconds}.{time.Milliseconds.ToString("D3")}s");
}

// Set the target as dirty so changes can be saved, if there are changes
if (GUI.changed)
{
if (!t.Options.Equals(_prevOptions))
{
// If the options have changed then update the _prevOptions and notify there are changes to be saved
// CloneTo is used to ensure a deep copy is made
t.Options.CloneTo(_prevOptions);
EditorUtility.SetDirty(target);
}
}
}

private void DrawUsedDlls(IList<NativeDllInfo> usedDlls)
Expand Down Expand Up @@ -208,11 +266,6 @@ private void DrawUsedDlls(IList<NativeDllInfo> usedDlls)

private void DrawOptions(DllManipulatorOptions options)
{
var guiEnabledStack = new Stack<bool>();
guiEnabledStack.Push(GUI.enabled);
if (EditorApplication.isPlaying)
GUI.enabled = false;

options.onlyInEditor = EditorGUILayout.Toggle(ONLY_IN_EDITOR, options.onlyInEditor);
options.enableInEditMode = EditorGUILayout.Toggle(ENABLE_IN_EDIT_MODE, options.enableInEditMode);

Expand Down Expand Up @@ -277,14 +330,14 @@ private void DrawOptions(DllManipulatorOptions options)
options.posixDlopenFlags = (PosixDlopenFlags)EditorGUILayout.EnumPopup(POSIX_DLOPEN_FLAGS_GUI_CONTENT, options.posixDlopenFlags);
#endif

guiEnabledStack.Push(GUI.enabled);
var guiEnabled = GUI.enabled;
if (options.loadingMode != DllLoadingMode.Preload)
{
options.threadSafe = false;
GUI.enabled = false;
}
options.threadSafe = EditorGUILayout.Toggle(THREAD_SAFE_GUI_CONTENT, options.threadSafe);
GUI.enabled = guiEnabledStack.Pop();
GUI.enabled = guiEnabled;

options.enableCrashLogs = EditorGUILayout.Toggle(CRASH_LOGS_GUI_CONTENT, options.enableCrashLogs);

Expand All @@ -299,8 +352,6 @@ private void DrawOptions(DllManipulatorOptions options)

EditorGUI.indentLevel = prevIndent;
}

GUI.enabled = guiEnabledStack.Pop();
}

/// <summary>
Expand Down