Add profiles to 'CollectFramesFixDef' settings

[MustafaJafar]

Changelog Description

Add profiles to 'CollectFramesFixDef' settings

Additional info

This PR is the twin of Houdini's PR ynput/ayon-houdini#45

Testing notes:

1. In Nuke, 'CollectFramesFixDef' should works as usual.
2. In Houdini, you should be able to see frames to fix in the publisher UI. (but, it won't fix anything without the mentioned PR above )

[moonyuet]

would it be better to create a new class to inherit from CollectFramesDef class for Houdini? Something like:

class CollectFramesFixDefHou(CollectFramesDef):
    """Provides text field to insert frame(s) to be rerendered for Houdini host.
    Published files of last version of an instance product are collected into
    instance.data["last_version_published_files"]. All these but frames
    mentioned in text field will be reused for new version.
    """
    label = "Collect Frames to Fix (Houdini)"
    hosts = ["houdini"]
    families = ["*"] 
    rewrite_version_enable = False

[MustafaJafar]

Cool idea but I still need to expose families to settings which will seem like we are duplicating the settings.

[MustafaJafar]

I can imagine implementing your idea as the following snippet:
and then I can add the dedicated settings in Houdini's addon.

from ayon_core.plugins.publish.collect_frames_fix import CollectFramesDef

class CollectFramesFixDefHou(CollectFramesDef):
  """Provides text field to insert frame(s) to be rerendered for Houdini host.
  Published files of last version of an instance product are collected into
  instance.data["last_version_published_files"]. All these but frames
  mentioned in text field will be reused for new version.
  """
  label = "Collect Frames to Fix (Houdini)"
  hosts = ["houdini"]   # Note how I still need to define `hosts` as I'm not using `plugin.HoudiniInstancePlugin`
  families = ["*"] 
  rewrite_version_enable = False

[moonyuet]

Guess this is the better temp solution if you want it to be specific as Houdini validator. But like @iLLiCiTiT said, it requires some discussion.
But I just want to add that the profile setting broke something in unreal publisher when I was using this branch to build the addon setting and put it into server.
The profile settings may not be the best options for this case as it affects all hosts.

[iLLiCiTiT]

To be honest I don't know how to handle these plugins. From my point of view the plugin should have only families filter without any settings. And some plugin in nuke, houdini, whatever should add the family to instance if this should happen. This topic requires more discussion.

The issue is, that if you enable this plugin for nuke, only nuke addon knows what instances (families) should be handled with this plugin, it is not something that a TD who is changing the settings should know. The same for houdini, how do TD know which families he should (can) set?

[MustafaJafar]

To be honest I don't know how to handle these plugins. From my point of view the plugin should have only families filter without any settings. And some plugin in nuke, houdini, whatever should add the family to instance if this should happen. This topic requires more discussion.

I think it might fail
Because, sometimes we use the same families names in different hosts, which may lead to undesired results.

The issue is, that if you enable this plugin for nuke, only nuke addon knows what instances (families) should be handled with this plugin, it is not something that a TD who is changing the settings should know. The same for houdini, how do TD know which families he should (can) set?

Totally agreed!
I'm not sure if this can be solved in this PR.

---

I've provided more info in my reply on discord.

[moonyuet]

Any update on this PR or?

[iLLiCiTiT]

Discussion is still alive. Is this feature that should be already done (for client)? If yes then please create houdini equivalent of the plugin in houdini addon please.

[MustafaJafar]

Closing this PR in favor of #839 (comment) .. I've copied the plugin to my ayon-houdini PR ynput/ayon-houdini#45.

[BigRoy]

Can we make this part of the "Extended Publisher" target @dee-ynput @iLLiCiTiT ? Or find another spot to track this well enough.

I understand the reason to moe this to ayon-houdini for now, but we're basically duplicating logic that we prefer to be shared between host integrations so that we don't need to redo this, each time. The more this remains, the harder it is to maintain but also to develop and maintain this feature for other existing host integrations or future host integrations since we'd just be rolling 'their own' for all otherwise.

[BigRoy]

@MustafaJafar can we for now, maybe, in core split the current functionality of this plug-in here in two.

1. We keep the CollectFramesToFix plug-in, yet ALL it does is set:

instance.data["frames_to_fix"] = blabla
instance.data["rewrite_to_last_version"] = True

Do note that I believe there are other ways of specifying that you want to write into the existing version, e.g. wouldn't just setting instance.data["version"] = previous_version do that in a way?

2. We have the other plug-in, that runs over all instances, regardless and whenever it finds that data THEN it does what the rest of the plug-in does now, like a ApplyFramesToFix plugin at collect stage that does whatever other logic is needed.

By doing so, we can still "move" Houdini-specific logic for the time being, but ALL we'd be putting there is a CollectFramesToFixHoudini that ONLY has to set that instance.data.

I believe we can even do this:

import ayon_core.plugins.publish.collect_frames_fix as frames_fix

class CollectFramesToFixHoudini(frames_fix.CollectFramesToFix):
    hosts = ["houdini"]
    families = ["*"]

Or create a dedicated 'base class' for now in some other place in core if we don't even want to duplicate the instance attribute definitions and WHAT attributes it sets on instance.data - that can then all live and be maintained in ayon-core. But we only specify the families for then in ayon-houdini.

And then we should also move this logic out of nuke... and into something "dedicated" for that frames_to_fix behavior that works across the board.

[MustafaJafar]

@BigRoy Your idea makes sense. let me give it a try.

[MustafaJafar]

or maybe I'll do it in another PR. but could we have an issue for it?

[iLLiCiTiT]

Could we rather make it family based on ayon-core with backwards compatibility?

class CollectFramesToFix(CollectFramesToFix):
    families = ["ayon.fix.frames"]

    ...Implementation...


# Backwards compatibility
class CollectFramesToFixOld(CollectFramesToFix):
    hosts = ["nuke"]
    families = ["render", "prerender"]

So only thing houdini has to do, is to add "ayon.fix.frames" to instance.data["families"].

[BigRoy]

Could we rather make it family based on ayon-core with backwards compatibility?

class CollectFramesToFix(CollectFramesToFix):
    families = ["ayon.fix.frames"]

    ...Implementation...


# Backwards compatibility
class CollectFramesToFixOld(CollectFramesToFix):
    hosts = ["nuke"]
    families = ["render", "prerender"]

So only thing houdini has to do, is to add "ayon.fix.frames" to instance.data["families"].

Almost - because the publisher UI won't show attributes for 'dynamic' families like that, only for the product type. Also, that'd mean that the ayon.fix.frames should be present on the CreatedInstance already?

Becuase preferably you'd even share the input field itself among them, yes? So that tooltip etc. is all maintained in one place?

[iLLiCiTiT]

Ok, I see this is more important than I thought #860

[iLLiCiTiT]

Ok, I see this is more important than I thought #860

Just realized this might not solve the issue.

Another issue is importing plugin from ayon_core/plugins/publish/. If we need to import something it should be in ayon_core/pipeline/publish/. Plugins should never be imported.

NOTE:
We might split the logic into 2 plugins, first would be same as my suggestion. Second would be to change hosts and families filtering to show definitions, it would store it to instance and add "ayon.fix.frames" family. This plugin base would be in ayon_core/pipeline/publish/ and imported where needed to change hosts and families.

[dee-ynput]

but could we have an issue for it?

Here it is: #879