-
Notifications
You must be signed in to change notification settings - Fork 67
Auto-tag instances with review family when review representation added #1617
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
base: develop
Are you sure you want to change the base?
Auto-tag instances with review family when review representation added #1617
Conversation
Ensures that instances with added review representations are properly tagged with the "review" family for downstream processing.
| if "delete" in tags and "thumbnail" not in tags: | ||
| instance.data["representations"].remove(repre) | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this. Could we at least add a comment why we add review family?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about this too. It may enforce this on things that are not "review" product types where a DCC may not currently be expecting it. I'm not saying having "review" family be wider isn't a good idea - it's just that with DCC-specific extractors triggering on "review" family I wonder if this really is the best approach if those remain as is.
A quick search shows review family on all of:
client/ayon_core/plugins/publish/integrate_attach_reviewable.py
client/ayon_core/plugins/publish/extract_slate_data.py
client/ayon_core/plugins/publish/extract_review_slate.py
client/ayon_core/plugins/publish/extract_review.py
client/ayon_core/plugins/publish/extract_otio_review.py
client/ayon_core/plugins/publish/extract_burnin.py
client/ayon_core/plugins/publish/collect_audio.py
client/ayon_tvpaint/plugins/publish/validate_layers_visibility.py
client/ayon_tvpaint/plugins/publish/extract_sequence.py
client/ayon_tvpaint/plugins/publish/collect_render_instances.py
client/ayon_tvpaint/plugins/publish/collect_instance_frames.py
client/ayon_traypublisher/plugins/publish/collect_review_frames.py
client/ayon_traypublisher/plugins/publish/collect_editorial_reviewable.py
client/ayon_photoshop/plugins/publish/extract_sources_review.py
client/ayon_photoshop/plugins/publish/collect_version.py
client/ayon_nuke/plugins/publish/extract_review_intermediates.py
client/ayon_nuke/plugins/publish/extract_review_data.py
client/ayon_maya/plugins/publish/validate_review.py
client/ayon_maya/plugins/publish/extract_thumbnail.py
client/ayon_maya/plugins/publish/collect_review.py
client/ayon_houdini/plugins/publish/collect_review_data.py
client/ayon_cinema4d/plugins/publish/validate_frame_range.py
client/ayon_cinema4d/plugins/publish/extract_review.py
client/ayon_blender/plugins/publish/validate_frame_range.py
client/ayon_blender/plugins/publish/extract_thumbnail.py
client/ayon_blender/plugins/publish/extract_playblast.py
client/ayon_blender/plugins/publish/collect_review.py
client/ayon_maya/plugins/publish/extract_playblast.py
Of those the addons that run 'after' are:
- Extract Burnin
- Extract Review
- Extract Slate Data in Nuke
- Extract Review with Slate Frame in Core (if it also has 'slate')
Which may not seem problematic much - but it starts getting confusing if e.g. some DCC specific plug-in for review was intended to be merely for "review" product type. Which there are, but luckily at an earlier order.
Can you elaborate on why you'd want this? Is it so that ExtractReview triggers? would it make sense to instead have ExtractReview ALWAYS run - but only ever consider representations with "review" tags or alike?
Or should we define a dedicated 'family' like has_reviewables to explicitly target instead?
It's a common problem due to how DCC plug-ins are implemented directly on the product type with review instead of e.g. creator.review or productType.review or whatever to be able to differentiate whether something is just marked as 'consider me for reviews' instead of 'this is a review product type in Maya'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This had been added for reason and specific case requested by client. They need it for plate product within editorial workflow. This will save their time and we can bypass Reviewable representation creation by OpenTimelineIO plugins. But missing review family on those instances which were not letting reviewable activated representations to be published into web-review. This is really needed for the case and I was actually surpriced we did not have already.
What would be the reason anybody would add review tag into OIIO transcode preset and the representation would not be processed via downstream reviewable plugins? Seriously, this was the missing bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is the need for review family.
- We have some cases where
reviewrefers to the product type. - And others where it's a case of "apply review to me".
We need to start differentiating between the two. I'm not saying this PR is that scenario - but I do feel it's important to start solving that. @iLLiCiTiT thoughts?
For now, if this doesn't break anything - I'm fine with merging. But I agree that a dedicated comment in the code here is helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to know why exactly this is added, for what plugins, what use-case. This code changes, out of the context, don't make sense and looks (and maybe is) dangerous, so we really should add more information why it was added.
I do understand that there is a reason, but I don't see it, meaning nobody will see it, in half an year nobody, even you, won't know what is this for and it will probably block us from other changes or we will need to remove it because it does cause other issues. Without comment we will just do it which might break something.
Because of that I would rather add changes to hosts which are affected by this instead of it being here. Right now, if I understand correctly, it adds "review" to families here to not trigger extract oiio transcode?
Which raise multiple other questions, for example why that case is handled in this plugin, and can't we do that the other way, that extract oiiot transcode will skip those instances in specific cases instead?
Changelog Description
Instances that receive review representations through OIIO color transcoding are now automatically tagged with the "review" family. This resolves processing gaps where review-enabled instances weren't properly identified downstream, causing batch delivery workflows to fail when review representations were expected but the family tag was missing.
The enhancement ensures consistent family tagging throughout the publish pipeline by automatically appending "review" to the instance families list whenever a review representation is added during color transcoding operations.
Additional info
The fix modifies
extract_color_transcode.pyto detect whenadded_reviewflag is set and the "review" family isn't already present ininstance.data["families"]. When both conditions are met, it appends "review" to the families list, ensuring proper downstream identification and processing.This change affects the
ExtractOIIOColorTranscodeplugin's process method, occurring after representation cleanup and before new representations are added to the instance data.Testing notes: