Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Maya: Maintain time connections on Alembic update. #4143

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

tokejepsen
Copy link
Member

Brief description

Maintain time connections on Alembic update.

Description

This maintains the connections to the time attribute on the alembic node in Maya so you can for example animate the time.

Additional info

The rest will be ignored in changelog and should contain any additional
technical information.

Documentation (add "type: documentation" label)

feature_documentation

Testing notes:

  1. Load Alembic.
  2. Disconnect time and key it.
  3. Update Alembic.

@LiborBatek LiborBatek self-requested a review November 29, 2022 07:58
LiborBatek
LiborBatek previously approved these changes Nov 29, 2022
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Been tested in maya 2022 and 2023 and works ok! Also tested with asset hero version switching. No problems found.

Comment on lines 282 to 283
else:
cmds.setAttr(node_attr, data["value"])
Copy link
Collaborator

@BigRoy BigRoy Nov 29, 2022

Choose a reason for hiding this comment

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

Doesn't updating the alembic automatically reconnect the time attribute and thus for us to set the value we'll first need to force disconnect it?

To test:

  • Load alembic
  • Disconnect time
  • Set manually (so basically not reconnecting/keying it)
  • Update alembic with this PR

Does that work as expected? Is time still disconnected and did it maintain the value that was set manually?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this would mean if you'd update without any changes to .time attribute it'd probably log a warning or error on update that time was already connected?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. Will have a look at this.

Comment on lines 230 to 237
data = {
"connected": False,
"attribute": None,
"value": cmds.getAttr(node_attr)
}
if connections:
data["connected"] = True
data["attribute"] = connections[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this could be rewritten to:

    "input": next(iter((cmds.listConnections(node_attr, plugs=True, destinations=False) or []), None))
    "value": cmds.getAttr(node_attr)
}

So basically no need to track connected + attribute separately since if input is None you know it wasn't connected. You can just store input (== attribute) as single value.

@@ -226,7 +226,17 @@ def update(self, container, representation):
if alembic_nodes:
for attr in alembic_attrs:
node_attr = "{}.{}".format(alembic_nodes[0], attr)
alembic_data[attr] = cmds.getAttr(node_attr)
connections = cmds.listConnections(node_attr, plugs=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have argument destinations=False since otherwise it could also list outputs of the attribute.

@LiborBatek LiborBatek dismissed their stale review November 30, 2022 09:20

According to BigRoys comments not fully functional yet and changes needed

@tokejepsen tokejepsen marked this pull request as draft November 30, 2022 10:19
- account for time attribute reconnection.
- simpler data collection
@tokejepsen tokejepsen marked this pull request as ready for review November 30, 2022 17:37
@antirotor antirotor added host: Maya type: bug Something isn't working labels Dec 2, 2022
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Works as expected ...even when disconnecting time atribute and put just frame# manually (without setting anim key). Keeps the value intact when changing asset version. No any errors given.

@tokejepsen
Copy link
Member Author

@BigRoy you happy with the PR?

Comment on lines 276 to 279
if data["input"]:
cmds.connectAttr(
data["input"], node_attr, force=True
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this make sure that the connection to be made doesn't already exist? (is a warning issues when e.g. time was previously connected and you'd update (the update reconnects time by default) and thus this tries to connect time again even though already connected. Does that error or log a warning?

Copy link
Member

Choose a reason for hiding this comment

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

note from testing side...it didnt reconnect time by default after updating the asset so no chance for any warning or error. If I am correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

note from testing side...it didnt reconnect time by default after updating the asset so no chance for any warning or error. If I am correct.

Thank you - so did it just always result in an unconnected attribute, even if you previously had it connected? :)
Doesn't it by default connect to time?

Comment on lines 229 to 231
inputs = cmds.listConnections(
node_attr, plugs=True, destination=False
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel the code would be more readable if we'd move this to a simple separate function:

def get_input(attr):
    connections = cmds.listConnections(attr, plugs=True, destination=False)
    return connections[0] if connections else None

The code here could then just be:

                    data = {
                        "input": get_input(node_attr),
                        "value": cmds.getAttr(node_attr)
                    }

And further down would just be:

            if alembic_nodes:
                alembic_node = alembic_nodes[0]  # assume single AlembicNode
                for attr, data in alembic_data.items():
                    node_attr = "{}.{}".format(alembic_node, attr)
                    if data["input"]:
                        if data["input"] != get_input(node_attr):
                            cmds.connectAttr(
                                data["input"], node_attr, force=True
                            )
                    else:
                        input = get_input(node_attr)
                        if input:
                            cmds.disconnectAttr(input, node_attr)
                        cmds.setAttr(node_attr, data["value"])

Anyway, just feels more readable to only me maybe.

@tokejepsen
Copy link
Member Author

@BigRoy what do you think now?

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Haven't run the code, but code looks good to me.

@antirotor antirotor merged commit 375a579 into ynput:develop Dec 12, 2022
@github-actions github-actions bot added this to the next-patch milestone Dec 12, 2022
@tokejepsen tokejepsen deleted the enhancement/maya_alembic branch December 12, 2022 16:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
host: Maya type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants