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

Creators: Remove legacy pop of the 'active' data on create of instance. #41

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Jul 13, 2024

Changelog Description

Creators: Remove legacy pop of the 'active' data on create of instance.

Additional info

This was a leftover from legacy creator where the active state was defined by the Bypass state of the node instead of a dedicated parm for the publisher.

⚠️ Question:

There also seems to be this left-over Collect Active State plug-in. Which currently would be unable to activate any inactive instances as far as I know because new publisher would have those instances already deactivated from CreateContext. However, it will disable instances if the node were currently set to "bypass" but the instance was active in publisher.

Question: Should we also remove the Collect Active State plug-in to avoid this unwanted side effect?


This fixes the default active state creation for ynput/ayon-core#774 combined with #36

Testing notes:

  1. Creators should still work
  2. The active state should be imprinted and collected the same way as before this PR.
  3. Double check HDA creator and publishing works, because that one always has been special. :)

This was a leftover from legacy creator where the active state was defined by the Bypass state of the node instead of a dedicated parm for the publisher.
Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

Tested by creating new instances, the active data are still located in the extra attributes.
Looks good.
image
image

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.

It works.

@MustafaJafar
Copy link
Contributor

MustafaJafar commented Jul 15, 2024

⚠️ Question:

There also seems to be this left-over Collect Active State plug-in. Which currently would be unable to activate any inactive instances as far as I know because new publisher would have those instances already deactivated from CreateContext. However, it will disable instances if the node were currently set to "bypass" but the instance was active in publisher.

Question: Should we also remove the Collect Active State plug-in to avoid this unwanted side effect?

What about adding these few lines here and remove Collect Active State plug-in ? (btw the it works on my side.)

            node = hou.node(node_path)
            if hasattr(node, "isBypassed") and node.isBypassed():
                node_data["active"] = False

Side note: I think your question is related to this issue #16

@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 15, 2024

⚠️ Question:
There also seems to be this left-over Collect Active State plug-in. Which currently would be unable to activate any inactive instances as far as I know because new publisher would have those instances already deactivated from CreateContext. However, it will disable instances if the node were currently set to "bypass" but the instance was active in publisher.
Question: Should we also remove the Collect Active State plug-in to avoid this unwanted side effect?

What about adding these few lines here and remove Collect Active State plug-in ? (btw the it works on my side.)

            node = hou.node(node_path)
            if hasattr(node, "isBypassed") and node.isBypassed():
                node_data["active"] = False

Side note: I think your question is related to this issue #16

We could - but I believe the relying on the "bypass" state was removed on purpose to avoid interfering with a native houdini feature that artists may use for something else. @antirotor was back then involved with that decision-making.

So if we were to stick with that I'd say just remove that collector (and then of course still validate that nodes that are set to bypass do actually actively render the ROPs the way we want them to.) (they should still work in publishing then.)

@MustafaJafar
Copy link
Contributor

MustafaJafar commented Jul 15, 2024

We could - but I believe the relying on the "bypass" state was removed on purpose to avoid interfering with a native houdini feature that artists may use for something else. @antirotor was back then involved with that decision-making.

So if we were to stick with that I'd say just remove that collector (and then of course still validate that nodes that are set to bypass do actually actively render the ROPs the way we want them to.) (they should still work in publishing then.)

I'd appreciate shedding some light on these feature.

The only thing I was able to think of is publishing existing files as Houdini will skip rendering without not raising any errors.
(btw, it was my first time to know that this actually works - I disabled validate bypass validator.)
image

@BigRoy
Copy link
Contributor Author

BigRoy commented Jul 15, 2024

We could - but I believe the relying on the "bypass" state was removed on purpose to avoid interfering with a native houdini feature that artists may use for something else. @antirotor was back then involved with that decision-making.
So if we were to stick with that I'd say just remove that collector (and then of course still validate that nodes that are set to bypass do actually actively render the ROPs the way we want them to.) (they should still work in publishing then.)

I'd appreciate shedding some light on these feature.

The only thing I was able to think of is publishing existing files as Houdini will skip rendering without not raising any errors. (btw, it was my first time to know that this actually works - I disabled validate bypass validator.) image

Thanks - we could make the Bypass validation optional and force the RopNode.render with the ignore_bypass_flags=True arguments but seems more counter-intuitive even. (Especially because that may hold even more untrue for what e.g. a deadline render farm may do with those ROPs if they were bypassed).

I'll merge this PR.

The Collect Active State collector being removed is in a separate PR #42

@BigRoy BigRoy merged commit 235ef6e into ynput:develop Jul 15, 2024
1 check passed
@BigRoy BigRoy deleted the bugfix/creators_remove_pop_active_state branch July 15, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants