Edit options when initialized, Fix cleanup when recompiling by rogerbarton · Pull Request #20 · mcpiroman/UnityNativeTool

rogerbarton · 2020-05-01T16:18:33Z

Allows editing options when initialized, this is done by having a separate copy of the Options in the script as in the manipulator. Changes are applied when entering play mode or can be applied immediately. This also avoids bugs when using enableInEditMode and changing the options when already initialized.
Properly cleans up when recompiling, the problem is that OnDestroy is not called when recompiling.

See commit messages for details.

…ns copy to DllManipScript When DllManipulator is initialized we pass a copy of the current DllManipulatorScript options. This allows us to keep editing the options when the manipulator is initialized/dlls are loaded as we are editing a copy. This is particularly useful when using enableInEditMode. Changes are always applied at OnEnable as before. To allow modifying without play/stop a DllManipulatorScript.Reinitialize() has been added which can be called via GUI (see DetectOptionChanges) which unloads dlls and initializes with new options. GUI is only shown when there are changes.

When recompiling only `OnDisable` is called, not `OnDestroy`. Now gets the callback for when the assembly is being reloaded, just when finished with compiling. This is called before `OnDisable`, so I just set a flag `_isRecompiling`. Previously the native function `UnityPluginUnload` was not being called causing various bugs.

rogerbarton · 2020-05-01T16:28:55Z

mcpiroman

Some quick notes I left here, will have a closer look soon, lgtm though, thanks!

mcpiroman · 2020-05-04T18:18:28Z

+
+            // 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))


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

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

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.

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

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 · 2020-05-04T18:20:13Z

+                if (_isRecompiling)
+                {
+                    _isRecompiling = false;
+                    OnDestroy();


I don't really like calling a callback method so I'd be after creating separate one

Created a Reset()

mcpiroman · 2020-05-04T18:21:54Z

+            if (DllManipulator.Options != null)
+                DllManipulator.Reset();
+
+#if UNITY_EDITOR


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).

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

mcpiroman · 2020-05-04T18:32:26Z

+            if(_singletonInstance != this)
+                return;
+
+            if (DllManipulator.Options != null)


I'm not certain, but it appears to me that we could reduce the number of different places when options are stored and move them from this class to the DllManipulator itself? TBH I don't really like how it's scattered.

Yes its a bit scattered but I think this makes sense:

DllManipulatorScript has an Options copy which are seen in the inspector.

DllManipulator has Options which are currently used.

DllManipulatorEditor has Options solely for detecting changes in the inspector.

Each .cs only has to consider one Options (except the editor). Having the previousInspectorOptions from the editor in DM means DM has a copy which it never uses. I would leave it as is personally.

Co-authored-by: mcpiroman <38111589+mcpiroman@users.noreply.github.com>

Also removed check if singletonInstance in Reinitialize as this should never occur

mcpiroman · 2020-05-06T09:44:04Z

-                    DllManipulator.Reset();
-                _singletonInstance = null;
-            }
+        public void Reset()


This one doesn't need to be public right?

No, its now private

mcpiroman · 2020-05-06T09:44:58Z

+
+            // 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 · 2020-05-06T09:50:35Z

            _customAfterUnloadTriggers?.Clear();
            _customBeforeUnloadTriggers?.Clear();
+
+            Options = null;


Wait a sec, what does it do?

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

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

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

mcpiroman · 2020-05-06T10:06:38Z

LGTM

rogerbarton added 2 commits May 1, 2020 17:45

mcpiroman reviewed May 4, 2020

View reviewed changes

rogerbarton and others added 2 commits May 6, 2020 09:39

Update scripts/Editor/DllManipulatorEditor.cs

247f2cd

Co-authored-by: mcpiroman <38111589+mcpiroman@users.noreply.github.com>

Review changes

698117a

Also removed check if singletonInstance in Reinitialize as this should never occur

mcpiroman reviewed May 6, 2020

View reviewed changes

rogerbarton added 2 commits May 6, 2020 11:50

Small fix

5091e0c

DllManip.Options private set

50efd28

mcpiroman merged commit ec2d35b into mcpiroman:master May 6, 2020

rogerbarton deleted the fixes branch May 6, 2020 10:13

Conversation

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

Uh oh!

Uh oh!

rogerbarton commented May 1, 2020

Uh oh!

mcpiroman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

mcpiroman commented May 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rogerbarton commented May 1, 2020 •

edited

Loading

rogerbarton May 6, 2020 •

edited

Loading