EMSUSD-2681 - Fix duplicate not duplicating connections to NodeGraph outputs.

[frohnej-adsk]

The bug could be reproduced easily in LookdevX:

• Create an add node inside of a compound
• Connect the add node to an output of the compound
• Copy/Paste the compound
• Connection is missing in the copied compound

[frohnej-adsk]

All builds are green except for 2023 (Windows), where 305 - GTest:AL_USDTransactionTests timed out. I can rerun the preflight once the PR was reviewed.

[frohnej-adsk]

The bug was caused by these lower_bound and upper_bound optimizations.

Cause of the bug

Assume the following hierarchy:

mtl
\---standard_surface1
    \---compound1
        \---add1

Let /mtl/standard_surface1/compound1.outputs:out be connected to /mtl/standard_surface1/compound1/add1.outputs:out.

Upon copying /mtl/standard_surface1/compound1 to the clipboard, updateSdfPathVector() will be called for the attribute /compound1.outputs:out within the clipboard with the following parameters:

• pathVec = ["/compound1/add1.outputs:out"]
• duplicatePair = ("/mtl/standard_surface1/compound1", "/compound1")
• otherPairs = [("/mtl/standard_surface1/compound1", "/compound1")]

Note that pathVec already contains the correct updated path. This case is supposed to be handled by the if (*itPath == duplicatePair) branch. However, due to the optimization, that line is never hit and we ended up erroneously deleting the connection:

SdfPaths are compared lexicographically. Thus, finalPath == /compound1/add1.outputs:out is less than /mtl/standard_surface1/compound1.

• otherPairs.lower_bound(finalPath) returns the first element e such that finalPath <= e. That's /mtl/standard_surface1/compound1.
• otherPairs.upper_bound(finalPath) returns the first element e such that finalPath < e. That's /mtl/standard_surface1/compound1.

I.e., lower_bound(finalPath) == upper_bound(finalPath), which causes the loop to never be entered and the connection to be deleted because it's assumed to be external.

Optimization and proposed fix

I'm still struggling to wrap my head around the optimization. I understand why it fails in this case but I don't fully understand how it works in regular cases. The STL lower_bound() and upper_bound() methods don't really do what their name suggests, which makes it kind of confusing (see https://stackoverflow.com/a/67551612) and makes me wonder if it actually works as intended.

Thus, I simply removed the optimization in this PR. That might be good enough and it makes the code easier to read and more similar to the other duplicate command (lib\mayaUsd\ufe\UsdUndoDuplicateSelectionCommand.cpp). If you can think of a good way to fix the bug while keeping the optimization, let me know!

[JGamache-autodesk]

The big question is "why is a remapping function being called on something that is already remapped". The updated code currently "stumbles" across it via the duplicatePair entry because it iterates everything, which is quite bad performance wise.

Let's try to explain the weird code you are seeing. The map contains prefixes that are the shortest possible, so even though you duplicated both /mtl/standard_surface1/compound1 and /mtl/standard_surface1/compound1/add1 the map will only contain /mtl/standard_surface1/compound1.

Now, let's say I want to know what happened to /mtl/standard_surface1/compound1/add1 and search for it using lower_bound(). I then get an iterator that is 1 element past /mtl/standard_surface1/compound1 so I need to back up one element when possible to include the prefix in the loop. I will end up on the /mtl/standard_surface1/compound1 entry which will allow me to remap correctly using ReplacePrefix.

If, instead I want to know about /mtl/standard_surface1/compound1, then the iterator returned by lower_bound() will actually directly point to it. We still go back one item (for nothing), but will get it on the second iteration. On the second iteration we will hit the equal case and directly rewrite to the destination path in pathVec.

In all cases upper_bound() will either be equal to lower_bound() if finalPath was not directly in the map, or will point one element past it if it was found, which is exactly what we want.

It is an interesting dance, but it actually works. This obviously requires a bit more code documentation.

But let's get back to your problem. The case you are seeing is that you have a connection on the compound boundary that is pointing inside the compound and should be left alone. If you look at what happens in the caller on the exit of that function you either remap modified paths, or remove the attribute. There is no "leave it alone" option.

This means the caller should check for that and remove paths that are obviously correct:

void UsdUndoDuplicateSelectionCommand::execute()
{
...
                    attr.GetConnections(&sources);
                    // Sources that are already inside the duplicate should be ignored:
                    const auto new_end = std::remove_if(sources.begin(), sources.end(), [&duplicatePair](const auto& path){ return path.HasPrefix(duplicatePair.second); });
                    sources.erase(new_end, sources.end());
                    // Now we know we only have external sources left:
                    if (!sources.empty() && updateSdfPathVector(sources, duplicatePair, stageData.second)) {
...
                    // Need the exact same for properties
...
}

[JGamache-autodesk]

Not good enough. Too brutal. If there is a mix of internal and external connections we might end up calling ClearConnections(). I suspect we need to process connections one by one.
What we want is:

finalSources = []
for cnx in sources:
    if cnx.HasPrefix(duplicatePair.second):
        # Internal, keep it:
        finalSources.push_back(cnx)
        continue  # Internal

    remappedCnx = None
    it = stageData.second.lower_bound(cnx)
    if it.first == cnx:
        # Direct match to one of the duplicate sources: remap
        # Note. Probably won't happen.
        remappedCnx = it.second
    else:
        # Can we find the prefix by going back one item?
        if it != stageData.second.begin():
            --it
            if cnx.HasPrefix(it.first):
                remappedCnx = cnx.RemapPrefix(it.first, it.second)
                
    if remappedCnx:
        # Remapped from external to internal:
        finalSources.push_back(remappedCnx)
    
    # Else that external connection gets removed
    
# Identical as before:
if (sources.empty()) {
    attr.ClearConnections();
    if (!attr.HasValue() && !UsdShadeNodeGraph(attr.GetPrim())) {
        p.RemoveProperty(prop.GetName());
    }
} else {
    attr.SetConnections(sources);
}

Could probably be done by the existing algo, by ignoring the changed return value in the caller and by keeping items that are already remapped by adding a (finalPath.HasPrefix(duplicatePair.second) || finalPath == duplicatePair.second) check at the beginning of the loop to continue and leave the item unchanged in the final vector.

[frohnej-adsk]

Thank you, this was very helpful to understand the idea of the optimization!

This is my current understanding and what I implemented in my latest commit. Please double-check if it's correct.

The goal is to check if any path in the DuplicatePathsMap is a prefix of the path we want to update. Due to lexicographical ordering, the prefix of a path is always less than or equal to the path itself. We can utilize that to check only a single candidate: The last/greatest path that's less then or equal to the path we want to update. This candidate is right before the path returned by upper_bound(), which is the first path that's greater than the path we want to update.

[JGamache-autodesk]

The big question is "why is a remapping function being called on something that is already remapped". The updated code currently "stumbles" across it via the duplicatePair entry because it iterates everything, which is quite bad performance wise.

Let's try to explain the weird code you are seeing. The map contains prefixes that are the shortest possible, so even though you duplicated both /mtl/standard_surface1/compound1 and /mtl/standard_surface1/compound1/add1 the map will only contain /mtl/standard_surface1/compound1.

Now, let's say I want to know what happened to /mtl/standard_surface1/compound1/add1 and search for it using lower_bound(). I then get an iterator that is 1 element past /mtl/standard_surface1/compound1 so I need to back up one element when possible to include the prefix in the loop. I will end up on the /mtl/standard_surface1/compound1 entry which will allow me to remap correctly using ReplacePrefix.

If, instead I want to know about /mtl/standard_surface1/compound1, then the iterator returned by lower_bound() will actually directly point to it. We still go back one item (for nothing), but will get it on the second iteration. On the second iteration we will hit the equal case and directly rewrite to the destination path in pathVec.

In all cases upper_bound() will either be equal to lower_bound() if finalPath was not directly in the map, or will point one element past it if it was found, which is exactly what we want.

It is an interesting dance, but it actually works. This obviously requires a bit more code documentation.

But let's get back to your problem. The case you are seeing is that you have a connection on the compound boundary that is pointing inside the compound and should be left alone. If you look at what happens in the caller on the exit of that function you either remap modified paths, or remove the attribute. There is no "leave it alone" option.

This means the caller should check for that and remove paths that are obviously correct:

void UsdUndoDuplicateSelectionCommand::execute()
{
...
                    attr.GetConnections(&sources);
                    // Sources that are already inside the duplicate should be ignored:
                    const auto new_end = std::remove_if(sources.begin(), sources.end(), [&duplicatePair](const auto& path){ return path.HasPrefix(duplicatePair.second); });
                    sources.erase(new_end, sources.end());
                    // Now we know we only have external sources left:
                    if (!sources.empty() && updateSdfPathVector(sources, duplicatePair, stageData.second)) {
...
                    // Need the exact same for properties
...
}

[JGamache-autodesk]

Not good enough. Too brutal. If there is a mix of internal and external connections we might end up calling ClearConnections(). I suspect we need to process connections one by one.
What we want is:

finalSources = []
for cnx in sources:
    if cnx.HasPrefix(duplicatePair.second):
        # Internal, keep it:
        finalSources.push_back(cnx)
        continue  # Internal

    remappedCnx = None
    it = stageData.second.lower_bound(cnx)
    if it.first == cnx:
        # Direct match to one of the duplicate sources: remap
        # Note. Probably won't happen.
        remappedCnx = it.second
    else:
        # Can we find the prefix by going back one item?
        if it != stageData.second.begin():
            --it
            if cnx.HasPrefix(it.first):
                remappedCnx = cnx.RemapPrefix(it.first, it.second)
                
    if remappedCnx:
        # Remapped from external to internal:
        finalSources.push_back(remappedCnx)
    
    # Else that external connection gets removed
    
# Identical as before:
if (sources.empty()) {
    attr.ClearConnections();
    if (!attr.HasValue() && !UsdShadeNodeGraph(attr.GetPrim())) {
        p.RemoveProperty(prop.GetName());
    }
} else {
    attr.SetConnections(sources);
}

Could probably be done by the existing algo, by ignoring the changed return value in the caller and by keeping items that are already remapped by adding a (finalPath.HasPrefix(duplicatePair.second) || finalPath == duplicatePair.second) check at the beginning of the loop to continue and leave the item unchanged in the final vector.

[seando-adsk]

@pierrebai-adsk Can you code review these changes?

[JGamache-autodesk]

Thanks, the code is easier to understand now.

[pierrebai-adsk]

Just one question about your own comment.

[JGamache-autodesk]

LGTM. Thanks!