Attempt to fix an issue with the MergingSceneIndex.

[marktucker]

Description of Change(s)

When triggering a "whole scene" removal, this scene index can cause the whole scene to be "added", and nothing to be "removed", which is a problem. This can be triggered by the removeal of a sublayer from the stage. This way of handling the change causes accesses to expired UsdPrims, which throw exceptions.

This commit attempts to be more precise about the add and remove changes sent in this situation by "adding" prims that are still in any merged scene index, and "removing" any prims that exist only in the scene index that triggered the change. This can result in a lot of small changes instead of one big change, so there may be a need to optimize this, but I think the principle is correct (and the commit does prevent the invlid UsdPrim access exceptions).

I'm not at all sure that this is the right fix here, but it was enough to get me over the bump I hit with using USDIMAGINGGL_ENGINE_ENABLE_SCENE_INDEX=1 in Houdini (which does a lot of sublayer removal and adding on a hydra-watched stage). So this is really more meant as a starting point rather than something I expect to be merged as is.

Fixes Issue(s)

• 3261

• [ X ] I have verified that all unit tests pass with the proposed changes

• [ X ] I have submitted a signed Contributor License Agreement

[jesschimein]

Filed as internal issue #USD-10054

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[unhyperbolic]

Thanks for your change.

I want to get clarity on one issue: UsdImagingStageSceneIndex::GetPrim and UsdImagingStageSceneIndex::GetChildPrimPaths both have an early bail "if (!prim) { return EMPTY; }". So even without your fix, we shouldn't get invalid UsdPrim access exceptions. Can you explain how you are still seeing them without your change?

I have two notes:

1. When emitting filteredEntries.emplace_back(descendantPath), we can actually skip all descendants. This can be achieved by calling HdSceneIndexPrimView::const_iterator::SkipDescendants(). Note that the for-loop needs to be "un-C++11-ified" to "for (auto it = primView.begin(); it != primView.end(); ++it)".

2. The new for-loop over the _inputs is using a different condition then existing for-loop. I think that either are actually not quite correct. That is, conservatively, they should check whether the prim type is non-trivial or the data source pointer is non-null or the children are non-empty. It would be good to take those two for-loops and turn them into a helper method _IsPrimFullyRemove(const HdSceneIndexBase &sender, const SdfPath &primPath).

[unhyperbolic]

Quick question: does your change fix #3261 ?

[unhyperbolic]

I think that the HdFlatteningSceneIndex can hold on to the prim source. And that ultimately prolongs the life time of some UsdImaging data sources that now access expired usd objects. Your change avoids the bug. But I wonder whether we should guard more generally in usd imaging against expired usd objects.

[marktucker]

Sorry, yes, this is intended to be a fix for #3261. I hope the bug description there answers your "Can you explain how you are still seeing them without your change?" question.

1. Yes! This makes a lot of sense, thanks.
2. Yes, it has a different condition because it wasn't skipping descendants using the method you described in point 1. But yes, I think you're right that the two loops can be made the same after following your first recommendation. But I don't understand your comment about changing both loops to add the new check that "they should check whether the prim type is non-trivial". Perhaps you could provide the source code for what you think this condition should be?

As to your final point of "I wonder whether we should guard more generally in usd imaging against expired usd objects.", I completely agree. And in such a world maybe this change would not be necessary at all. However I have no idea how to enforce this more general guarding against expired USD objects. Thus my original comment that "I'm not at all sure that this is the right fix here". I'm happy to have this PR rejected if you can fix this bug at its root, but I don't think I'm capable of doing that.

Thanks!

[unhyperbolic]

I had another realization. Assume there are two input scene to the merging scene index that start as
SceneIndex 1:
/P/A
/P/B
SceneIndex 2:
/P/C
/P/D

SceneIndex1 removes /P (and sends the corresponding PrimRemovedEntry to the merging scene index).
The merging scene index now needs to send:
PrimRemovedEntries: /P/A, /P/B

With your change, I think it would send instead:
PrimAddedEntries: /P/C, /P/D [These are not really necessary because there were no contributing data sources in SceneIndex 1].

The problem is that we do not keep state in the HdMergingSceneIndex what the children of /P were, so we cannot really send out the targeted PrimRemovedEntries.

The conservative thing we could do is:
PrimRemovedEntries: /P
PrimAddedEntries: /P, /P/C, /P/D

Also note that _PrimsRemoved is not consulting the _InputEntry::sceneRoot. I wonder whether we can use that for optimization in the HdMergingSceneIndex when processing messages as well.

I am hesitant to make the HdMergingSceneIndex stateful to know which children got removed. But I am also afraid of the performance implication of the conservative approach. Maybe it is not really that bad?

[marktucker]

It sounds like you are becoming convinced that this PR is not doing the right thing, so you should probably just close it. I will be of no help in resolving the issues you are describing. At best I could try to follow some detailed instructions, but I would have little input or ability to verify the correctness of your suggestions. I think this bug will have to be resolved by Pixar. At least I'm glad to hear that you think that there is a problem with the existing code.