fix issue #568

Restoring links is now done in two passes: 1. Save the links during deserialization in the KnobI 2. Restore the links once all nodes were created
NatronGitHub · Apr 3, 2021 · f369197 · f369197
1 parent b9b2cda
commit f369197
Show file tree

Hide file tree

Showing 22 changed files with 336 additions and 129 deletions.
diff --git a/Engine/EffectInstance.cpp b/Engine/EffectInstance.cpp
@@ -553,6 +553,12 @@ EffectInstance::getScriptName_mt_safe() const
     return getNode()->getScriptName_mt_safe();
 }
 
+std::string
+EffectInstance::getFullyQualifiedName() const
+{
+    return getNode()->getFullyQualifiedName();
+}
+
 int
 EffectInstance::getRenderViewsCount() const
 {

diff --git a/Engine/EffectInstance.h b/Engine/EffectInstance.h
@@ -291,6 +291,7 @@ GCC_DIAG_SUGGEST_OVERRIDE_ON
      **/
     const std::string & getScriptName() const WARN_UNUSED_RETURN;
     virtual std::string getScriptName_mt_safe() const OVERRIDE FINAL WARN_UNUSED_RETURN;
+    virtual std::string getFullyQualifiedName() const OVERRIDE FINAL WARN_UNUSED_RETURN;
     virtual void onScriptNameChanged(const std::string& /*fullyQualifiedName*/) {}
 
     /**

diff --git a/Engine/Knob.cpp b/Engine/Knob.cpp
@@ -56,6 +56,8 @@
 #include "Engine/StringAnimationManager.h"
 #include "Engine/TLSHolder.h"
 #include "Engine/TimeLine.h"
+#include "Engine/TrackMarker.h"
+#include "Engine/TrackerContext.h"
 #include "Engine/Transform.h"
 #include "Engine/ViewIdx.h"
 #include "Engine/ViewerInstance.h"
@@ -149,6 +151,125 @@ KnobI::slaveTo(int dimension,
     return slaveToInternal(dimension, other, otherDimension, eValueChangedReasonNatronInternalEdited, ignoreMasterPersistence);
 }
 
+static KnobIPtr
+findMaster(const KnobIPtr & knob,
+           const NodesList & allNodes,
+           const std::string& masterKnobName,
+           const std::string& masterNodeName,
+           const std::string& masterTrackName,
+           const std::map<std::string, std::string>& oldNewScriptNamesMapping)
+{
+    ///we need to cycle through all the nodes of the project to find the real master
+    NodePtr masterNode;
+    std::string masterNodeNameToFind = masterNodeName;
+
+    qDebug() << "Link slave/master for" << knob->getName().c_str() << "restoring linkage" << masterNodeNameToFind.c_str()  << '/' << masterTrackName.c_str() << '/' << masterKnobName.c_str();
+
+    /*
+       When copy pasting, the new node copied has a script-name different from what is inside the serialization because 2
+       nodes cannot co-exist with the same script-name. We keep in the map the script-names mapping
+     */
+    std::map<std::string, std::string>::const_iterator foundMapping = oldNewScriptNamesMapping.find(masterNodeName);
+
+    if ( foundMapping != oldNewScriptNamesMapping.end() ) {
+        masterNodeNameToFind = foundMapping->second;
+    }
+
+    for (NodesList::const_iterator it2 = allNodes.begin(); it2 != allNodes.end(); ++it2) {
+        qDebug() << "Trying node" << (*it2)->getScriptName().c_str() << '/' << (*it2)->getFullyQualifiedName().c_str();
+        if ( (*it2)->getFullyQualifiedName() == masterNodeNameToFind ) {
+            masterNode = *it2;
+            break;
+        }
+    }
+    if (!masterNode) {
+        qDebug() << "Link slave/master for" << knob->getName().c_str() << "could not find master node" << masterNodeNameToFind.c_str() << "trying without the group name (Natron project files before 2.3.16)...";
+
+        for (NodesList::const_iterator it2 = allNodes.begin(); it2 != allNodes.end(); ++it2) {
+            qDebug() << "Trying node" << (*it2)->getScriptName().c_str() << '/' << (*it2)->getFullyQualifiedName().c_str();
+            if ( (*it2)->getScriptName() == masterNodeNameToFind ) {
+                masterNode = *it2;
+                qDebug() << "Got node" << (*it2)->getFullyQualifiedName().c_str();
+                break;
+            }
+        }
+    }
+    if (!masterNode) {
+        qDebug() << "Link slave/master for" << knob->getName().c_str() << "could not find master node" << masterNodeNameToFind.c_str();
+
+        return KnobIPtr();
+    }
+
+    if ( !masterTrackName.empty() ) {
+        TrackerContextPtr context = masterNode->getTrackerContext();
+        if (context) {
+            TrackMarkerPtr masterTrack = context->getMarkerByName(masterTrackName);
+            if (!masterTrack) {
+                qDebug() << "Link slave/master for" << knob->getName().c_str() << "could not find track" << masterNodeNameToFind.c_str() << '/' << masterTrackName.c_str();
+            } else {
+                KnobIPtr masterKnob = masterTrack->getKnobByName(masterKnobName);
+                if (!masterKnob) {
+                    qDebug() << "Link slave/master for" << knob->getName().c_str() << "cound not find knob in track" << masterNodeNameToFind.c_str()  << '/' << masterTrackName.c_str() << '/' << masterKnobName.c_str();
+                }
+                return masterKnob;
+            }
+        }
+    } else {
+        ///now that we have the master node, find the corresponding knob
+        const std::vector<KnobIPtr> & otherKnobs = masterNode->getKnobs();
+        for (std::size_t j = 0; j < otherKnobs.size(); ++j) {
+            qDebug() << "Trying knob" << otherKnobs[j]->getName().c_str();
+            // The other knob doesn't need to be persistent (it may be a pushbutton)
+            if (otherKnobs[j]->getName() == masterKnobName) {
+                return otherKnobs[j];
+            }
+        }
+        qDebug() << "Link slave/master for" << knob->getName().c_str() << "cound not find knob" << masterNodeNameToFind.c_str()  << '/' << masterTrackName.c_str() << '/' << masterKnobName.c_str();
+    }
+
+    qDebug() << "Link slave/master for" << knob->getName().c_str() << "failed to restore linkage" << masterNodeNameToFind.c_str()  << '/' << masterTrackName.c_str() << '/' << masterKnobName.c_str();
+
+    return KnobIPtr();
+}
+
+void
+KnobI::restoreLinks(const NodesList & allNodes,
+                    const std::map<std::string, std::string>& oldNewScriptNamesMapping,
+                    bool throwOnFailure)
+{
+    int i = 0;
+    KnobIPtr thisKnob = shared_from_this();
+    for (std::vector<Link>::const_iterator l = _links.begin(); l != _links.end(); ++l) {
+
+        KnobIPtr linkedKnob = findMaster(thisKnob, allNodes, l->knobName, l->nodeName, l->trackName, oldNewScriptNamesMapping);
+        if (!linkedKnob) {
+            if (throwOnFailure) {
+                throw std::runtime_error(std::string("KnobI::restoreLinks(): Could not restore link to node/knob/track ") + l->nodeName + '/' + l->knobName + '/' + l->trackName);
+            } else {
+                continue;
+            }
+        }
+        if (linkedKnob == thisKnob) {
+            qDebug() << "Link for" << getName().c_str()
+            << "failed to restore the following linkage:"
+            << "node=" << l->nodeName.c_str()
+            << "knob=" << l->knobName.c_str()
+            << "track=" << l->trackName.c_str()
+            << "because it returned the same Knob. Probably a pre-2.3.16 project with Group clones";
+            if (throwOnFailure) {
+                throw std::runtime_error(std::string("KnobI::restoreLinks(): Could not restore link to node/knob/track ") + l->nodeName + '/' + l->knobName + '/' + l->trackName + " because it returned the same Knob. Probably a pre-2.3.16 project with Group clones.");
+            } else {
+                continue;
+            }
+       } else if (l->dimension == -1) {
+            setKnobAsAliasOfThis(linkedKnob, true);
+        } else {
+            slaveTo(i, linkedKnob, l->dimension);
+        }
+    }
+    _links.clear();
+}
+
 void
 KnobI::onKnobSlavedTo(int dimension,
                       const KnobIPtr &  other,

diff --git a/Engine/Knob.h b/Engine/Knob.h
@@ -1187,6 +1187,32 @@ class KnobI
      * @param ignoreMasterPersistence If true the master will not be serialized.
      **/
     bool slaveTo(int dimension, const KnobIPtr & other, int otherDimension, bool ignoreMasterPersistence = false);
+
+
+    void storeLink(int dimension,
+                      const std::string& knobName,
+                      const std::string& nodeName,
+                      const std::string& trackName)
+    {
+        _links.push_back(Link({dimension, knobName, nodeName, trackName}));
+    }
+
+    void restoreLinks(const NodesList & allNodes,
+                      const std::map<std::string, std::string>& oldNewScriptNamesMapping,
+                      bool throwOnFailure);
+
+private:
+    struct Link {
+        int dimension; // -1 means alias
+        std::string knobName;
+        std::string nodeName;
+        std::string trackName;
+    };
+
+    std::vector<Link> _links;
+
+public:
+
     virtual bool isMastersPersistenceIgnored() const = 0;
     virtual KnobIPtr createDuplicateOnHolder(KnobHolder* otherHolder,
                                             const boost::shared_ptr<KnobPage>& page,
@@ -2839,6 +2865,8 @@ class NamedKnobHolder
     }
 
     virtual std::string getScriptName_mt_safe() const = 0;
+
+    virtual std::string getFullyQualifiedName() const = 0;
 };
 
 

diff --git a/Engine/KnobSerialization.cpp b/Engine/KnobSerialization.cpp
@@ -129,10 +129,10 @@ ValueSerialization::initForSave(const KnobIPtr & knob,
         TrackMarker* isMarker = dynamic_cast<TrackMarker*>(holder);
         if (isMarker) {
             _master.masterTrackName = isMarker->getScriptName_mt_safe();
-            _master.masterNodeName = isMarker->getContext()->getNode()->getScriptName_mt_safe();
+            _master.masterNodeName = isMarker->getContext()->getNode()->getFullyQualifiedName();
         } else {
             // coverity[dead_error_line]
-            _master.masterNodeName = holder ? holder->getScriptName_mt_safe() : "";
+            _master.masterNodeName = holder ? holder->getFullyQualifiedName() : "";
         }
         _master.masterKnobName = m.second->getName();
     } else {
@@ -193,68 +193,9 @@ KnobSerialization::createKnob(const std::string & typeName,
     return ret;
 }
 
-static KnobIPtr
-findMaster(const KnobIPtr & knob,
-           const NodesList & allNodes,
-           const std::string& masterKnobName,
-           const std::string& masterNodeName,
-           const std::string& masterTrackName,
-           const std::map<std::string, std::string>& oldNewScriptNamesMapping)
-{
-    ///we need to cycle through all the nodes of the project to find the real master
-    NodePtr masterNode;
-    std::string masterNodeNameToFind = masterNodeName;
-
-    /*
-       When copy pasting, the new node copied has a script-name different from what is inside the serialization because 2
-       nodes cannot co-exist with the same script-name. We keep in the map the script-names mapping
-     */
-    std::map<std::string, std::string>::const_iterator foundMapping = oldNewScriptNamesMapping.find(masterNodeName);
-
-    if ( foundMapping != oldNewScriptNamesMapping.end() ) {
-        masterNodeNameToFind = foundMapping->second;
-    }
-
-    for (NodesList::const_iterator it2 = allNodes.begin(); it2 != allNodes.end(); ++it2) {
-        if ( (*it2)->getScriptName() == masterNodeNameToFind ) {
-            masterNode = *it2;
-            break;
-        }
-    }
-    if (!masterNode) {
-        qDebug() << "Link slave/master for " << knob->getName().c_str() <<   " failed to restore the following linkage: " << masterNodeNameToFind.c_str();
-
-        return KnobIPtr();
-    }
-
-    if ( !masterTrackName.empty() ) {
-        TrackerContextPtr context = masterNode->getTrackerContext();
-        if (context) {
-            TrackMarkerPtr marker = context->getMarkerByName(masterTrackName);
-            if (marker) {
-                return marker->getKnobByName(masterKnobName);
-            }
-        }
-    } else {
-        ///now that we have the master node, find the corresponding knob
-        const std::vector<KnobIPtr> & otherKnobs = masterNode->getKnobs();
-        for (std::size_t j = 0; j < otherKnobs.size(); ++j) {
-            if ( (otherKnobs[j]->getName() == masterKnobName) && otherKnobs[j]->getIsPersistent() ) {
-                return otherKnobs[j];
-                break;
-            }
-        }
-    }
-
-    qDebug() << "Link slave/master for " << knob->getName().c_str() <<   " failed to restore the following linkage: " << masterNodeNameToFind.c_str();
-
-    return KnobIPtr();
-}
 
 void
-KnobSerialization::restoreKnobLinks(const KnobIPtr & knob,
-                                    const NodesList & allNodes,
-                                    const std::map<std::string, std::string>& oldNewScriptNamesMapping)
+KnobSerialization::storeKnobLinks(const KnobIPtr & knob)
 {
     int i = 0;
 
@@ -266,24 +207,20 @@ KnobSerialization::restoreKnobLinks(const KnobIPtr & knob,
             const std::string& aliasKnobName = _masters.front().masterKnobName;
             const std::string& aliasNodeName = _masters.front().masterNodeName;
             const std::string& masterTrackName  = _masters.front().masterTrackName;
-            KnobIPtr alias = findMaster(knob, allNodes, aliasKnobName, aliasNodeName, masterTrackName, oldNewScriptNamesMapping);
-            if (alias) {
-                knob->setKnobAsAliasOfThis(alias, true);
-            }
+            knob->storeLink(-1, aliasKnobName, aliasNodeName, masterTrackName);
         }
     } else {
         for (std::list<MasterSerialization>::iterator it = _masters.begin(); it != _masters.end(); ++it) {
             if (it->masterDimension != -1) {
-                KnobIPtr master = findMaster(knob, allNodes, it->masterKnobName, it->masterNodeName, it->masterTrackName, oldNewScriptNamesMapping);
-                if (master) {
-                    knob->slaveTo(i, master, it->masterDimension);
-                }
+                knob->storeLink(it->masterDimension, it->masterKnobName, it->masterNodeName, it->masterTrackName);
             }
             ++i;
         }
     }
 }
 
+
+
 void
 KnobSerialization::restoreExpressions(const KnobIPtr & knob,
                                       const std::map<std::string, std::string>& oldNewScriptNamesMapping)

diff --git a/Engine/KnobSerialization.h b/Engine/KnobSerialization.h
@@ -928,11 +928,9 @@ class KnobSerialization
     ~KnobSerialization() { delete _extraData; }
 
     /**
-     * @brief This function cannot be called until all knobs of the project have been created.
+     * @brief Store the links, which can be restored once all Nodes and Knobs have been created, using KnobI::restoreLinks()
      **/
-    void restoreKnobLinks(const KnobIPtr & knob,
-                          const NodesList & allNodes,
-                          const std::map<std::string, std::string>& oldNewScriptNamesMapping);
+    void storeKnobLinks(const KnobIPtr & knob);
 
     /**
      * @brief This function cannot be called until all knobs of the project have been created.

diff --git a/Engine/Node.h b/Engine/Node.h
@@ -165,9 +165,13 @@ GCC_DIAG_SUGGEST_OVERRIDE_ON
 
     ///This cannot be done in loadKnobs as to call this all the nodes in the project must have
     ///been loaded first.
-    void restoreKnobsLinks(const NodeSerialization & serialization,
-                           const NodesList & allNodes,
-                           const std::map<std::string, std::string>& oldNewScriptNamesMapping);
+    void storeKnobsLinks(const NodeSerialization & serialization,
+                         const std::map<std::string, std::string>& oldNewScriptNamesMapping);
+
+    // Once all nodes are created, we can restore the links that were previously saved by storeKnobsLinks()
+    void restoreKnobsLinks(const NodesList & allNodes,
+                           const std::map<std::string, std::string>& oldNewScriptNamesMapping,
+                           bool throwOnFailure);
 
     void restoreUserKnobs(const NodeSerialization& serialization);
 

diff --git a/Engine/NodeGroupSerialization.cpp b/Engine/NodeGroupSerialization.cpp
@@ -395,19 +395,15 @@ NodeCollectionSerialization::restoreFromSerialization(const std::list<NodeSerial
     } // for (std::list<NodeSerializationPtr>::const_iterator it = serializedNodes.begin(); it != serializedNodes.end(); ++it) {
 
     ///Now that the graph is setup, restore expressions
-    NodesList nodes = group->getNodes();
-    if (isNodeGroup) {
-        nodes.push_back( isNodeGroup->getNode() );
-    }
-
     {
         std::map<std::string, std::string> oldNewScriptNamesMapping;
         for (std::map<NodePtr, NodeSerializationPtr>::const_iterator it = createdNodes.begin(); it != createdNodes.end(); ++it) {
             if ( appPTR->isBackground() && ( it->first->isEffectViewer() ) ) {
                 //ignore viewers on background mode
                 continue;
             }
-            it->first->restoreKnobsLinks(*it->second, nodes, oldNewScriptNamesMapping);
+            it->first->storeKnobsLinks(*it->second, oldNewScriptNamesMapping);
+            // restoreKnobsLinks() is called on all nodes at the end of ProjectPrivate::restoreFromSerialization()
         }
     }