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

Houdini: Remove setParms call since it's responsibility of self.imprint to set the values #5796

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Oct 18, 2023

Changelog Description

Revert a recent change made in #5621 due to this comment. However the change is faulty as can be seen mentioned here

Additional info

This is how I tested - if I open the publisher and "reset" in the publisher the instances also match the states set in this code, e.g. all on or all off depending on how I ran it.

from openpype.pipeline import registered_host
from openpype.pipeline.create import CreateContext

host = registered_host()
context = CreateContext(host, reset=True)
for instance in context.instances:
    instance.data["active"] = False
context.save_changes()

# Assert all are off - then enable all
context = CreateContext(host, reset=True)
assert all(instance.data["active"] == False for instance in context.instances)
print("All are off")
for instance in context.instances:
    instance.data["active"] = True
context.save_changes()

# Assert all are on
context = CreateContext(host, reset=True)
assert all(instance.data["active"] == True for instance in context.instances)
print("All are on")
print("Success")

It could be that the original test case that @MustafaJafar tried that failed has an instance_node that by chance has an active parm of its own and thus can't be a replaced spare parm because it is not a spare parm as such lib.imprint might fail to update? However, without a clear test case that reproduces the issue it's hard to figure out if that's the issue he was facing. If that is the case it just hints that we should have prefixed all Creator related attributes with e.g. openpype_ or ayon_ to avoid such clashes.

Testing notes:

  1. Publishing should work as expected
  2. Self publishing should also work as expected

@ynbot ynbot added the size/XS Denotes a PR changes 0-99 lines, ignoring general files label Oct 18, 2023
@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 18, 2023

This is the issue @MustafaJafar is facing:

issue.mp4

That's why originally he added the line:

hotfix.mp4

@BigRoy BigRoy marked this pull request as draft October 18, 2023 17:43
@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 18, 2023

Converting to draft since I'm able to reproduce and luckily have my test code failing too - will investigate more tomorrow.

@MustafaJafar
Copy link
Contributor

I think I found it but I have no idea yet how to solve it.
imprint changes only those attributes that we didn't interact with them

The_Bug.mp4

…lue manually + refactor deprecated `Node.replaceSpareParmTuple` to use `ParmTemplateGroup.replace` instead
…s + do nothing if both no new and no update parms to process
@BigRoy BigRoy marked this pull request as ready for review October 18, 2023 20:33
@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 18, 2023

So I found the issue - the issue was that if a parm was NOT on its default value (the user had manually changed it for example or called setParms on it instead of the imprint) then on update to a parm with a new default then it'd preserve the value override and thus NOT use the new value.

It was thus a matter of calling Parm.revertToDefault().

I've also refactored the code a bit because:

  • Variables were often named parm where they were actually referring to parm_template so I renamed those.
  • If previously there was no new parm and no update parm and all parms were NOT in the "Extra" group then it'd still create an empty "Extra" group. This was a bit of an edge case but I moved the folder creation/change into an if statement to only ever occur IF it had to add new parms

Copy link
Contributor

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

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

Tested and It works!

@antirotor antirotor merged commit 9155029 into ynput:develop Oct 19, 2023
10 checks passed
@ynbot ynbot added this to the next-patch milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution host: Houdini size/XS Denotes a PR changes 0-99 lines, ignoring general files type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants