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 1 commit
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
29 changes: 14 additions & 15 deletions scripts/DllManipulatorScript.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ private void OnEnable()
#endif
}

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

Expand All @@ -99,11 +99,7 @@ private void Initialize()
/// </summary>
public void Reinitialize()
{
if(_singletonInstance != this)
return;

if (DllManipulator.Options != null)
DllManipulator.Reset();
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)
Expand Down Expand Up @@ -152,7 +148,7 @@ private void OnDisable()
if (_isRecompiling)
{
_isRecompiling = false;
OnDestroy();
Reset();
}
}
}
Expand All @@ -161,15 +157,18 @@ private void OnDisable()
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;
}
}
}
13 changes: 5 additions & 8 deletions scripts/Editor/DllManipulatorEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,14 +142,16 @@ private void DetectOptionChanges(DllManipulatorScript t)
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.Reinitialize();
t.Initialize();
}
}

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

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

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

Expand Down Expand Up @@ -331,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 @@ -353,8 +352,6 @@ private void DrawOptions(DllManipulatorOptions options)

EditorGUI.indentLevel = prevIndent;
}

GUI.enabled = guiEnabledStack.Pop();
}

/// <summary>
Expand Down