SG-44786 Reference previous pipeline step publish when building a new scene (Rig, Texture) - #163
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ticket/SG-44787-validate-previous-step-published #163 +/- ##
====================================================================================
- Coverage 17.83% 17.80% -0.03%
====================================================================================
Files 32 32
Lines 3180 3184 +4
====================================================================================
Hits 567 567
- Misses 2613 2617 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| source_path=template_path, | ||
| prep_scene_callback=functools.partial(self._prep_scene, sg_publish_data), | ||
| ) | ||
| # Bound after construction so the callback can reference create_inputs |
There was a problem hiding this comment.
Can we pass this call back at construction time?
There was a problem hiding this comment.
Yes - reverted to binding at construction. Fixed in bf20af5.
| return depdata.file_path | ||
|
|
||
|
|
||
| def reference_published_workfile(revision_id: str) -> str: |
There was a problem hiding this comment.
Should we consider extract a private function for shared logic instead of create a mirror function?
There was a problem hiding this comment.
Agreed. No duplicated body. Fixed in 07f8797.
| self._reference_upstream_step(create_inputs) | ||
| self._prep_scene(sg_publish_data) | ||
|
|
||
| def _reference_upstream_step(self, create_inputs: CreateInputs) -> None: |
There was a problem hiding this comment.
_confirm_upstream_step_published and _reference_upstream_step each traverse the Flow AM hierarchy independently. The validation step's result is discarded and the traversal repeated. Should we design this carefully?
| return None | ||
|
|
||
| workfiles = node.find_children(type_id=workfile_type_id) | ||
| return workfiles[0] if workfiles else None |
There was a problem hiding this comment.
Is this safe since we allow multiple same type workfile from the same pipeline step?
There was a problem hiding this comment.
I think for now we have to take this naive approach until we better design what it means to have "multiple maya workfiles" under a given asset root. I have no doubt this criteria will need to become more complicated and maybe customizable in the future.
There was a problem hiding this comment.
Keeping the first-match (naive) approach for now per the discussion below. Documented at 9436cb7.
| root_folder_name = _get_root_folder_name(sg_entity_type) | ||
| if not root_folder_name: | ||
| return None | ||
|
|
||
| workfile_type_id = schema.get_schema_id(workfile_type) | ||
| if not workfile_type_id: | ||
| logger.warning( | ||
| f'Could not resolve the schema id for workfile type "{workfile_type}". ' | ||
| f'Skipping referencing of pipeline step "{upstream_step}".' | ||
| ) | ||
| return None |
There was a problem hiding this comment.
This code seems repeated in find_upstream_workfile() and has_published_workfile(). Feels like it can be pushed down into the _find_workfile_asset() helper. In general, it feels like these two wrappers aren't terribly necessary and you could get away with just one find_workfile_asset() function. The get_upstream_step() work can be done in the calling function. The error that gets logged can also go into the calling functions. Makes for a simpler interface - the extra functions have big long docstrings and make things feel more complex than they are. Just my 2 cents.
There was a problem hiding this comment.
Merged them into find_workfile_asset; Fixed in 9436cb7.
| return depdata.file_path | ||
|
|
||
|
|
||
| def reference_published_workfile(revision_id: str) -> str: |
There was a problem hiding this comment.
Confused why we need another function here at all? We already have a reference operation implemented. This is what we should call. What extra work or different work is this function doing? We should be able to just use the existing reference_revision(revision_id) function no?
| # TDs can override this method to add custom scene prep logic | ||
| pass | ||
|
|
||
| def _prepare_build_scene( |
There was a problem hiding this comment.
So I think we don't need this extra wrapper. I would go with one of these approaches:
- We consider this behaviour "one pipeline opinion" - not meant to be hardcoded and but rather overridable. In that case, put the implementation into the default prep_scene() callback. Clients can override as they see fit.
- We consider this behaviour be "hardcoded" into the create process and separate from the prep_scene_callback concept. In this case, we simply put the code directly into the create._create_dcc_workfile_asset() function.
I would probably opt for option 1 because I feel what we're doing is presenting a default pipeline opinion. I would avoid the complexity of creating another layer of customization in the prep scene process.
| # Bound after construction so the callback can reference create_inputs | ||
| # (which needs the fully built object to resolve the upstream scene). | ||
| create_inputs.prep_scene_callback = functools.partial( | ||
| self._prepare_build_scene, create_inputs, sg_publish_data |
There was a problem hiding this comment.
The self._prep_scene() callback already has access to sg_publish_data (which is what populates the information in the create_inputs). It should be sufficient to implement the callback without access to the create_inputs object. That way we can avoid this complexity of having to set the callback after construction.
There was a problem hiding this comment.
I believe this is the same as Ming's comment. Fixed in bf20af5.
…ding When building a new Maya scene for a downstream step, automatically reference the upstream step's published Maya workfile into the scene so it opens ready to work from. Referencing runs in the build prep callback (after the scene is created/loaded, before it is saved into the draft) and is a no-op unless the host is Maya and the upstream step actually has a published workfile. - step_validation: share the hierarchy walk via _find_workfile_asset() and add find_upstream_workfile() to resolve the upstream step's published workfile asset, returning None for every unresolved case so referencing is skipped. - reference: add reference_published_workfile(), mirroring reference_revision() without its flow_draft_id guard, since a freshly built scene has no draft context yet. - flowam_actions: wire referencing into the build prep callback, Maya-gated like the SG-44787 publish check; a reference failure warns but never aborts. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover find_upstream_workfile(): returns the upstream step's published workfile asset, and returns None with no configured upstream, an unsupported entity type, an unresolved workfile schema id, nothing published, or a Flow AM error. Co-authored-by: Cursor <cursoragent@cursor.com>
Custom loader actions defined their description but only the caption reached the UI. Surface the description as a tooltip, mirroring the built-in Refresh action (setToolTip plus driving the hovered signal, since QMenu shows no action tooltips on its own). Expand the Maya Build New Scene description to explain the new referencing/validation behavior driven by pipeline_step_dependencies. Co-authored-by: Cursor <cursoragent@cursor.com>
e77697e to
2ee69ab
Compare
Address review: drop the reference_published_workfile() mirror and instead add a require_asset_context flag to reference_revision(). Build New Scene calls it with require_asset_context=False, since the freshly created scene has no draft context yet - the only reason a separate function existed. The interactive reference action keeps the default (True), preserving its "open an asset first" guard. Co-authored-by: Cursor <cursoragent@cursor.com>
Address review: replace the find_upstream_workfile()/has_published_workfile() wrappers with one find_workfile_asset(sg_entity_type, ..., workfile_type) that resolves the root folder and workfile type internally and walks the hierarchy. It returns the workfile asset or None, and raises FlowError when the lookup cannot be performed (unknown entity type, unresolved schema id, or a query error) so callers can tell "not published" from "not checked". get_upstream_step() and the skip logging now live in the callers (find_unpublished_upstream_step and the referencing path), giving a simpler, shared traversal. Behavior of the pre-build publish warning is unchanged. Also note the deliberate "first workfile wins" simplification, per the review discussion about multiple workfiles under one step. Co-authored-by: Cursor <cursoragent@cursor.com>
Address review: drop the _prepare_build_scene()/_reference_upstream_step() wrappers and move the referencing into the default _prep_scene() callback, since this is a default pipeline opinion TDs can override rather than an extra layer of customization. The callback is bound at construction time again and works from sg_publish_data alone (deriving the pipeline step with a Task query), so there is no longer a need to set it after construction or pass create_inputs. Co-authored-by: Cursor <cursoragent@cursor.com>
This pull request introduces a robust system for automatically referencing upstream pipeline step workfiles when building a new Maya scene, along with improvements to code structure, error handling, and test coverage. The main feature ensures that when a new scene is built for a dependent step (e.g., Rig or Texture), the published scene from its upstream step (e.g., Model) is referenced in, streamlining artist workflows and reducing manual steps.