MAYA-127095 remove usage of macros leading to fatal exits

We should strive to reduce the number of scenarios under which Maya crashes. Instead, when possible, we should report an error to user and abort the operation. MayaUSD has extensive undo/redo capabilities now and code is able to undo partially done operations. Given this, errors should be reported and propagated back. The macros TF_AXIOM and TF_FATAL_ERROR cause crashes when their conditions are not met, even in release builds. Remove TF_AXIOM and TF_FATAL_ERROR, replacing them with: - TF_WARN when the situation was not an error but an unusual situation. - TF_VERIFY when it still warrant an error message to the user, for example during error handling. - Simple if/else, with an error return value when not a hard error. - Simple if when the condition was trivial, like an empty node when iterating over USD nodes. - In a few cases, TF_DEV_AXIOM when the condition was really strictly enforced by a class constructor. - Entirely removed, when the code was already dealing with the error. - C++ exception when the error needs to propagate to caller, in particular in constructors.
Autodesk · May 11, 2023 · e92d7bb · e92d7bb
1 parent 65707a8
commit e92d7bb
Show file tree

Hide file tree

Showing 28 changed files with 199 additions and 139 deletions.
diff --git a/doc/codingGuidelines.md b/doc/codingGuidelines.md
@@ -165,7 +165,7 @@ In general, macros should be avoided (see [Modern C++](https://docs.google.com/d
 inline PXR_NS::UsdPrim prim() const
 {
     PXR_NAMESPACE_USING_DIRECTIVE
-    TF_AXIOM(fItem != nullptr);
+    TF_VERIFY(fItem != nullptr);
     return fItem->prim();
 }
 ```

diff --git a/lib/mayaUsd/fileio/orphanedNodesManager.cpp b/lib/mayaUsd/fileio/orphanedNodesManager.cpp
@@ -174,12 +174,19 @@ OrphanedNodesManager::OrphanedNodesManager()
 
 void OrphanedNodesManager::add(const Ufe::Path& pulledPath, const MDagPath& editedAsMayaRoot)
 {
+    // Adding a node twice to the orphan manager is idem-potent. The manager was already
+    // racking that node.
+    if (pulledPrims().containsDescendantInclusive(pulledPath))
+        return;
+
     // Add the edited-as-Maya root to our pulled prims prefix tree.  Also add the full
     // configuration of variant set selections for each ancestor, up to the USD
     // pseudo-root.  Variants on the pulled path itself are ignored, as once
     // pulled into Maya they cannot be changed.
-    TF_AXIOM(!pulledPrims().containsDescendantInclusive(pulledPath));
-    TF_AXIOM(pulledPath.runTimeId() == MayaUsd::ufe::getUsdRunTimeId());
+    if (pulledPath.runTimeId() != MayaUsd::ufe::getUsdRunTimeId()) {
+        TF_WARN("Trying to monitor a non-USD node for edit-as-Maya orphaning.");
+        return;
+    }
 
     // We store a list of (path, list of (variant set, variant set selection)),
     // for all ancestors, starting at closest ancestor.
@@ -192,7 +199,7 @@ void OrphanedNodesManager::add(const Ufe::Path& pulledPath, const MDagPath& edit
 OrphanedNodesManager::Memento OrphanedNodesManager::remove(const Ufe::Path& pulledPath)
 {
     Memento oldPulledPrims(preserve());
-    TF_AXIOM(pulledPrims().remove(pulledPath) != nullptr);
+    TF_VERIFY(pulledPrims().remove(pulledPath) != nullptr);
     return oldPulledPrims;
 }
 
@@ -323,7 +330,7 @@ void OrphanedNodesManager::handleOp(const Ufe::SceneCompositeNotification::Op& o
                 // USD sends resync changes (UFE subtree invalidate) on the
                 // pseudo-root itself.  Since the pseudo-root has no payload or
                 // variant, ignore these.
-                TF_AXIOM(parentItem->path().nbSegments() == 1);
+                TF_VERIFY(parentItem->path().nbSegments() == 1);
                 return;
             }
             auto parentPrim = parentUsdItem->prim();

diff --git a/lib/mayaUsd/fileio/primUpdater.cpp b/lib/mayaUsd/fileio/primUpdater.cpp
@@ -71,7 +71,9 @@ bool UsdMayaPrimUpdater::canEditAsMaya() const
     // prim will round-trip back through export, so we do not check for
     // exporter (to USD) capability.
     auto prim = MayaUsd::ufe::ufePathToPrim(_path);
-    TF_AXIOM(prim);
+    // Invalid prim cannot be edited.
+    if (!prim)
+        return false;
     return (UsdMayaPrimReaderRegistry::Find(prim.GetTypeName()) != nullptr);
 }
 

diff --git a/lib/mayaUsd/fileio/primUpdaterManager.cpp b/lib/mayaUsd/fileio/primUpdaterManager.cpp
@@ -239,7 +239,7 @@ bool allowTopologyModifications(MDagPath& root)
 // prim as the root of the USD hierarchy to be pulled.  The UFE path and
 // the prim refer to the same object: the prim is passed in as an
 // optimization to avoid an additional call to ufePathToPrim().
-using PullImportPaths = std::pair<std::vector<MDagPath>, std::vector<Ufe::Path>>;
+using PullImportPaths = std::vector<std::pair<MDagPath, Ufe::Path>>;
 PullImportPaths pullImport(
     const Ufe::Path&                 ufePulledPath,
     const UsdPrim&                   pulledPrim,
@@ -282,14 +282,13 @@ PullImportPaths pullImport(
     }
     progressBar.advance();
 
-    std::vector<MDagPath>  addedDagPaths;
-    std::vector<Ufe::Path> pulledUfePaths;
+    std::vector<MDagPath> addedDagPaths;
 
     // Execute the command, which can succeed but import nothing.
     bool success = readJob->Read(&addedDagPaths);
     if (!success || addedDagPaths.size() == 0) {
         TF_WARN("Nothing to edit in the selection.");
-        return PullImportPaths({}, {});
+        return PullImportPaths();
     }
     progressBar.advance();
 
@@ -423,17 +422,18 @@ PullImportPaths pullImport(
     progressBar.advance();
 
     // For each added Dag path, get the UFE path of the pulled USD prim.
-    pulledUfePaths.reserve(addedDagPaths.size());
+    PullImportPaths pulledPaths;
+    pulledPaths.reserve(addedDagPaths.size());
     for (const auto& dagPath : addedDagPaths) {
         auto found = objToUfePath.find(MObjectHandle(dagPath.node()));
-        TF_AXIOM(found != objToUfePath.end());
-        pulledUfePaths.emplace_back(found->second);
+        if (TF_VERIFY(found != objToUfePath.end())) {
+            pulledPaths.emplace_back(std::make_pair(dagPath, found->second));
+        }
     }
     progressBar.advance();
 
-    auto ret = PullImportPaths(addedDagPaths, pulledUfePaths);
     progressBar.advance();
-    return ret;
+    return pulledPaths;
 }
 
 //------------------------------------------------------------------------------
@@ -460,18 +460,15 @@ bool pullCustomize(const PullImportPaths& importedPaths, const UsdMayaPrimUpdate
 {
     // The number of imported paths should (hopefully) never be so great
     // as to overwhelm the computation with progress bar updates.
-    MayaUsd::ProgressBarScope progressBar(importedPaths.first.size());
+    MayaUsd::ProgressBarScope progressBar(importedPaths.size());
 
     // Record all USD modifications in an undo block and item.
     UsdUndoBlock undoBlock(
         &UsdUndoableItemUndoItem::create("Pull customize USD data modifications"));
 
-    TF_AXIOM(importedPaths.first.size() == importedPaths.second.size());
-    auto dagPathIt = importedPaths.first.begin();
-    auto ufePathIt = importedPaths.second.begin();
-    for (; dagPathIt != importedPaths.first.end(); ++dagPathIt, ++ufePathIt) {
-        const auto&       dagPath = *dagPathIt;
-        const auto&       pulledUfePath = *ufePathIt;
+    for (const auto& importedPair : importedPaths) {
+        const auto&       dagPath = importedPair.first;
+        const auto&       pulledUfePath = importedPair.second;
         MFnDependencyNode dgNodeFn(dagPath.node());
 
         auto registryItem = getUpdaterItem(dgNodeFn);
@@ -1156,7 +1153,7 @@ bool PrimUpdaterManager::editAsMaya(const Ufe::Path& path, const VtDictionary& u
 
     // 1) Perform the import
     PullImportPaths importedPaths = pullImport(path, pulledPrim, context);
-    if (importedPaths.first.empty()) {
+    if (importedPaths.empty()) {
         return false;
     }
     progressBar.advance();
@@ -1170,7 +1167,7 @@ bool PrimUpdaterManager::editAsMaya(const Ufe::Path& path, const VtDictionary& u
 
 #ifdef HAS_ORPHANED_NODES_MANAGER
     if (_orphanedNodesManager) {
-        RecordPullVariantInfoUndoItem::execute(_orphanedNodesManager, path, importedPaths.first[0]);
+        RecordPullVariantInfoUndoItem::execute(_orphanedNodesManager, path, importedPaths[0].first);
     }
 #endif
 
@@ -1498,7 +1495,6 @@ bool PrimUpdaterManager::duplicate(
     }
     // Copy from DG to USD
     else if (srcProxyShape == nullptr && dstProxyShape) {
-        TF_AXIOM(srcPath.nbSegments() == 1);
         MDagPath dagPath = PXR_NS::UsdMayaUtil::nameToDagPath(Ufe::PathString::string(srcPath));
         if (!dagPath.isValid()) {
             return false;

diff --git a/lib/mayaUsd/ufe/UsdCamera.h b/lib/mayaUsd/ufe/UsdCamera.h
@@ -51,8 +51,10 @@ class MAYAUSD_CORE_PUBLIC UsdCamera : public Ufe::Camera
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
     // Ufe::Camera overrides

diff --git a/lib/mayaUsd/ufe/UsdConnections.cpp b/lib/mayaUsd/ufe/UsdConnections.cpp
@@ -41,9 +41,8 @@ UsdConnections::UsdConnections(const Ufe::SceneItem::Ptr& item)
     , _sceneItem(std::dynamic_pointer_cast<UsdSceneItem>(item))
 #endif
 {
-    if (!TF_VERIFY(_sceneItem)) {
-        TF_FATAL_ERROR("Invalid scene item.");
-    }
+    // Note: TF_VERIFY emits a TF_CODING_ERROR.
+    TF_VERIFY(_sceneItem);
 }
 
 UsdConnections::~UsdConnections() { }
@@ -55,7 +54,8 @@ UsdConnections::Ptr UsdConnections::create(const Ufe::SceneItem::Ptr& item)
 
 std::vector<Ufe::Connection::Ptr> UsdConnections::allConnections() const
 {
-    TF_AXIOM(_sceneItem);
+    if (!TF_VERIFY(_sceneItem))
+        return {};
 
     std::vector<Ufe::Connection::Ptr> result;
 

diff --git a/lib/mayaUsd/ufe/UsdContextOps.cpp b/lib/mayaUsd/ufe/UsdContextOps.cpp
@@ -1172,7 +1172,8 @@ Ufe::UndoableCommand::Ptr UsdContextOps::doOpCmd(const ItemPath& itemPath)
     } // Variant sets
     else if (itemPath[0] == kUSDToggleVisibilityItem) {
         auto object3d = UsdObject3d::create(fItem);
-        TF_AXIOM(object3d);
+        if (!TF_VERIFY(object3d))
+            return nullptr;
         auto current = object3d->visibility();
         return object3d->setVisibleCmd(!current);
     } // Visibility

diff --git a/lib/mayaUsd/ufe/UsdContextOps.h b/lib/mayaUsd/ufe/UsdContextOps.h
@@ -66,8 +66,10 @@ class MAYAUSD_CORE_PUBLIC UsdContextOps : public Ufe::ContextOps
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
     // When we are created from the ProxyShapeContextOpsHandler we do not have the proper

diff --git a/lib/mayaUsd/ufe/UsdHierarchy.h b/lib/mayaUsd/ufe/UsdHierarchy.h
@@ -53,8 +53,10 @@ class MAYAUSD_CORE_PUBLIC UsdHierarchy : public Ufe::Hierarchy
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
     UsdSceneItem::Ptr usdSceneItem() const;

diff --git a/lib/mayaUsd/ufe/UsdLight.h b/lib/mayaUsd/ufe/UsdLight.h
@@ -49,8 +49,10 @@ class MAYAUSD_CORE_PUBLIC UsdLight : public Ufe::Light
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
     // Ufe::Light overrides

diff --git a/lib/mayaUsd/ufe/UsdPathMappingHandler.cpp b/lib/mayaUsd/ufe/UsdPathMappingHandler.cpp
@@ -109,7 +109,7 @@ Ufe::Path UsdPathMappingHandler::toHost(const Ufe::Path& runTimePath) const { re
 
 Ufe::Path UsdPathMappingHandler::fromHost(const Ufe::Path& hostPath) const
 {
-    TF_AXIOM(hostPath.nbSegments() == 1);
+    TF_VERIFY(hostPath.nbSegments() == 1);
 
     // First look in our cache to see if we've already computed a mapping for the input.
     auto found = fromHostCache.find(hostPath);
@@ -134,7 +134,9 @@ Ufe::Path UsdPathMappingHandler::fromHost(const Ufe::Path& hostPath) const
     Ufe::Path                    mayaMappedPath;
     Ufe::PathSegment::Components mayaComps;
     while (dagPath.isValid() && (dagPath.length() > 0)) {
-        TF_AXIOM(!mayaHostPath.empty());
+        if (!TF_VERIFY(!mayaHostPath.empty())) {
+            return {};
+        }
         mayaComps.emplace_back(mayaHostPath.back());
         mayaHostPath = mayaHostPath.pop();
         Ufe::Path ufePath;
@@ -143,7 +145,9 @@ Ufe::Path UsdPathMappingHandler::fromHost(const Ufe::Path& hostPath) const
             // append the Maya component array.
             std::reverse(mayaComps.begin(), mayaComps.end());
 
-            TF_AXIOM(ufePath.nbSegments() == 2);
+            if (!TF_VERIFY(ufePath.nbSegments() == 2)) {
+                return {};
+            }
             const auto& usdSegment = ufePath.getSegments()[1];
             mayaMappedPath = ufePath.popSegment() + replaceLastComponent(usdSegment, mayaComps);
 

diff --git a/lib/mayaUsd/ufe/UsdRotatePivotTranslateUndoableCommand.h b/lib/mayaUsd/ufe/UsdRotatePivotTranslateUndoableCommand.h
@@ -75,8 +75,10 @@ class MAYAUSD_CORE_PUBLIC UsdRotatePivotTranslateUndoableCommand
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
 private:

diff --git a/lib/mayaUsd/ufe/UsdSceneItemOps.h b/lib/mayaUsd/ufe/UsdSceneItemOps.h
@@ -49,8 +49,10 @@ class MAYAUSD_CORE_PUBLIC UsdSceneItemOps : public Ufe::SceneItemOps
     inline PXR_NS::UsdPrim prim() const
     {
         PXR_NAMESPACE_USING_DIRECTIVE
-        TF_AXIOM(fItem != nullptr);
-        return fItem->prim();
+        if (TF_VERIFY(fItem != nullptr))
+            return fItem->prim();
+        else
+            return PXR_NS::UsdPrim();
     }
 
     //@{

diff --git a/lib/mayaUsd/ufe/UsdShaderAttributeDef.cpp b/lib/mayaUsd/ufe/UsdShaderAttributeDef.cpp
@@ -47,27 +47,27 @@ UsdShaderAttributeDef::~UsdShaderAttributeDef() { }
 
 std::string UsdShaderAttributeDef::name() const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     return fShaderAttributeDef->GetName().GetString();
 }
 
 std::string UsdShaderAttributeDef::type() const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     return usdTypeToUfe(fShaderAttributeDef);
 }
 
 std::string UsdShaderAttributeDef::defaultValue() const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     std::ostringstream defaultValue;
     defaultValue << fShaderAttributeDef->GetDefaultValue();
     return defaultValue.str();
 }
 
 Ufe::AttributeDef::IOType UsdShaderAttributeDef::ioType() const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     return fShaderAttributeDef->IsOutput() ? Ufe::AttributeDef::OUTPUT_ATTR
                                            : Ufe::AttributeDef::INPUT_ATTR;
 }
@@ -131,7 +131,7 @@ static const MetadataMap _metaMap = {
 
 Ufe::Value UsdShaderAttributeDef::getMetadata(const std::string& key) const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     const NdrTokenMap& metadata = fShaderAttributeDef->GetMetadata();
     auto               it = metadata.find(TfToken(key));
     if (it != metadata.cend()) {
@@ -154,7 +154,7 @@ Ufe::Value UsdShaderAttributeDef::getMetadata(const std::string& key) const
 
 bool UsdShaderAttributeDef::hasMetadata(const std::string& key) const
 {
-    TF_AXIOM(fShaderAttributeDef);
+    TF_DEV_AXIOM(fShaderAttributeDef);
     const NdrTokenMap& metadata = fShaderAttributeDef->GetMetadata();
     auto               it = metadata.find(TfToken(key));
     if (it != metadata.cend()) {

diff --git a/lib/mayaUsd/ufe/UsdShaderAttributeHolder.cpp b/lib/mayaUsd/ufe/UsdShaderAttributeHolder.cpp
@@ -52,7 +52,9 @@ UsdShaderAttributeHolder::UsdShaderAttributeHolder(
 
     // sdrProp must be valid at creation and will stay valid.
     PXR_NAMESPACE_USING_DIRECTIVE
-    TF_AXIOM(sdrProp && sdrType != PXR_NS::UsdShadeAttributeType::Invalid);
+    if (!TF_VERIFY(sdrProp && sdrType != PXR_NS::UsdShadeAttributeType::Invalid)) {
+        throw std::runtime_error("Invalid shader attribute holder");
+    }
 }
 
 UsdAttributeHolder::UPtr UsdShaderAttributeHolder::create(