Conversation
jakubjezek001
added
type: enhancement
There was a problem hiding this comment.
Pull request overview
This PR updates the Resolve publish settings/schema and extraction logic to support rendering multiple representations (“render passes”) per instance, and introduces an additional representation tag intended for a “reviewable upload” workflow.
Changes:
- Extends publish profile settings to define multiple render-pass entries per product base type (plate/editorial_pkg), with shared representation attributes.
- Updates
ExtractProductResourcesto iterate over multiple output settings and optionally disable “clip source” representation integration. - Adds a new representation tag option:
webreview(“Upload as reviewable”).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| server/settings.py | Adds new settings models/structure to support multiple render-pass entries and introduces the webreview tag; defaults block was commented out. |
| client/ayon_resolve/plugins/publish/extract_product_resources.py | Updates extraction to handle multiple passes, adds optional clip source integration behavior, and normalizes new nested settings structure. |
Comments suppressed due to low confidence (1)
client/ayon_resolve/plugins/publish/extract_product_resources.py:157
get_default_settingsdoesn't include required keys likename/tags/custom_tags(andwith_handlesfor plates). When defaults are used, later code accessessettings['name']and will raise aKeyError.
def get_default_settings(self, product_base_type="editorial_pkg"):
"""Return hard-coded defaults when no matching preset is found."""
if product_base_type == "plate":
return {
"file_format": "EXR",
"codec": "RGB half (DWAA)",
"preset_path": (
"{ayon_render_presets}/clip/EXR_RGB_half_(DWAA).xml"
),
}
return {
"file_format": "QuickTime",
"codec": "H.264",
"preset_path": (
"{ayon_render_presets}/timeline/QuickTime_H264.xml"
),
"export_otio": True,
"otio_rootless": True,
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return (profile["integrate_clip_source"], result) | ||
|
|
||
| def _normalize_preset(self, preset): |
There was a problem hiding this comment.
Would it make more sense for this to return a dataclass? So that the process methods also take these dataclasses? Would probably make the code way more sane?
There was a problem hiding this comment.
tbh i only used what was already there trying to be minimally invasive.
but sure. refactor everything preset related into 1 dataclass. can do that ✌️
There was a problem hiding this comment.
Happy to do so in separate PR though @jakubjezek001 ?
There was a problem hiding this comment.
alrighty, then i'll continue to adress the notes where possible and eventually set this to be ready for review.
on that note thanks for checking this out already
There was a problem hiding this comment.
It would have to be raw class, dataclasses are failing to be importable in publish plugins.
There was a problem hiding this comment.
noted. i'll pick this up in a separate PR then or shall i adress it in here?
There was a problem hiding this comment.
Id agree with initial comment from Roy separate PR is fine for this one.
| {"value": "review", "label": "Extract review processing"}, | ||
| {"value": "delete", "label": "Delete - as intermediate"}, | ||
| {"value": "passing", "label": "Skip Extract Review"}, | ||
| {"value": "webreview", "label": "Upload as reviewable"}, |
There was a problem hiding this comment.
Does it mean that this can skip ExtractReview with this change?
This will mark the representation to be used for AYON web review. Which means that all output representations from ExtractReview will have it too (even if the the output definition in the settings won't have it).
There was a problem hiding this comment.
hm not really, this change just lets me add a tag to a representation defined in resolve's export preset.
I'm using this PR without anything configured for resolve in Extract Review. I'm completely bypassing it which is the goal of this PR.
General goal of this is to make use of resolve rendering engine as much as possible to not needing to go through
extract_reviewas this comes with performance penalties compared to resolve.
| ``file_format``, ``codec``, ``preset_path``, | ||
| and for *editorial_pkg* only: ``export_otio``, ``otio_rootless``. | ||
| """ | ||
| entity = get_current_task_entity() |
There was a problem hiding this comment.
Why this uses get_current_task_entity instead of context.data["taskEntity"]?
BTW Why was the validation removed? Is it validated ahead in validations?
There was a problem hiding this comment.
Why this uses get_current_task_entity instead of context.data["taskEntity"]?
can't really tell, this is part of the current codebase and was introduced here: 8e95826#diff-1967197894dd67b098a925d8f9b4cf1f525872550105a43ce555004a25f0e335R71
but I also don't see why this can't just be retrieved from context. @jakubjezek001 any gotchas here?
BTW Why was the validation removed? Is it validated ahead in validations?
i removed it in 72ff09d as i found this scenario to be impossible to reach.
AYON requires a task to open a DCC, so if i can't open resolve from a non-task context how can there ever be no task entity collected?
but i can just revert this commit if u want. just wanted to cut down on a few lines
There was a problem hiding this comment.
@jakubjezek001 can you comment here and resolve if you're ok? Using the context taskEntity might be better?
jakubjezek001
left a comment
jakubjezek001
left a comment
There was a problem hiding this comment.
Tested and with some improvements form bEpic-studio#1 this could be merged
…der-from-single-profile-yn Small enhancements for contribution pr#104
|
@iLLiCiTiT or @BigRoy can this be merged and released? |
BigRoy
left a comment
BigRoy
left a comment
There was a problem hiding this comment.
Mostly good to solve the pending comments, but if it works and you're happy @jakubjezek001 then I'm ok with it. Feel free to poke Robin if you really need a more in-depth review/testing.
|
@rdelillo will you have a time to test and review the code? |
|
Sure. Will test this tomorrow. |
rdelillo
left a comment
rdelillo
left a comment
There was a problem hiding this comment.
Overhall looks good, I just added a few fixes consolidation here: 9ac832cf849a64c65dec57479b94fa3080d402b3
- Fix defaults settings
settings->output_defsin default dicts - Handle settings conversion from previous
ayon-resolveversion - Default representation name to extension
- Raise when no output_def is defined in settings
@tweak-wtf feel free to merge my commit with your branch, or we can merge mine and close this one.
@jakubjezek001 would be great if you could quickly give it another try before we merge.
|
hey @rdelillo thanks for the additions :) |
|
@tweak-wtf I believe this good to merge. |
sweet... just pulled in develop ✌️ |
Changelog Description
This draft shall make it possible to render more than 1 representation per instance.
Also it aims at making source clip integration optional.
General goal of this is to make use of resolve rendering engine as much as possible to not needing to go through
extract_reviewas this comes with performance penalties compared to resolve.Additional review information
Upload as reviewabletag (really justwebreviewunder the hood)I tested this for
plateandeditorial_pkgproducts on windows only.Testing notes: