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

XML not saving node values #184

Closed JMFrancia closed 5 years ago

JMFrancia commented 5 years ago

Hello,

One of the features I'm most interested in for Node Editor is its ability to import and export in XML format. While XML does appear to save some aspects of the node graph, including node positions and their connections to one another, it makes no apparent attempt to save the current values of the nodes.

So for instance using the calculation canvas that comes as an example: if you create a simple graph adding 5 and 5 together to make 10, save export the graph as XML, and then re-import it, the values of the float nodes will be their default values of 1.

This is unlike the normal save/load mode (directly to a .asset file) which does save the node values.

Is this intended behavior, and if so, are there any plans to make the XML export more robust? Thank you for your time on this great tool.

Seneral commented 5 years ago

No, that is definitely not intended behaviour - it's been a while though since I implemented it, so I'll have to take a look tomorrow. Thanks for reporting

Seneral commented 5 years ago

FYI here is the relevant implementation

JMFrancia commented 5 years ago

Thank you very much, I'm taking a look into it as well.

The main thing I'm confronting is that my nodes have several drop-down menus in the form of either enums (using RTEditorGUI.EnumPopup(Enum val)) or strings (using RTEditorGUI.Popup(string[], index). I'll set these drop downs to some value or another, export to XML, re-import and the values will not have been saved.

Based on the implementation of ReflectionUtility.getSerializedFields() I thought perhaps the issue was that I was not marking the popup index vals as serialized. However I just ran a test by marking those values as serialized before exporting and it doesn't seem to make a difference.

JMFrancia commented 5 years ago

Found the issue. On the XML export process, obj.GetType().GetField(fieldName) is failing to find every field that is NOT a string. It fails to find int field, fails to find bool field, fails to find enum field: https://github.com/Seneral/Node_Editor_Framework/blob/develop/Node_Editor/Default/IO%20Formats/XMLImportExport.cs#L308

Seneral commented 5 years ago

Ok so just tested myself on windows, Unity 2017.4.24f1 and it works fine. If you can tell me your version I'll test that as well, might be some changes broke things. Am I right to assume that the coded warning popped up or an error? Edit: Found a bug where it would fail to find the variable when it's private but with [SerializeField] - XML-IO assumes all serializable variables to be public. Could that be related, although you also said it failed in the default example?

JMFrancia commented 5 years ago

I was just coming here to mention that if I make the fields public it works correctly. I was surprised to find that [SerializeField] did not have the same effect. I also tried passing BindingsFlags.NonPublic but that did not help either. Only using Public has worked so far.

And yes, I also saw the fields not saving in the calc. graph. I'm using 2019.1.13f1

Seneral commented 5 years ago

Just noticed it isn't even meant to refetch the variable value at all - that is done during creation of the structured data beforehand. So just change this to:

else // Serialize value-type fields in-line
{
    XmlElement serializedValue = SerializeObjectToXML(node, varData.value);
    serializedValue.SetAttribute("name", varData.name);
}

That should work (only for export, error will still appear on import) - although I'm still not sure what causes the error for you on the default nodes... Maybe they made the BindingFlags necessarily explicit on newer versions? Not really up to date with unity versions anymore unfortunately

Seneral commented 5 years ago

With 1bbb287 private [SerializeField] fields work for me now. Can you check if that fixes your issue with the CaluclationCanvas as well? Will check in 2019 now

Seneral commented 5 years ago

Ok finally got around to try in 2019.1. I'm still not getting the field not found error on line 308 - but a serialization error in line 363. Seems the way XMLSerializer works changed with Net 4.x, it will now always call a method that will throw an error even though it knows it would throw an error... Workaround added in 19f0059 Does that fix it for you now? Works fine for me in 2019.1 now

JMFrancia commented 5 years ago

That solved it! Excellent work. Thank you for being so responsive.