-
Notifications
You must be signed in to change notification settings - Fork 19
Editor GUI Fixes #15
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.
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
Editor GUI Fixes #15
Changes from 13 commits
9263c10
88a8895
34a6b01
74c1c96
87dcb2a
3d11bc8
b478c7b
1a33f4d
39c4fce
5f7b79c
5bda990
66c60cf
a0dc185
e5edbf4
05fc35c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Reflection; | ||
|
|
@@ -30,10 +31,12 @@ public partial class DllManipulator | |
| private static List<NativeFunction> _mockedNativeFunctions = new List<NativeFunction>(); | ||
| private static int _createdDelegateTypes = 0; | ||
| private static int _lastNativeCallIndex = 0; //Use with synchronization | ||
| private static List<MethodInfo> _customLoadedTriggers = null; | ||
| private static List<MethodInfo> _customBeforeUnloadTriggers = null; | ||
| private static List<MethodInfo> _customAfterUnloadTriggers = null; | ||
|
|
||
|
|
||
| private static List<Tuple<MethodInfo, bool>> _customLoadedTriggers = null; //List of callbacks to run, whether to run them on the main thread. | ||
| private static List<Tuple<MethodInfo, bool>> _customBeforeUnloadTriggers = null; | ||
| private static List<Tuple<MethodInfo, bool>> _customAfterUnloadTriggers = null; | ||
| private static ConcurrentQueue<Tuple<MethodInfo, object[]>> _mainThreadTriggerQueue = new ConcurrentQueue<Tuple<MethodInfo, object[]>>(); | ||
|
|
||
| /// <summary> | ||
| /// Initialization. | ||
| /// Finds and mocks relevant native function declarations. | ||
|
|
@@ -81,17 +84,16 @@ internal static void Initialize(int unityMainThreadId, string assetsPath) | |
| if (Options.mockAllNativeFunctions || method.IsDefined(typeof(MockNativeDeclarationAttribute)) || method.DeclaringType.IsDefined(typeof(MockNativeDeclarationsAttribute))) | ||
| MockNativeFunction(method); | ||
| } | ||
| else if(method.IsDefined(typeof(NativeDllLoadedTriggerAttribute))) | ||
| { | ||
| RegisterTriggerMethod(method, ref _customLoadedTriggers); | ||
| } | ||
| else if (method.IsDefined(typeof(NativeDllBeforeUnloadTriggerAttribute))) | ||
| else | ||
| { | ||
| RegisterTriggerMethod(method, ref _customBeforeUnloadTriggers); | ||
| } | ||
| else if (method.IsDefined(typeof(NativeDllAfterUnloadTriggerAttribute))) | ||
| { | ||
| RegisterTriggerMethod(method, ref _customAfterUnloadTriggers); | ||
| if (method.IsDefined(typeof(NativeDllLoadedTriggerAttribute))) | ||
| RegisterTriggerMethod(method, ref _customLoadedTriggers, method.GetCustomAttribute<NativeDllLoadedTriggerAttribute>().UseMainThreadQueue); | ||
|
|
||
| if (method.IsDefined(typeof(NativeDllBeforeUnloadTriggerAttribute))) | ||
| RegisterTriggerMethod(method, ref _customBeforeUnloadTriggers, method.GetCustomAttribute<NativeDllBeforeUnloadTriggerAttribute>().UseMainThreadQueue); | ||
|
|
||
| if (method.IsDefined(typeof(NativeDllAfterUnloadTriggerAttribute))) | ||
| RegisterTriggerMethod(method, ref _customAfterUnloadTriggers, method.GetCustomAttribute<NativeDllAfterUnloadTriggerAttribute>().UseMainThreadQueue); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -101,18 +103,33 @@ internal static void Initialize(int unityMainThreadId, string assetsPath) | |
| LoadAll(); | ||
| } | ||
|
|
||
| private static void RegisterTriggerMethod(MethodInfo method, ref List<MethodInfo> triggersList) | ||
| /// <summary> | ||
| /// Will unload/forget all dll's and reset the state | ||
| /// </summary> | ||
| public static void Reset() | ||
| { | ||
| UnloadAll(); | ||
| ForgetAllDlls(); | ||
| ClearCrashLogs(); | ||
|
|
||
| _customLoadedTriggers?.Clear(); | ||
| _customAfterUnloadTriggers?.Clear(); | ||
| _customBeforeUnloadTriggers?.Clear(); | ||
| } | ||
|
|
||
| private static void RegisterTriggerMethod(MethodInfo method, ref List<Tuple<MethodInfo, bool>> triggersList, bool runOnMainThread) | ||
| { | ||
| var parameters = method.GetParameters(); | ||
| if (parameters.Length == 0 || parameters.Length == 1 && parameters[0].ParameterType == typeof(NativeDll)) | ||
| if (parameters.Length == 0 || parameters.Length == 1 && parameters[0].ParameterType == typeof(NativeDll) | ||
| || parameters.Length == 2 && parameters[0].ParameterType == typeof(NativeDll) && parameters[1].ParameterType == typeof(int)) | ||
| { | ||
| if (triggersList == null) | ||
| triggersList = new List<MethodInfo>(2); | ||
| triggersList.Add(method); | ||
| triggersList = new List<Tuple<MethodInfo, bool>>(); | ||
| triggersList.Add(new Tuple<MethodInfo, bool>(method, runOnMainThread)); | ||
| } | ||
| else | ||
| { | ||
| Debug.LogError($"Trigger method must either take no parameters or one parameter of type {nameof(NativeDll)}. Violation on method {method.Name} in {method.DeclaringType.FullName}"); | ||
| Debug.LogError($"Trigger method must either take no parameters or one parameter of type {nameof(NativeDll)} or two of type {nameof(NativeDll)} and int. Violation on method {method.Name} in {method.DeclaringType.FullName}"); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed it to be clearer and given examples of the possible functions in the The |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -509,20 +526,41 @@ internal static void LoadTargetFunction(NativeFunction nativeFunction, bool igno | |
| } | ||
| } | ||
|
|
||
| private static void InvokeCustomTriggers(List<MethodInfo> triggers, NativeDll dll) | ||
| private static void InvokeCustomTriggers(List<Tuple<MethodInfo, bool>> triggers, NativeDll dll) | ||
| { | ||
| if (triggers == null) | ||
| return; | ||
|
|
||
| foreach(var triggerMethod in triggers) | ||
| foreach(var (methodInfo, useMainThreadQueue) in triggers) | ||
| { | ||
| if (triggerMethod.GetParameters().Length == 1) | ||
| triggerMethod.Invoke(null, new object[] { dll }); | ||
| object[] args; | ||
|
|
||
| // Determine args for method | ||
| if (methodInfo.GetParameters().Length == 2) | ||
| args = new object[] { dll, _unityMainThreadId }; | ||
| else if (methodInfo.GetParameters().Length == 1) | ||
| args = new object[] { dll }; | ||
| else | ||
| triggerMethod.Invoke(null, Array.Empty<object>()); | ||
| args = Array.Empty<object>(); | ||
|
|
||
| // Execute now or queue to the main thread | ||
| if (useMainThreadQueue /*&& Thread.CurrentThread.ManagedThreadId != _unityMainThreadId*/) | ||
| _mainThreadTriggerQueue.Enqueue(new Tuple<MethodInfo, object[]>(methodInfo, args)); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using tuple to wrap arguments, it could be simpler to just store an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes makes sense |
||
| else | ||
| methodInfo.Invoke(null, args); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Executes queued methods. | ||
| /// Should be called from the main thread in Update. | ||
| /// </summary> | ||
| public static void InvokeMainThreadQueue() | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't wanna be fussy, but I'd move the dispatcher stuff to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you only mean to move this function, not the queue itself
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean the queue itself. I don't quite imagine how would you move just this method, so that might be interesting too.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean the queue itself. I don't quite imagine how you would move just the method, so I don't disagree with that now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By making the queue public (its static already)
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, but no, I see this whole in the Script as it is the interface with Unity. |
||
| { | ||
| while (_mainThreadTriggerQueue.TryDequeue(out var action)) | ||
| action.Item1.Invoke(null, action.Item2); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Logs native function's call to file. If that file exists, it is overwritten. One file is maintained for each thread. | ||
| /// Note: This method is being called by dynamically generated code. Be careful when changing its signature. | ||
|
|
@@ -654,6 +692,33 @@ public class DllManipulatorOptions | |
| public bool mockAllNativeFunctions; | ||
| public bool onlyInEditor; | ||
| public bool enableInEditMode; | ||
|
|
||
| public DllManipulatorOptions CloneTo(DllManipulatorOptions other) | ||
| { | ||
| other.dllPathPattern = dllPathPattern; | ||
| other.assemblyNames = (string[]) assemblyNames.Clone(); | ||
| other.loadingMode = loadingMode; | ||
| other.posixDlopenFlags = posixDlopenFlags; | ||
| other.threadSafe = threadSafe; | ||
| other.enableCrashLogs = enableCrashLogs; | ||
| other.crashLogsDir = crashLogsDir; | ||
| other.crashLogsStackTrace = crashLogsStackTrace; | ||
| other.mockAllNativeFunctions = mockAllNativeFunctions; | ||
| other.onlyInEditor = onlyInEditor; | ||
| other.enableInEditMode = enableInEditMode; | ||
|
|
||
| return other; | ||
| } | ||
|
|
||
| public bool Equals(DllManipulatorOptions other) | ||
| { | ||
| return other.dllPathPattern == dllPathPattern && other.assemblyNames.SequenceEqual(assemblyNames) && | ||
| other.loadingMode == loadingMode && other.posixDlopenFlags == posixDlopenFlags && | ||
| other.threadSafe == threadSafe && other.enableCrashLogs == enableCrashLogs && | ||
| other.crashLogsDir == crashLogsDir && other.crashLogsStackTrace == crashLogsStackTrace && | ||
| other.mockAllNativeFunctions == mockAllNativeFunctions && other.onlyInEditor == onlyInEditor && | ||
| other.enableInEditMode == enableInEditMode; | ||
| } | ||
| } | ||
|
|
||
| public enum DllLoadingMode | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,7 @@ private void OnEnable() | |
| { | ||
| if (EditorApplication.isPlaying) | ||
| Destroy(gameObject); | ||
| else | ||
| else if(_singletonInstance != this) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Allows the user to properly toggle enabled in edit mode. |
||
| enabled = false; //Don't destroy as the user may be editing a Prefab | ||
| return; | ||
| } | ||
|
|
@@ -57,6 +57,10 @@ private void OnEnable() | |
|
|
||
| if(EditorApplication.isPlaying || Options.enableInEditMode) | ||
| Initialize(); | ||
|
|
||
| if(!EditorApplication.isPlaying && Options.enableInEditMode) | ||
| EditorApplication.update += Update; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doens't just having
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For |
||
|
|
||
| #else | ||
| if (Options.onlyInEditor) | ||
| return; | ||
|
|
@@ -83,6 +87,22 @@ private void Initialize() | |
| initTimer.Stop(); | ||
| InitializationTime = initTimer.Elapsed; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Note: also called in edit mode if Options.enableInEditMode is set. | ||
| /// </summary> | ||
| private void Update() | ||
| { | ||
| DllManipulator.InvokeMainThreadQueue(); | ||
| } | ||
|
|
||
| #if UNITY_EDITOR | ||
| private void OnDisable() | ||
| { | ||
| if(!EditorApplication.isPlaying && Options.enableInEditMode) | ||
| EditorApplication.update -= Update; | ||
| } | ||
| #endif | ||
|
|
||
| private void OnDestroy() | ||
| { | ||
|
|
@@ -92,9 +112,8 @@ private void OnDestroy() | |
| //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. | ||
|
|
||
| DllManipulator.UnloadAll(); | ||
| DllManipulator.ForgetAllDlls(); | ||
| DllManipulator.ClearCrashLogs(); | ||
| if (DllManipulator.Options != null) // Check that we have initialized | ||
| DllManipulator.Reset(); | ||
| _singletonInstance = null; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,10 +64,23 @@ public class DllManipulatorEditor : Editor | |
| private string[] _allKnownAssemblies = null; | ||
| private DateTime _lastKnownAssembliesRefreshTime; | ||
|
|
||
| /// <summary> | ||
| /// To check if the options have change in order to set the object as dirty | ||
| /// </summary> | ||
| private DllManipulatorOptions _prevOptions = new DllManipulatorOptions(); | ||
|
|
||
| public static event Action RepaintAllEditors = delegate {}; | ||
|
|
||
| public DllManipulatorEditor() | ||
| { | ||
| EditorApplication.pauseStateChanged += _ => Repaint(); | ||
| EditorApplication.playModeStateChanged += _ => Repaint(); | ||
| RepaintAllEditors += Repaint; | ||
| } | ||
|
|
||
| private void Awake() | ||
| { | ||
| ((DllManipulatorScript)target).Options.CloneTo(_prevOptions); | ||
| } | ||
|
|
||
| public override void OnInspectorGUI() | ||
|
|
@@ -114,9 +127,9 @@ public override void OnInspectorGUI() | |
|
|
||
|
|
||
| bool unloadAll; | ||
| if(EditorApplication.isPlaying && t.Options.threadSafe) | ||
| if((EditorApplication.isPlaying || t.Options.enableInEditMode) && t.Options.threadSafe) | ||
| unloadAll = GUILayout.Button(UNLOAD_ALL_DLLS_WITH_THREAD_SAFETY_GUI_CONTENT); | ||
| else if (EditorApplication.isPlaying && !EditorApplication.isPaused && t.Options.loadingMode == DllLoadingMode.Preload) | ||
| else if ((EditorApplication.isPlaying && !EditorApplication.isPaused || t.Options.enableInEditMode) && t.Options.loadingMode == DllLoadingMode.Preload) | ||
| unloadAll = GUILayout.Button(UNLOAD_ALL_DLLS_IN_PLAY_PRELOADED_GUI_CONTENT); | ||
| else | ||
| unloadAll = GUILayout.Button("Unload all DLLs"); | ||
|
|
@@ -127,7 +140,7 @@ public override void OnInspectorGUI() | |
|
|
||
| DrawUsedDlls(usedDlls); | ||
| } | ||
| else if(EditorApplication.isPlaying) | ||
| else if(EditorApplication.isPlaying || t.Options.enableInEditMode) | ||
| { | ||
| GUILayout.BeginHorizontal(); | ||
| GUILayout.FlexibleSpace(); | ||
|
|
@@ -136,13 +149,23 @@ public override void OnInspectorGUI() | |
| GUILayout.EndHorizontal(); | ||
| } | ||
|
|
||
| if(EditorApplication.isPlaying && t.InitializationTime != null) | ||
| if((EditorApplication.isPlaying || t.Options.enableInEditMode) && 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)) | ||
| { | ||
| t.Options.CloneTo(_prevOptions); | ||
| EditorUtility.SetDirty(target); | ||
| } | ||
| } | ||
|
rogerbarton marked this conversation as resolved.
|
||
| } | ||
|
|
||
| private void DrawUsedDlls(IList<NativeDllInfo> usedDlls) | ||
|
|
@@ -339,5 +362,12 @@ public static void UnloadAll() | |
| { | ||
| DllManipulator.UnloadAll(); | ||
| } | ||
|
|
||
| [NativeDllLoadedTrigger(UseMainThreadQueue = true)] | ||
| [NativeDllAfterUnloadTrigger(UseMainThreadQueue = true)] | ||
| public static void RepaintAll() | ||
| { | ||
| RepaintAllEditors.Invoke(); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's usually safer to handle null case with events (which can be done simply by putting ? before the dot). In this case you initialize the event, however I'm not sure if adding and removing an listener doesn't reset it back to null. Either or, it's better to be safe.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed however as the event is initialized with the default delegate it will never be null. https://stackoverflow.com/q/170907/9295437 |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just about the line 39 - I don't think this behavior is really beneficial, I imagine usually one would want main thread events to stay in order. And as of consistency, dispatching frameworks which I recall tend to invoke calls from target ("main") thread right away. For events though, while it doesn't prevent dead-locks as much as with direct calls, I think this also makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've changed it. I wasn't quite sure either which is why I left it commented out:
https://github.com/rogerbarton/UnityNativeTool/blob/a0dc185babf73065dad5935be87e2de2f8d5811a/scripts/DllManipulator.cs#L547-L548