Seneral / Node_Editor_Framework

A flexible and modular Node Editor Framework for creating node based displays and editors in Unity
https://nodeeditor.seneral.dev
MIT License
2.01k stars 414 forks source link

Wierd behaviour of Nodetypes.FetchNode(); #101

Closed No3371 closed 7 years ago

No3371 commented 7 years ago

I'm creating a custom dialog system, made a canvas and all nodes, then if I reopened unity with my nodes inside the project(importing them back after reopen is ok), Node Editor will fail to load (stay grey) and stop at FetchNodes(): node = node.create(Vector2.zero) and keep saying "NullReferenceException: Object reference not set to an instance of an object".

I spent several hours to test by inserting some Debug.Log() to FetchNodes(), and it seems that FetchNodes() refuse to accept SystemControlNode.

The order of the loop in the FetchNodes() are [several other nodes], SystemConditionalNode, then SystemControlNode, the loop will stop at SystemControlNode's node = node.create(Vector2.zero) and does not even go into SystemControlNode.Create(Vector2 pos).

So I gave it a try to change class name of SystemControlNode into "ControlNode" and bang! It works, pretty unexpected.

send.zip

Seneral commented 7 years ago

Sorry for the late response, was on a class trip last week.

It seems I cannot reproduce this error. Which system&unity version are you using?

And you say SystemConditionalNode does work? My first guess was that ScriptableObject.CreateInstance would not accept any class starting with "System", but if the other node works, I don't know. Node node = ScriptableObject.CreateInstance (type.Name) as Node; Can you validate it's that line? Also, can you replace it with

ScriptableObject soNode = ScriptableObject.CreateInstance (type.Name);
Node node = soNode as Node;

and check if soNode is null?

A different idea why this would not work is that if you have a different class in you project that is named SystemControlNode or similarily, not necessarily in the same namespace, just because only the name is passed, not the assembly information. Try to rename to SystemControlNodeTwo, maybe that will work:)

But I hope I can reproduce it eventually...

No3371 commented 7 years ago

I did have a SystemControlNode in another namespace since I use Node Editor as a creation tool, then convert the canvas and nodes into another set of classes my dialog system use.

But, I did also have a SystemConditionalNode in the namespace, so it's just so weird. And no, I don't think it's ScriptableObject.CreateInstance()'s fault, since during the test I put a Debug.Log() at first line in the SystemControlNode.Create(), but It's not even executed.

Win10 / Unity5.4.0f3

Currently everything is working fine after I rename SystemControlNode to ControlNode, if a test with the original SystemControlNode script will help, I'am willing to do that when I have some free time :)

No3371 commented 7 years ago

It was like:

Node node = ScriptableObject.CreateInstance (type.Name) as Node; // Create a 'raw' instance (not setup using the appropriate Create function)

Debug.Log("Log Me!!");

node = node.Create (Vector2.zero); // From that, call the appropriate Create Method to init the previously 'raw' instance
nodes.Add (node, new NodeData (attr == null? node.name : attr.contextText, attr.limitToCanvasTypes));

And in SystemControlNode.Create():

Debug.Log("We are now in Create()...");
SystemControlNode node = CreateInstance<SystemControlNode>();

Then only "Log Me!!!" is logged.

It's really just so weird that I can't come up with any possibility it's Node Editor's problem , though.

Seneral commented 7 years ago

From the last post it does suggest it's ScriptableObject.CreateInstance's fault because if that fails and returns Null, create wouldn't be called. Also, you said a NullRef exceptions would be thrown, so that makes sense...

No3371 commented 7 years ago

Ahh, you are right, I was too reckless. But it still bothers me why SystemConditionalNode could be created successfully... I'm going to try the steps you mentioned anyway, tomorrow maybe... then I'll let you know the result.

Seneral commented 7 years ago

Ok, that'd be great. Maybe, can you send me the whole project or atleast those files that certainly reproduce the problem? I'll also try the same setup with exactly the same name of classes in different namespaces and take a look. Maybe it works with SystemConditionalNode because the order in wich is was read by the compiler is with the correct one first, whereas with SystemControlNode the class in the other assembly was first?

Seneral commented 7 years ago

One question, are the classes in the other namespace of the type 'Node'? I'd assume no because of what you said of NodeEditor being a creational tool, but then why naming them '*Node' in the first place? Well if they are nodes, then the system can only target the type which ScriptableObject.CreateInstance creates an instance of when the name is passed... That means even if they have different IDs, the same node with the same ID is recorded for both and an error is thrown when trying to open the canvas context menu to create nodes as it tries to uniquely get one node of that ID.

If they are not of type node or any other scriptable object type, then ScriptableObject.CreateInstance does always create the correct type and that is not the point of failure...

No3371 commented 7 years ago

They are not NodeEditorFramework.Node, but they do derived from ScriptableObject.

I'll send you my DialogSystem, but note that only DialogSystem.Data is relevant to the problem (the other parts are not even finished and used).

BTW I just copied the original SystemControlNode back to my project and opened NodeEditor, same thing happened, here's the stack trace:

NullReferenceException: Object reference not set to an instance of an object
NodeEditorFramework.NodeTypes.FetchNodes () (at Assets/Plugins/Node_Editor/Framework/NodeTypes.cs:35)
NodeEditorFramework.NodeEditor.setupBaseFramework () (at Assets/Plugins/Node_Editor/Framework/NodeEditor.cs:76)
NodeEditorFramework.NodeEditor.checkInit (Boolean GUIFunction) (at Assets/Plugins/Node_Editor/Framework/NodeEditor.cs:46)
NodeEditorFramework.Standard.NodeEditorWindow.OnGUI () (at Assets/Plugins/Editor/Node_Editor/NodeEditorWindow.cs:126)
System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) (at /Users/builduser/buildslave/mono/build/mcs/class/corlib/System.Reflection/MonoMethod.cs:222)

I'll upload the files later.

No3371 commented 7 years ago

Plugins.zip

My goal is to create usable dialog system ASAP, and to maintain the independence of the data ( for possibility I want to learn creating a dedicated dialog editor by myself), I use DialogSystem.Data.Converter to convert DialogTreeCanvas to DialogTree.

BUT Converter is not related to the issue either, because it's only called by a button in NodeEditorWindow. ( I modified NodeEditorWindow a bit to keep it simple for my team members which are new to Unity)

p.s. The Control Node (the renamed one) , SystemControlNode (which is causing problem) and all nodes I use in Node Editor are located at DialogEditor/Core/Nodes.

No3371 commented 7 years ago

Oh and all the stuff in DialogEditor/Core/Source are not used, they're there just because I started by studying the Dialog Editor Example in the branches.

No3371 commented 7 years ago

It seems that renaming SystemControlNode to SystemControlNodeTwo does not work.

Combined with the fact that renaming to ControlNode solved my problem, I doubt it's possibly related to class name, especially "System" prefix? Don't know.

Seneral commented 7 years ago

So SystemControlNodeTwostill does throw the error? I have yet to test your files, but as the other classes are scriptableObjects, too, that seems to be causing the issue for some classes as I said in my last post... ScriptableObject.CreateInstance simply doesn't know by which class to instantiate when both have the exact same name and both derive from ScriptableObject, and sometimes mixes them up. In that case, the conversion as Node fails and silently returns Null, which causes the error. Everything logic or did I miss something? I have some things in mind that could fix that;)

Seneral commented 7 years ago

Ok, got it, change line Node node = ScriptableObject.CreateInstance (type.Name) as Node; to Node node = (Node)ScriptableObject.CreateInstance (type); Then it's exactly that type which we wanted, no confusion with names and also throws an error incase it still fails. Don't know why I didn't used that overload before though...

Anyway, I'll create a commit now:)