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.

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

Fix lost connection for output port with parent #2729

Merged
merged 7 commits into from
Dec 1, 2022

Conversation

alicedegirolamo
Copy link
Collaborator

Simple fix.
When renaming output ports on the compound, connections with the parent are lost.
This was caused by the fact that the update of connections with the new path was done on the children of the parent and not also on the parent itself.

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Very good. Can you add a unit test that checks this bug stays fixed?

@alicedegirolamo
Copy link
Collaborator Author

Very good. Can you add a unit test that checks this bug stays fixed?

Sure! I'm going to do that.

@JGamache-autodesk JGamache-autodesk added ufe Related to UFE component in Maya ufe-usd Related to UFE-USD plugin in Maya-Usd labels Nov 22, 2022
@alicedegirolamo
Copy link
Collaborator Author

Very good. Can you add a unit test that checks this bug stays fixed?

Ok, I pushed and tested the new test. I had to add a new simple compound to the .usd since I didn't find in the sample folder a compound suitable for my test case.

@JGamache-autodesk
Copy link
Collaborator

Preflight was successful (after a review of the logs). The two failure to upload artifacts happened after the tests succeeded.

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 24, 2022
}
}

static void setConnectionsWithChildren(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find the name ambiguous. It could be:

  • setConnectionsOfAllChildren()
  • updateChildrenConnections() +1

lib/mayaUsd/ufe/UsdAttributes.cpp Outdated Show resolved Hide resolved
@@ -504,28 +504,37 @@ bool UsdAttributes::canRenameAttribute(

static void setConnections(
const PXR_NS::UsdPrim& prim,
const SdfPath& OldPropertyPath,
const SdfPath& oldPropertyPath,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The method does not set the connection it only updates existing connections:
updateConnections()

lib/mayaUsd/ufe/UsdAttributes.cpp Show resolved Hide resolved
test/lib/ufe/testAttributes.py Outdated Show resolved Hide resolved

self.assertEqual(ufe.PathString.string(dstAttr.path),
'|stage|stageShape,/pCube2/Looks/standardSurface2SG')
self.assertEqual(dstAttr.name, 'outputs:out')
Copy link
Collaborator

@hodoulp hodoulp Nov 24, 2022

Choose a reason for hiding this comment

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

Could you enhance the test with cmds.undo() ?

@seando-adsk seando-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Nov 24, 2022
@hodoulp
Copy link
Collaborator

hodoulp commented Nov 28, 2022

I think that there are two main pending points:

  • The recursivity question here
  • the undo() / redo() improvement in the unit test

But I will @JGamache-autodesk and Orn have the last call on that.

hodoulp
hodoulp previously approved these changes Nov 28, 2022
@JGamache-autodesk
Copy link
Collaborator

  • Going recursive is unnecessary on well constructed graphs. Connections on outer ports can only come from nodes in the englobing compound (or inner ports of the englobing compound). Connections on inner ports can only come from nodes directly inside the compound.
  • Agreed that testing undo redo would be nice.

@alicedegirolamo
Copy link
Collaborator Author

alicedegirolamo commented Nov 29, 2022

I think that there are two main pending points:

* The recursivity question [here](https://github.com/Autodesk/maya-usd/pull/2729/files/218a234bccfb647f2ea3b5bf7e1b05ff75846762#diff-db3eb61f02eef0d4aa689c03f427b680616a013e850e3ea7870664d3e009a191R527)

* the undo() / redo() improvement in the unit test

But I will @JGamache-autodesk and Orn have the last call on that.

Yes, I have to add the undo/redo test but I'm having problems compiling the updated Maya. As soon as I fixed my build I'm going to add it. Sorry for the wait.

Edit: I added the undo/redo test.

@alicedegirolamo alicedegirolamo added the ready-for-merge Development process is finished, PR is ready for merge label Dec 1, 2022
@neilh-adsk neilh-adsk merged commit 0f3cb4c into dev Dec 1, 2022
@neilh-adsk neilh-adsk deleted the degiroa/MAYA-126335/lost-connections-renaming branch December 1, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge ufe Related to UFE component in Maya ufe-usd Related to UFE-USD plugin in Maya-Usd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants