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 415 forks source link

Node's OnDelete is not called properly #104

Closed CanBaycay closed 7 years ago

CanBaycay commented 7 years ago

I'm writing a custom node and noticed that overridden OnDelete method is not called when I delete the node. So I tracked down and found out that "node.OnDelete();" in IssueOnDeleteNode method is written inside in Callback Receivers loop.

    public static void IssueOnDeleteNode (Node node) 
    {
        if (OnDeleteNode != null)
            OnDeleteNode.Invoke (node);
        for (int cnt = 0; cnt < receiverCount; cnt++) 
        {
            if (callbackReceiver [cnt] == null)
                callbackReceiver.RemoveAt (cnt--);
            else 
            {
                callbackReceiver [cnt].OnDeleteNode (node);
                node.OnDelete (); // WRONG LINE
            }
        }
    }

This way node's OnDelete called for every Callback Receiver, which results it does not even called if there is no Callback Receiver or called more than once if there are multiple Callback Receivers.

I'm a newcomer and I don't exactly know if this was written intentionally. So please correct me if I'm wrong. I guess the code should be something like:

    public static void IssueOnDeleteNode (Node node) 
    {
        if (OnDeleteNode != null)
            OnDeleteNode.Invoke (node);
        for (int cnt = 0; cnt < receiverCount; cnt++) 
        {
            if (callbackReceiver [cnt] == null)
                callbackReceiver.RemoveAt (cnt--);
            else 
            {
                callbackReceiver [cnt].OnDeleteNode (node);
            }
        }
        node.OnDelete(); // CORRECTED LINE
    }
Seneral commented 7 years ago

Yep you're right, I'm very sorry for that mistake! I'm going to make a fix and commit. Thanks!

Seneral commented 7 years ago

Sorry, was mixing up branches. Now the fix is on both master and develop...