Skip to content

Conversation

@otoomey
Copy link

@otoomey otoomey commented Apr 10, 2020

To recap Issue #242: XNode's NodeEditor does not have an OnEnable method like the Unity Editor does. This PR adds an OnEnable virtual method to NodeEditor and calls it when the editor is instantiated - for instance, when a graph containing the node is opened, or when the graph window switches to a graph containing the node.

I have also deprecated NodeEditor.OnCreate as it has been superseded by OnEnable, which has a name more inline with what I assume most developers would expect. Currently OnCreate functions identically to OnEnable.

I made a major change to the way editors are cached in order to make OnEnable work as expected. XNode stores the type and an instance of every NodeEditor it can find in NodeEditorBase. Keeping the type cached makes sense, as reflection is expensive and it's highly unlikely to change during (Editor) runtime. However, I have added a method that flushes the instance cache (See ClearCachedEditors) when called. When a node graph is opened, it is called, and the cache is cleared. This way the required editors for the graph are instantiated, and their editors' OnEnable methods are called.

This is my first every PR on a public repo, I have to admit I'm a little nervous :) I did my best to follow the contribution guidelines, but I saw that NodeEditorBase is indented using tabs. I figured I'd leave it alone...

@Siccity
Copy link
Owner

Siccity commented Apr 12, 2020

Looks fine, just a few thoughts:

  • Your ClearCachedEditors doesn't seem to be destroying editors. By dereferencing their dictionary, are they destroyed automatically? Or do they persist in memory. Perhaps it's worth to destroy them before clearing the dict.
  • Would calling dict.Clear() be a better alternative than creating a new dict?
  • What happens if i have one graph open, and open an additional one? Will the first graph have all its node editors cleared and thereby call OnEnable again for all editors, but in new editor instances? This might cause issues for people who have set data in their editor scripts and then open a new graph side-by-side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants