From d0a29f88484c49378d1625d00ec9489b7400dc32 Mon Sep 17 00:00:00 2001 From: sunyab Date: Wed, 5 Apr 2017 22:52:51 -0700 Subject: [PATCH] Better handling of layer resolved path changes Calling PcpCache::Reload or ReloadReferences after a layer's resolved path had changed would cause any prims depending on asset paths relative to that resolved path to be recomposed. However, directly reloading the affected layer via SdfLayer::Reload did not trigger this recomposition. It does now. SdfLayer now registers a change when its resolved path has changed. Pcp uses this to detect when it needs to recompose affected prims as part of its normal change processing. This means the PcpCache methods above no longer need to handle this case specially and can be reverted to their original behavior. (Internal change: 1733305) --- pxr/usd/lib/pcp/cache.cpp | 76 +------------------ pxr/usd/lib/pcp/changes.cpp | 119 +++++++++++++++++++----------- pxr/usd/lib/pcp/changes.h | 12 +-- pxr/usd/lib/sdf/changeList.cpp | 8 ++ pxr/usd/lib/sdf/changeList.h | 2 + pxr/usd/lib/sdf/changeManager.cpp | 8 ++ pxr/usd/lib/sdf/changeManager.h | 1 + pxr/usd/lib/sdf/layer.cpp | 16 +++- 8 files changed, 115 insertions(+), 127 deletions(-) diff --git a/pxr/usd/lib/pcp/cache.cpp b/pxr/usd/lib/pcp/cache.cpp index 8c0ff5175d..faa8b06541 100644 --- a/pxr/usd/lib/pcp/cache.cpp +++ b/pxr/usd/lib/pcp/cache.cpp @@ -985,56 +985,6 @@ PcpCache::Apply(const PcpCacheChanges& changes, PcpLifeboat* lifeboat) _includedPayloads.insert(newIncludes.begin(), newIncludes.end()); } -// Scan given prim indexes and layer stacks and register significant -// changes for anything that needs to be recomputed due to asset path -// changes. This is potentially expensive, but the alternative -// is to store extra information when computing prim indexes, which -// would impact composition/load times and memory usage. -template -static void -_RegisterChangesForAssetPathChanges( - PcpCache* cache, PcpChanges* changes, - PrimIndexIter beginPrimIndex, PrimIndexIter endPrimIndex, - LayerStackIter beginLayerStack, LayerStackIter endLayerStack) -{ - tbb::concurrent_vector pathsToResync; - tbb::concurrent_vector layerStacksToResync; - - WorkDispatcher dispatcher; - dispatcher.Run( - [&pathsToResync, beginPrimIndex, endPrimIndex]() { - WorkParallelForEach( - beginPrimIndex, endPrimIndex, - [&pathsToResync](const std::pair& e) { - if (e.second.IsValid() && - Pcp_NeedToRecomputeDueToAssetPathChange(e.second)) { - pathsToResync.push_back(e.first); - } - }); - }); - - dispatcher.Run( - [&layerStacksToResync, beginLayerStack, endLayerStack]() { - WorkParallelForEach( - beginLayerStack, endLayerStack, - [&layerStacksToResync](const PcpLayerStackPtr& layerStack) { - if (Pcp_NeedToRecomputeDueToAssetPathChange(layerStack)) { - layerStacksToResync.push_back(layerStack); - } - }); - }); - - dispatcher.Wait(); - - for (const SdfPath& path : pathsToResync) { - changes->DidChangeSignificantly(cache, path); - } - - for (const PcpLayerStackPtr& layerStack : layerStacksToResync) { - changes->DidChangeLayerStack(cache, layerStack); - } -} - void PcpCache::Reload(PcpChanges* changes) { @@ -1078,10 +1028,7 @@ PcpCache::Reload(PcpChanges* changes) } // Reload every layer we've reached except the session layers (which we - // never want to reload from disk). Hold notifications until the end - // of this function so that clients don't alter the state of the PcpCache - // before the subsequent code is run. - SdfChangeBlock changeBlock; + // never want to reload from disk). SdfLayerHandleSet layersToReload = GetUsedLayers(); for (const SdfLayerHandle &layer : _layerStack->GetSessionLayers()) { @@ -1089,16 +1036,6 @@ PcpCache::Reload(PcpChanges* changes) } SdfLayer::ReloadLayers(layersToReload); - - // Find prim indexes and layer stacks that need to be recomputed - // due to asset path changes resulting from the reload above, - // but that aren't due to scene description changes. This ensures - // that we reload anything that may be affected by external state - // changes that aren't visible to Pcp but affect asset path resolution. - _RegisterChangesForAssetPathChanges( - this, changes, - _primIndexCache.begin(), _primIndexCache.end(), - allLayerStacks.begin(), allLayerStacks.end()); } void @@ -1146,10 +1083,7 @@ PcpCache::ReloadReferences(PcpChanges* changes, const SdfPath& primPath) } // Reload every layer used by prims at or under primPath, except for - // local layers. Hold notifications until the end of this function so - // that clients don't alter the state of the PcpCache before the - // subsequent code is run. - SdfChangeBlock changeBlock; + // local layers. SdfLayerHandleSet layersToReload; for (const PcpLayerStackPtr& layerStack: layerStacksAtOrUnderPrim) { for (const SdfLayerHandle& layer: layerStack->GetLayers()) { @@ -1160,12 +1094,6 @@ PcpCache::ReloadReferences(PcpChanges* changes, const SdfPath& primPath) } SdfLayer::ReloadLayers(layersToReload); - - // See comments in Reload. - _RegisterChangesForAssetPathChanges( - this, changes, - range.first, range.second, - layerStacksAtOrUnderPrim.begin(), layerStacksAtOrUnderPrim.end()); } void diff --git a/pxr/usd/lib/pcp/changes.cpp b/pxr/usd/lib/pcp/changes.cpp index c56c9360b3..d7370c7da0 100644 --- a/pxr/usd/lib/pcp/changes.cpp +++ b/pxr/usd/lib/pcp/changes.cpp @@ -301,10 +301,11 @@ PcpChanges::DidChange(const std::vector& caches, const SdfLayerChangeListMap& changes) { // LayerStack changes - static const int LayerStackLayersChange = 1; - static const int LayerStackOffsetsChange = 2; - static const int LayerStackRelocatesChange = 4; - static const int LayerStackSignificantChange = 8; + static const int LayerStackLayersChange = 1; + static const int LayerStackOffsetsChange = 2; + static const int LayerStackRelocatesChange = 4; + static const int LayerStackSignificantChange = 8; + static const int LayerStackResolvedPathChange = 16; typedef int LayerStackChangeBitmask; typedef std::map LayerStackChangeMap; @@ -395,7 +396,7 @@ PcpChanges::DidChange(const std::vector& caches, PcpLayerStackPtrVector stacks = cache->FindAllLayerStacksUsingLayer(layer); if (!stacks.empty()) { - cacheLayerStacks.push_back(std::make_pair( cache, stacks) ); + cacheLayerStacks.emplace_back(cache, std::move(stacks)); } } if (cacheLayerStacks.empty()) { @@ -553,6 +554,10 @@ PcpChanges::DidChange(const std::vector& caches, } break; } + + if (entry.flags.didChangeResolvedPath) { + layerStackChangeMask |= LayerStackResolvedPathChange; + } } // Handle changes that require a prim graph change. @@ -864,17 +869,27 @@ PcpChanges::DidChange(const std::vector& caches, // Process layer stack changes. This will handle both blowing // caches (as needed) for the layer stack contents and offsets, - // as well as analyzing relocation changes in the layer stack. - TF_FOR_ALL(layerStackChange, layerStackChangesMap) { - _DidChangeLayerStack( - layerStackChange->first, - layerStackChange->second & LayerStackLayersChange, - layerStackChange->second & LayerStackOffsetsChange, - layerStackChange->second & LayerStackSignificantChange); - if (layerStackChange->second & LayerStackRelocatesChange) { - _DidChangeLayerStackRelocations(caches, layerStackChange->first, - debugSummary); + // as well as analyzing relocation changes in the layer stack. + for (const auto& entry : layerStackChangesMap) { + const PcpLayerStackPtr& layerStack = entry.first; + LayerStackChangeBitmask layerStackChanges = entry.second; + + if (layerStackChanges & LayerStackResolvedPathChange) { + _DidChangeLayerStackResolvedPath(caches, layerStack, debugSummary); + if (Pcp_NeedToRecomputeDueToAssetPathChange(layerStack)) { + layerStackChanges |= LayerStackSignificantChange; + } } + + if (layerStackChanges & LayerStackRelocatesChange) { + _DidChangeLayerStackRelocations(caches, layerStack, debugSummary); + } + + _DidChangeLayerStack( + layerStack, + layerStackChanges & LayerStackLayersChange, + layerStackChanges & LayerStackOffsetsChange, + layerStackChanges & LayerStackSignificantChange); } if (debugSummary && !debugSummary->empty()) { @@ -1084,36 +1099,6 @@ PcpChanges::DidChangeLayerOffsets(PcpCache* cache) } } -void -PcpChanges::DidChangeLayerStack(PcpCache* cache, - const PcpLayerStackPtr& layerStack) -{ - _DidChangeLayerStack( - layerStack, - /* requiresLayerStackChange = */ true, - /* requiresLayerStackOffsetChange = */ true, - /* requiresSignificantChange = */ true); - - // Since this layer stack will be recomputed, need to register - // significant changes to all prim indexes that use it so that - // they pick up the new contents. - - const PcpDependencyVector deps = cache->FindSiteDependencies( - layerStack, SdfPath::AbsoluteRootPath(), - PcpDependencyTypeAnyIncludingVirtual, - /* recurseOnSite */ true, - /* recurseOnIndex */ true, - /* filter */ true); - - for (const PcpDependency& dep : deps) { - if (!dep.indexPath.IsAbsoluteRootOrPrimPath()) { - // Filter to only prims; see comment above re: properties. - continue; - } - DidChangeSignificantly(cache, dep.indexPath); - } -} - void PcpChanges::DidChangeSignificantly(PcpCache* cache, const SdfPath& path) { @@ -1820,6 +1805,50 @@ PcpChanges::_DidChangeLayerStackRelocations( } } +void +PcpChanges::_DidChangeLayerStackResolvedPath( + const std::vector& caches, + const PcpLayerStackPtr& layerStack, + std::string* debugSummary) +{ + for (PcpCache* cache : caches) { + PcpDependencyVector deps = + cache->FindSiteDependencies( + layerStack, SdfPath::AbsoluteRootPath(), + PcpDependencyTypeAnyIncludingVirtual, + /* recurseOnSite */ true, + /* recurseOnIndex */ false, + /* filterForExisting */ true); + + auto noResyncNeeded = [cache](const PcpDependency& dep) { + if (!dep.indexPath.IsPrimPath()) { + return false; + } + const PcpPrimIndex* primIndex = cache->FindPrimIndex(dep.indexPath); + return (TF_VERIFY(primIndex) && + !Pcp_NeedToRecomputeDueToAssetPathChange(*primIndex)); + }; + + deps.erase( + std::remove_if(deps.begin(), deps.end(), noResyncNeeded), + deps.end()); + if (deps.empty()) { + continue; + } + + PCP_APPEND_DEBUG( + " Resync following in @%s@ significant due to layer " + "resolved path change:\n", + cache->GetLayerStackIdentifier().rootLayer-> + GetIdentifier().c_str()); + + for (const PcpDependency& dep : deps) { + PCP_APPEND_DEBUG(" <%s>\n", dep.indexPath.GetText()); + DidChangeSignificantly(cache, dep.indexPath); + } + } +} + void PcpChanges::_DidChangeSpecStackInternal(PcpCache* cache, const SdfPath& path) { diff --git a/pxr/usd/lib/pcp/changes.h b/pxr/usd/lib/pcp/changes.h index e2adda98c3..f7b7b11751 100644 --- a/pxr/usd/lib/pcp/changes.h +++ b/pxr/usd/lib/pcp/changes.h @@ -217,12 +217,6 @@ class PcpChanges { PCP_API void DidChangeLayerOffsets(PcpCache* cache); - /// The layer stack \p layerStack changed significantly, requiring - /// the layer stack to be recomputed. - PCP_API - void DidChangeLayerStack(PcpCache* cache, - const PcpLayerStackPtr& layerStack); - /// The object at \p path changed significantly enough to require /// recomputing the entire prim or property index. A significant change /// implies changes to every namespace descendant's index, specs, and @@ -413,6 +407,12 @@ class PcpChanges { const PcpLayerStackPtr & layerStack, std::string* debugSummary ); + // Register changes to any prim indexes in \p caches that are affected + // by a change to a layer's resolved path used by \p layerStack. + void _DidChangeLayerStackResolvedPath(const std::vector& caches, + const PcpLayerStackPtr& layerStack, + std::string* debugSummary); + // The spec stack for the prim or property index at \p path must be // recomputed due to a change that affects only the internal representation // of the stack and not its contents. diff --git a/pxr/usd/lib/sdf/changeList.cpp b/pxr/usd/lib/sdf/changeList.cpp index f9c8499ad7..c61e9a9f84 100644 --- a/pxr/usd/lib/sdf/changeList.cpp +++ b/pxr/usd/lib/sdf/changeList.cpp @@ -74,6 +74,8 @@ std::ostream& operator<<(std::ostream &os, const SdfChangeList &cl) os << " didRename\n"; if (entry.flags.didChangeIdentifier) os << " didChangeIdentifier\n"; + if (entry.flags.didChangeResolvedPath) + os << " didChangeResolvedPath\n"; if (entry.flags.didReplaceContent) os << " didReplaceContent\n"; if (entry.flags.didReloadContent) @@ -156,6 +158,12 @@ SdfChangeList::DidChangeLayerIdentifier(const std::string &oldIdentifier) } } +void +SdfChangeList::DidChangeLayerResolvedPath() +{ + GetEntry(SdfPath::AbsoluteRootPath()).flags.didChangeResolvedPath = true; +} + void SdfChangeList::DidChangeSublayerPaths( const std::string &subLayerPath, SubLayerChangeType changeType ) diff --git a/pxr/usd/lib/sdf/changeList.h b/pxr/usd/lib/sdf/changeList.h index 146767bed9..a1e720d07d 100644 --- a/pxr/usd/lib/sdf/changeList.h +++ b/pxr/usd/lib/sdf/changeList.h @@ -57,6 +57,7 @@ class SdfChangeList void DidReplaceLayerContent(); void DidReloadLayerContent(); + void DidChangeLayerResolvedPath(); void DidChangeLayerIdentifier(const std::string &oldIdentifier); void DidChangeSublayerPaths(const std::string &subLayerPath, SubLayerChangeType changeType); @@ -127,6 +128,7 @@ class SdfChangeList // SdfLayer bool didChangeIdentifier:1; + bool didChangeResolvedPath:1; bool didReplaceContent:1; bool didReloadContent:1; diff --git a/pxr/usd/lib/sdf/changeManager.cpp b/pxr/usd/lib/sdf/changeManager.cpp index ca8c80b55e..72ba9106d4 100644 --- a/pxr/usd/lib/sdf/changeManager.cpp +++ b/pxr/usd/lib/sdf/changeManager.cpp @@ -226,6 +226,14 @@ Sdf_ChangeManager::DidChangeLayerIdentifier(const SdfLayerHandle &layer, _data.local().changes[layer].DidChangeLayerIdentifier(oldIdentifier); } +void +Sdf_ChangeManager::DidChangeLayerResolvedPath(const SdfLayerHandle &layer) +{ + if (!layer->_ShouldNotify()) + return; + _data.local().changes[layer].DidChangeLayerResolvedPath(); +} + static bool _IsOrderChangeOnly(const VtValue & oldVal, const VtValue & newVal ) { diff --git a/pxr/usd/lib/sdf/changeManager.h b/pxr/usd/lib/sdf/changeManager.h index 08f9bf0140..7eb4d4d502 100644 --- a/pxr/usd/lib/sdf/changeManager.h +++ b/pxr/usd/lib/sdf/changeManager.h @@ -66,6 +66,7 @@ class Sdf_ChangeManager : boost::noncopyable { void DidReloadLayerContent(const SdfLayerHandle &layer); void DidChangeLayerIdentifier(const SdfLayerHandle &layer, const std::string &oldIdentifier); + void DidChangeLayerResolvedPath(const SdfLayerHandle &layer); void DidChangeField(const SdfLayerHandle &layer, const SdfPath & path, const TfToken &field, const VtValue & oldValue, const VtValue & newValue ); diff --git a/pxr/usd/lib/sdf/layer.cpp b/pxr/usd/lib/sdf/layer.cpp index 927ed1cb40..d7ca66054f 100644 --- a/pxr/usd/lib/sdf/layer.cpp +++ b/pxr/usd/lib/sdf/layer.cpp @@ -814,6 +814,11 @@ SdfLayer::_Reload(bool force) } _assetModificationTime.Swap(timestamp); + + if (realPath != oldRealPath) { + Sdf_ChangeManager::Get().DidChangeLayerResolvedPath( + SdfLayerHandle(this)); + } } _MarkCurrentStateAsClean(); @@ -1236,6 +1241,7 @@ SdfLayer::_InitializeFromIdentifier( // must occur prior to updating the layer registry, as the new layer // information is used to recompute registry indices. string oldIdentifier = _assetInfo->identifier; + string oldRealPath = _assetInfo->realPath; _assetInfo.swap(newInfo); // Update layer state delegate. @@ -1249,9 +1255,15 @@ SdfLayer::_InitializeFromIdentifier( // Only send a notice if the identifier has changed (this notice causes // mass invalidation. See http://bug/33217). If the old identifier was // empty, this is a newly constructed layer, so don't send the notice. - if (!oldIdentifier.empty() && (oldIdentifier != GetIdentifier())) { + if (!oldIdentifier.empty()) { SdfChangeBlock block; - Sdf_ChangeManager::Get().DidChangeLayerIdentifier(self, oldIdentifier); + if (oldIdentifier != GetIdentifier()) { + Sdf_ChangeManager::Get().DidChangeLayerIdentifier( + self, oldIdentifier); + } + if (oldRealPath != GetRealPath()) { + Sdf_ChangeManager::Get().DidChangeLayerResolvedPath(self); + } } }