Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAYA-123579 fix crash when deleting a stage #2393

Merged
merged 2 commits into from
Jun 1, 2022
Merged

MAYA-123579 fix crash when deleting a stage #2393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
581e02d
MAYA-123579 fix crash when deleting a stage
pierrebai-adsk May 31, 2022
3edaa09
MAYA-123579 ifx unit test requirements
pierrebai-adsk Jun 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions lib/mayaUsd/ufe/UsdStageMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,21 @@ UsdStageMap::StageSet UsdStageMap::allStages()
{
rebuildIfDirty();

StageSet stages;
// Note: stage() calls proxyShape() which can modify fPathToObject,
// so we cannot call stage within a loop on fPathToObject.
//
// So, we first cache all UFE paths and then loop over them
// calling stage().

std::vector<Ufe::Path> paths;
paths.reserve(fPathToObject.size());
for (const auto& pair : fPathToObject) {
stages.insert(stage(pair.first));
paths.emplace_back(pair.first);
}

Comment on lines +239 to +244
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to copy the vector yourself. There are both copy constructor and assignment operator. So you can just do:
auto paths(fPathToObject);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source container is a std::map, so there are no copy constructor for its keys.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I missed that. I guess in that case I would have made a copy of the map (to a map) and loop over the map copy. But your way is also fine.

StageSet stages;
for (const auto& path : paths) {
stages.insert(stage(path));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code will append an empty/invalid Stage to the set when processing a deleted stage (but only once since all deleted stages will return the same hash). Would it be better to test that the stage is valid before adding it to the StageSet? The function is called allStages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same reflection, but the old code did not check the validity either. The stage map holds weak pointers, so the validity can change at any moment anyway...

}
return stages;
}
Expand Down
29 changes: 29 additions & 0 deletions test/lib/mayaUsd/nodes/testProxyShapeBase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,35 @@ def testDuplicateProxyStageAnonymous(self):
self.assertEqual(0, len(ufe.Hierarchy.hierarchy(duplProxyShapeItem).children()))
self.assertEqual(1, len(ufe.Hierarchy.hierarchy(proxyShapeItem).children()))

@unittest.skipUnless(os.getenv('UFE_PREVIEW_VERSION_NUM', '0000') > '4012', 'testDeleteStage requires Ufe Preview Version at least 4013.')
def testDeleteStage(self):
'''
Verify that we can delete the stage.

Only testable in recent UFE API because it requires findGatewayItems
to trigger the original crash that led to this test being created.
'''
# create new stage
cmds.file(new=True, force=True)

# Open usdCylinder.ma scene in testSamples twice, so that deleting one instance
# will still leave another and have something to iterate over in th estage cache.
mayaUtils.openCylinderScene()
mayaUtils.openCylinderScene()

# get the proxy shape path
proxyShapes = cmds.ls(type="mayaUsdProxyShapeBase", long=True)

# Get the USD handler.
proxyShapePath = ufe.PathString.path(proxyShapes[0])
handler = ufe.RunTimeMgr.instance().sceneSegmentHandler(proxyShapePath.runTimeId())

# Delete the proxy shape.
cmds.delete(proxyShapes[0])

# Request the now dead proxy shape. This should not crash.
result = handler.findGatewayItems(proxyShapePath)

@unittest.skipUnless(ufeUtils.ufeFeatureSetVersion() >= 2, 'testDuplicateProxyStageFileBacked only available in UFE v2 or greater.')
def testDuplicateProxyStageFileBacked(self):
'''
Expand Down