Skip to content
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

Implemented #242 - Added NodeEditor.OnEnable() #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implemented #242 - Added NodeEditor.OnEnable() #243

wants to merge 1 commit into from

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