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

Enhancement: Workfile Templates add events, plugin discovery, run script and build from published workfile #5830

Closed

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Oct 28, 2023

Changelog Description

  • Implement an event-system for the Workfile Templates Builder
  • Add plug-in registry/discovery so plug-ins can be registered dynamically instead of requiring to be defined in the Host class
  • Maya: Add Run Script plugin, Add Assign Look plugin, Add Build from Publisher Workfile loader and some refactoring
  • Change in behavior: This may fix a bug where previously placeholders may sometimes load only one subset when it should have loaded more. It could thus influence the loading behavior on existing templates, see here.
  • And some cosmetics

Additional info

Events

Plug-in discovery

Other

Maya


Also see this comment here describing what might be other todos to investigate or discuss.

Testing notes:

  1. Create and build maya workfile templates (basically existing templates should continue to work)
  2. Make sure workfile templates in Nuke and After Effects still behave as intended (basically existing templates should continue to work)
  3. The Run Python Script Placeholder in Maya should work (including 'ordering' it with the order attribute)
  4. The Assign Look Placeholder in Maya should work - any contents inside the placeholder should get shaders assigned, even if e,g. the loaded content is itself loaded from a placeholder.
  5. Ordering in Maya outliner should be preserved for placeholders, also when "keep placeholders" is enabled.

@BigRoy BigRoy added type: enhancement Enhancements to existing functionality community contribution labels Oct 28, 2023
@ynbot ynbot added size/L Denotes a PR changes 1000-1499 lines, ignoring general files host: Maya labels Oct 28, 2023
@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 29, 2023

For context, this plug-in:
image

Runs as:

On populate
On depth iteration finished
<openpype.lib.events.Event object at 0x0000020A103A51E0>
On build finished
{'depth': 1, 'placeholders_by_scene_id': {'|maya_runscript': < PlaceholderItem |maya_runscript >}}

@BigRoy
Copy link
Collaborator Author

BigRoy commented Oct 29, 2023

@ClementHector could this be of interest to you for some test runs? But maybe @mre7a and @friquette have a say as well.

@BigRoy BigRoy marked this pull request as ready for review October 29, 2023 20:30
@@ -90,7 +94,9 @@ def install(self):
register_loader_plugin_path(LOAD_PATH)
register_creator_plugin_path(CREATE_PATH)
register_inventory_action_path(INVENTORY_PATH)
self.log.info(PUBLISH_PATH)

# TODO: Expose this via a dedicated `register_template_plugin_path`
Copy link
Member

Choose a reason for hiding this comment

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

I like this TODO and I think it should be separated PR, merged before this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How would you like to name the dedicated function for it?

register_workfile_template_builder_placeholder_plugin_path

Seems a bit too verbose haha.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iLLiCiTiT any naming preference on this? I can take a look at creating a separate PR for this then.

@tokejepsen
Copy link
Member

Could we resolve the merge conflicts before any testing?

I would also suggest maybe splitting this into smaller PRs.

@tokejepsen tokejepsen marked this pull request as draft January 23, 2024 08:08
@BigRoy BigRoy marked this pull request as ready for review January 29, 2024 16:17
@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 29, 2024

@tokejepsen Resolved the conflicts - should be testable again. Some features have become available in develop already which already takes quite a bit of complexity out of this PR. And I've updated the PR description for that as well - and marked one todo there that would be nice to implement outside of this PR which then makes the diff of this PR smaller in scope as well.

I've refactored usage of partial to weakref_partial @iLLiCiTiT - this was the intended use case, right? It's basically this commit: 27b89ed

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Lots of cool stuff here for the workfile builder!

I think there is a lot to test on this PR, so could you update the testing notes with different cases we should look for?

For example I see the plugins look assigner and run python script.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 30, 2024

Added testing notes to PR description - is this sufficient?

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

I think there is bugfix in this PR as well.

I've got two subsets of family model. In current develop it'll only bring in 1 model subset main, but testing this PR and it brings in both; main and unique subsets.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 6, 2024

I think there is bugfix in this PR as well.

I've got two subsets of family model. In current develop it'll only bring in 1 model subset main, but testing this PR and it brings in both; main and unique subsets.

Could very well be - there's also some fixes in how it orders things in the outliner, etc. I just 'patched' as I hit issues while testing and implementing whilst keeping in mind in the code 'original behavior' but just fixing Maya code issues that I knew could have edge cases where it failed.

So it's hoping I didn't actually break another thing! ;)

@tokejepsen
Copy link
Member

So it's hoping I didn't actually break another thing! ;)

Could we update the description, just so we are clear on release what might happen for studios updating. I think it's correct behaviour but studios might be used to a single subset being loaded. Unless the original intent was 1 subset per placeholder?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 6, 2024

Unless the original intent was 1 subset per placeholder?

As far as I know the placeholder design of Workfile Templates was always designed to behave like that.

@mkolar
Copy link
Member

mkolar commented Feb 9, 2024

Because we're splitting OpenPype into ayon-core and individual host addons, this PR would have to be re-created to target one of those.

We're closing it down, but we'll he happy for a new PR to ynput/ayon-core or the host addon repository once it's up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community contribution host: Maya port to AYON size/L Denotes a PR changes 1000-1499 lines, ignoring general files target: AYON target: OpenPype type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants