Editor GUI Fixes by rogerbarton · Pull Request #15 · mcpiroman/UnityNativeTool

rogerbarton · 2020-03-21T14:36:31Z

Editors (component/window) are repainted when dll's are un/loaded using the new attributes
- Involves running the triggers on the main thread in a queue 39c4fce
Editor variable changes are detected and saved properly
- Involves creating a clone of the options and comparing changes when the GUI has changed 5bda990

Note: The attributes [NativeDllLoadedTrigger] etc are not searched for in the source code of this tool itself. So by creating an .asmdef in #14 these attributes may not work, do you maybe know of an easy fix for this @mcpiroman?.

Before changing a variable would not be detected and so not saved/serialized in some cases.

This would not work with the `upm-support` branch mcpiroman#14 as the attributes are not searched for in the attribute that these scripts are in by default.

mcpiroman

Note: The attributes [NativeDllLoadedTrigger] etc are not searched for in the source code of this tool itself

Yeah, I just search for them in the same assemblies as for native function declarations. I guess I should just search every assembly and maybe exclude standard unity ones for perf, because searching in every assembly doesn't sound the quickest thing.

The scene is automatically set as dirty when the gui target is

Admittedly this should have been in mcpiroman#12. However, here are the fixes anyways. GUI buttons to un/load show in edit mode if enableInEditMode is true. Disabling and re-enabling the DllManipulatorScript works now.

Previously triggers would not be cleared properly and so be duplicated. (Static variables seem to persist between entering/exiting playmode)

Allow custom triggers to be executed on the main thread in a queue. We need this internally to repaint the editor GUI as it uses the Unity API.

rogerbarton · 2020-04-17T08:11:35Z

                if (EditorApplication.isPlaying)
                    Destroy(gameObject);
-                else
+                else if(_singletonInstance != this)


Allows the user to properly toggle enabled in edit mode.

rogerbarton · 2020-04-17T08:18:31Z

Ready for review @mcpiroman.

You will probably want to review 39c4fce separately.

(side note: #14 should already be compatible with these changes)

Initialize() is not always called so we should not always reset.

mcpiroman

Sorry this took me this long to have a look at this.
I'm somewhat scared about that option cloning, comparing (mainly bout forgetting to update that), but (partly because I'm not quite in the problem it tries to solve), I don't see a better way.

mcpiroman · 2020-04-30T10:26:03Z

+        /// <summary>
+        /// Should the method always be executed on the main thread, to allow use of the Unity API.
+        /// Note: this means the method is not immediately executed but put in a queue.
+        /// For consistent behaviour, the method is put in the queue even if it is triggered from the main thread.


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.

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

mcpiroman · 2020-04-30T10:30:10Z

+
+                // Execute now or queue to the main thread
+                if (useMainThreadQueue /*&& Thread.CurrentThread.ManagedThreadId != _unityMainThreadId*/)
+                    _mainThreadTriggerQueue.Enqueue(new Tuple<MethodInfo, object[]>(methodInfo, args));


Instead of using tuple to wrap arguments, it could be simpler to just store an Action created with lambda, like so:
_mainThreadTriggerQueue.Enqueue(() => methodInfo.Invoke(null, args)); (You could also make e.g. Action<int> to provide an argument at call side, JIC you didn't know).

Yes makes sense

mcpiroman · 2020-04-30T10:34:03Z

            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}");


or two of type {nameof(NativeDll)} and int at first reading this sounded like two NativeDll parameters and one int parameter, don't know if that's just me though.
Also why do you assume the main thread id information would be useful? And when it turns out to be, it should be documented somehow (or I just missed that, in which case I'm sorry)

I've changed it to be clearer and given examples of the possible functions in the TriggerAttribute doc.

The mainThreadId can be used to tell if we are on the main thread and are allowed to call unity functions. I guess this is partially obsolete now that we have the MainThreadTriggerQueue.

mcpiroman · 2020-04-30T10:41:43Z

+        /// Executes queued methods.
+        /// Should be called from the main thread in Update.
+        /// </summary>
+        public static void InvokeMainThreadQueue()


Don't wanna be fussy, but I'd move the dispatcher stuff to DllManipulatorScript, as it is closer to Unity, whilst this is intended to do 'the stuff', if you will.

I assume you only mean to move this function, not the queue itself

I mean the queue itself. I don't quite imagine how would you move just this method, so that might be interesting too.

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.

By making the queue public (its static already)

Ah yes, but no, I see this whole in the Script as it is the interface with Unity.

mcpiroman · 2020-04-30T10:44:41Z

                Initialize();
+
+            if(!EditorApplication.isPlaying && Options.enableInEditMode)
+                EditorApplication.update += Update;


Doens't just having [ExecuteInEditMode] make the Update be invoke in editor? Or do I misunderstand sth.

For ExecuteInEditMode "Update is only called when something in the Scene changed" - Docs. So we need to manually add this to cover all cases.

mcpiroman · 2020-04-30T10:49:14Z

+        [NativeDllAfterUnloadTrigger(UseMainThreadQueue = true)]
+        public static void RepaintAll()
+        {
+            RepaintAllEditors.Invoke();


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.

Agreed however as the event is initialized with the default delegate it will never be null. https://stackoverflow.com/q/170907/9295437
Ill add it anyways...

Also RegisterTriggerMethod receives attribute instead of bool, Fix naming in DllManipulatorOptions

rogerbarton · 2020-04-30T19:22:08Z

Regarding cloning the Options, its just that we need a separate copy for detecting changes. This copy is never changed directly only update when changes are made. Its pretty safe in my opinion.

I've actually found this quite a good solution.

mcpiroman · 2020-05-01T11:05:05Z

I'm fine with that, thanks!

rogerbarton added 3 commits March 21, 2020 15:37

Repaint editor GUIs on shortcut

9263c10

Editor variable changes saved properly

88a8895

Before changing a variable would not be detected and so not saved/serialized in some cases.

Use callback attributes to repaint editors

34a6b01

This would not work with the `upm-support` branch mcpiroman#14 as the attributes are not searched for in the attribute that these scripts are in by default.

rogerbarton force-pushed the shortcuts branch from 373ea4d to 34a6b01 Compare March 21, 2020 14:45

mcpiroman reviewed Mar 23, 2020

View reviewed changes

Comment thread scripts/Editor/DllManipulatorWindowEditor.cs Outdated

Comment thread scripts/Editor/DllManipulatorEditor.cs Outdated

Comment thread scripts/Editor/DllManipulatorEditor.cs Outdated

mcpiroman mentioned this pull request Mar 23, 2020

Use relative path for assemblyPaths #17

Closed

rogerbarton added 2 commits March 24, 2020 14:48

Allow multiple un/loadTrigger attributes, use action to trigger repaints

74c1c96

Remove SceneManagement, it's not required

87dcb2a

The scene is automatically set as dirty when the gui target is

rogerbarton force-pushed the shortcuts branch from bad463c to 87dcb2a Compare March 25, 2020 18:51

rogerbarton commented Apr 7, 2020

View reviewed changes

Comment thread scripts/Editor/DllManipulatorEditor.cs

rogerbarton added 2 commits April 8, 2020 20:29

Merge branch 'master' into shortcuts

3d11bc8

Fixes to gui with enableInEdit mode

b478c7b

Admittedly this should have been in mcpiroman#12. However, here are the fixes anyways. GUI buttons to un/load show in edit mode if enableInEditMode is true. Disabling and re-enabling the DllManipulatorScript works now.

rogerbarton mentioned this pull request Apr 9, 2020

Add upm support #14

Merged

4 tasks

rogerbarton added 4 commits April 16, 2020 22:39

Properly reset custom triggers (fix duplicates)

1a33f4d

Previously triggers would not be cleared properly and so be duplicated. (Static variables seem to persist between entering/exiting playmode)

Custom triggers optional execute on main thread

39c4fce

Allow custom triggers to be executed on the main thread in a queue. We need this internally to repaint the editor GUI as it uses the Unity API.

Update README.md

5f7b79c

Mark scene dirty only if options changed

5bda990

rogerbarton commented Apr 17, 2020

View reviewed changes

rogerbarton added 2 commits April 17, 2020 11:13

Bug fix, resetting when not initialized

66c60cf

Initialize() is not always called so we should not always reset.

small fix

a0dc185

rogerbarton requested a review from mcpiroman April 25, 2020 08:13

mcpiroman reviewed Apr 30, 2020

View reviewed changes

Review fixes

e5edbf4

Also RegisterTriggerMethod receives attribute instead of bool, Fix naming in DllManipulatorOptions

Add comments

05fc35c

mcpiroman merged commit 1f0e9bc into mcpiroman:master May 1, 2020

Conversation

rogerbarton commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcpiroman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogerbarton commented Apr 17, 2020

Uh oh!

mcpiroman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogerbarton commented Apr 30, 2020

Uh oh!

mcpiroman commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rogerbarton commented Mar 21, 2020 •

edited

Loading