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

Fusion: new creator for image product type #6057

Merged

Conversation

kalisp
Copy link
Member

@kalisp kalisp commented Dec 14, 2023

Changelog Description

In many DCC render product type is expected to be sequence of files. This PR adds new explicit creator for image product type which is focused on single frame image.
Workflows for both product types might be a bit different, this gives artists more granularity to choose better workflow.

Additional info

Original CreateSaver name is staying even if more descriptive name (as CreateSequenceSaver) would be better, but there are existing Settings which translation would bring too much risk to demand this change.
Both creators contain same settings schema, but it is expected that they might be separated more in the future.

Testing notes:

  1. Try to use new Image (saver) creator and publish
  2. Play with various frame ranges, it should validate that only single frame is being published (but allows same source of frame range as original creator)
  3. Use original Render (saver) creator - nothing should break.

'image' product type should result in single frame output, 'render' should be more focused on multiple frames.
@ynbot
Copy link
Contributor

ynbot commented Dec 14, 2023

@ynbot ynbot added host: Fusion size/M Denotes a PR changes 500-999 lines, ignoring general files type: enhancement Enhancements to existing functionality labels Dec 14, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Dec 14, 2023

I like some of the descriptions, etc. but I don't really see the need for a separate creator for this.

There is nothing restricting an image family in other hosts (or other areas) to not be a sequence. As far as I've seen there also hasn't been a strict ground rule for image family defining a single image as opposed to a sequence.

I understand the potential need to different the "product types" but having just multiple 'families' being what is essentially the same thing (an image or a sequence of images) is just confusing. We should be slimming down if we can, not be expanding.

  • imagesequence
  • image
  • render
  • clip
  • plate
  • take
  • review

Personally I can hardly find the differences really. There has been so many discussions about is this an image or a render? or is it a sequence? or is it a review? a plate? or a clip?

Unless we make it so that these are literally aliases for the same publishing family (behavior is the same during publishing) but they only differentiate for the resulting product type in the database (preferably supporting a 'base' type).

What this PR does clearly shows the issue that @fabiaserra elaborated on quite a few times. E.g. their publishing logic in Houdini (or wherever?) just had a "family" dropdown menu they could pick. I feel, to some extent, that's what is going on here. Why not allow the saver to just have a drop down menu to decide between being either: "render", "image", "review"?

The amount of lines changed just for now ingesting a product that's described as an "image" as opposed to a "render" product shows the root of the issue. This literally is maintenance hell for future updates.

So, I really want to ask: Shouldn't we focus on simplifying than enlarging the ecosystem with more code that in the end does the exact same thing? This to me just shows that wherever we use "render" we should now also add "image" to the families list.


Workflows for both product types might be a bit different, this gives artists more granularity to choose better workflow.

Could you describe this - what differs in practice that it warrants this low-level separation in the code base multiplying the maintenance by two? (If not more since now the plugin itself does not dynamically reload potentially making debugging harder)

…ect_fusion.json

Co-authored-by: Jakub Ježek <jakubjezek001@gmail.com>
@kalisp kalisp added the sponsored Client endorsed or requested label Dec 14, 2023
It might be movie type not only image sequence.
…reator-plugin-single-frame-saver' into enhancement/OP-7470_Fusion-new-creator-plugin-single-frame-saver
@jakubjezek001
Copy link
Member

Unless we make it so that these are literally aliases for the same publishing family (behavior is the same during publishing) but they only differentiate for the resulting product type in the database (preferably supporting a 'base' type).

Hey @BigRoy I do agree partly with you and there is already created issue for Aliases here #2738. I beileve this PR is more feature request from our client and at the moment we do not have implemented aliases workflow so only way to do this is to create new creator.

Could you describe this - what differs in practice that it warrants this low-level separation in the code base multiplying the maintenance by two? (If not more since now the plugin itself does not dynamically reload potentially making debugging harder)

Their use case is following:
Projects have two stages in which Fusion is used: Storyboarding and Compositing. Storyboard artists heavily rely on the use of the Vriants enumerator, as it provides a wide range of predefined options. This greatly simplifies the production process by ensuring consistency in naming conventions. The same applies to Compositors, who will use a different set of variants.

Another noticeable difference is the loader product type column, where it is necessary to distinguish between render and image. Additionally, we are facing the challenge of different file formats for compositors and storyboarders. However, this is a minor issue that can be resolved with OIIO transcoding presets.

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 15, 2023

Storyboard artists heavily rely on the use of the Vriants enumerator, as it provides a wide range of predefined options.

What is the variants enumerator? What types of variants are we referring to here?


Again, I understand the need to have different sets of rules, etc. However, how specific you are getting to me it makes much more sense to start saying "consider this to be an image for storyboard" instead of image.

You're trying to make something specific but keeping it to generic for your use case.
You want to apply storyboard specific rules, on what in essence is still just an image.
As such, low level, the rules still also apply to any image that is NOT intended for storyboarding.

From Pyblish logic this usually was:

  • There is a generic "render" family - which can be a single image or sequence.
  • But this particular publish applies additional focused rules, for e.g. storyboarding. So you also apply the storyboard family.

Now it goes through the regular render logic, but can additionally apply validations SOLELY allow for storyboard (e.g. file extension validations, etc.).

The issue of course is that in OpenPype/AYON the pyblish family == render product - whereas you likely don't want that.

Anyway, note that adding this now - and changing it later will be a massive backwards compatible hurden. So I understand it's a feature request, but it's best to already focus on implementing the best we can. Or make it absolutely clear that this is a "HOTFIX" and will be deprecated over time in favor of better workflows.

If you set the first family to e.g. storyboard and the second to render then OpenPype will name it according the first. So you can still have different product names visible even if both in the backend at lower level during publishing behave as render.

@jakubjezek001
Copy link
Member

What is the variants enumerator? What types of variants are we referring to here?

image

@jakubjezek001
Copy link
Member

Anyway, note that adding this now - and changing it later will be a massive backwards compatible hurden. So I understand it's a feature request, but it's best to already focus on implementing the best we can.

Since there is not better way to approach this and this is a valid client request, we are not having another option. Also, just to note, this is an approach already inspired from Nuke so this is not so unique solution.

@fabiaserra
Copy link
Contributor

I agree with the points @BigRoy is raising although I also understand the approach and need of sometimes creating very specific semantic families that might technically look the same but it makes querying and filtering easier for some workflows (i.e. filtering by family being easier than variant or perhaps tags when they work), but we should implement that through aliases instead of growing the complexity of the "family plugin flow". For Houdini the problem was that there are families being named "arnold_rop", or "karma_rop", which doesn't make any sense and there's already a forum post about and I know @mkolar wants to change too so that's where my generic Houdini publisher addresses so they are all called simply "render".

However, we are actually creating reference, plate, render and review families because of this need of making it easier to distinguish in the DB...

Do we allow creating a single frame render family?

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 15, 2023

Do we allow creating a single frame render family?

The render family is totally fine to use for single frame outputs, yes.

However, we are actually creating reference, plate, render and review families because of this need of making it easier to distinguish in the DB...

Honestly - if you want that now.
You'd basically be doing:

reference families: `["reference", "render"]
plate families: `["plate", "render"]
render families: ["render"]
review families: `["review", "render"]

Each of these then currently - without other changes in OpenPype, behave as render but reference, plate and review will then have their subset/product name based on that first family only.

Also, filtering in the Loader I believe would filter against all of the families - not just the first. (Not entirely sure about this though). But that'd mean that a side effect of this would currently be that selecting render would list all of these.

However, nothing is holding us from instead of using render as the generic plate to have that be image instead. So that:

reference families: `["reference", "image"]
plate families: `["plate", "image"]
render families: ["render", "image"]
review families: `["review", "image"]

Then filtering to image would show all of the reference/plate/render/review but you can also selectively pick only render or plate, etc.

So it would basically be:

image
    reference
    render
    review
    plate

@fabiaserra
Copy link
Contributor

fabiaserra commented Dec 15, 2023

However, nothing is holding us from instead of using render as the generic plate to have that be image instead. So that:

reference families: `["reference", "image"]
plate families: `["plate", "image"]
render families: ["render", "image"]
review families: `["review", "image"]

Then filtering to image would show all of the reference/plate/render/review but you can also selectively pick only render or plate, etc.

Where do you define those?

Also, filtering in the Loader I believe would filter against all of the families

Yeah that would be exactly what we don't want, once filtering by family you'd want JUST that family

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 15, 2023

Where do you define those?

That'd be up to the creator to expose those families. I believe I've implemented this as an example in the USD PR recently via a get_publish_families on the Creator, e.g. like here

However since that's data you want to ALWAYS define from the creator and not store in the scene you will need to somehow manage it as transient data that does not persist, e.g. these methods are used to add the data to the instance for the publisher but remove it just before "persisting" to the scene.

There were objections for setting up this creator as it seems unnecessary. There is currently no other way how to implement customer requirement but this, but in the future 'alias' product types implementation might solve this.
@BigRoy
Copy link
Collaborator

BigRoy commented Jan 9, 2024

  • Image Saver settings default frame number (perhaps in new PR)

Could you elaborate? The output frame number? The source frame number?

  • Render Saver settings default selection of frame range enum option

We have this overridden here as well. We don't have it default to the asset frame range.

@kalisp
Copy link
Member Author

kalisp commented Jan 9, 2024

It looks like this:
image

@jakubjezek001
Copy link
Member

  • Image Saver settings default frame number (perhaps in new PR)

Could you elaborate? The output frame number? The source frame number?

  • Render Saver settings default selection of frame range enum option

We have this overridden here as well. We don't have it default to the asset frame range.

I believe the screenshot @kalisp was posting had explained it perfectly. Would that work for you?

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 10, 2024

It looks like this: image

@Innders Style-wise why is the (+) sign for the Default Variants here further indented to the left (that line and lines below it all are slightly more left than the text field entry just above it. Which I suppose is fine for the entries below, but why is the (+) misaligned which I think is part of the "Default Variants". It seems offset by 4-5 pixels?

Settings changed, so addon version should change too. 0.1.2 is in develop
…ame-saver' of https://github.com/ynput/OpenPype into enhancement/OP-7470_Fusion-new-creator-plugin-single-frame-saver

# Conflicts:
#	server_addon/fusion/server/version.py
Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

LGTM ;)

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

LGTM ;)

@jakubjezek001 jakubjezek001 self-requested a review January 12, 2024 10:03
Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

looks good again and lets merge it, right?

@kalisp kalisp merged commit 7d94fb9 into develop Jan 15, 2024
1 check passed
@kalisp kalisp deleted the enhancement/OP-7470_Fusion-new-creator-plugin-single-frame-saver branch January 15, 2024 09:32
@ynbot ynbot added this to the next-patch milestone Jan 15, 2024
"label": "Create Render Saver",
"is_group": true,
"children": [
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kalisp It seems that the OpenPype settings are lacking the implemented settings for default_frame_range_option. Should we still support the backwards compatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Fusion size/M Denotes a PR changes 500-999 lines, ignoring general files sponsored Client endorsed or requested target: AYON type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants