ynput / ayon-houdini

Houdini addon for AYON
Apache License 2.0
11 stars 9 forks source link

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

Closed BigRoy closed 4 months ago

BigRoy commented 4 months ago

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 https://github.com/ynput/ayon-core/pull/774 combined with https://github.com/ynput/ayon-houdini/pull/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. :)
MustafaJafar commented 4 months ago

⚠️ 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 https://github.com/ynput/ayon-houdini/issues/16

BigRoy commented 4 months ago

⚠️ 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 commented 4 months ago

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 commented 4 months ago

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