For #46368: Customizable export UI, plus additional hook methods for Shot/Cut updating. #42

thebeeland · 2018-02-20T02:58:31Z

This is implementing the idea outlined in #41 from CBSD. It's providing hook methods for building custom widgets for each processor we provide, plus allows for custom preset properties to be created, and those properties then being used to set/initialize the custom widget.

In addition, to meet the workflow requirements outlined by CBSD, additional hook methods have been added to allow for customization of if/how Shots, Cuts, and CutItems are created/updated during the export process.

NOTE: There are likely going to be use cases for custom UI elements/properties that we then don't have hooks for in the right places to control certain export functionality via those custom properties. The hiero_update_shot and hiero_update_cuts hooks added here provide the flexibility necessary to implement CBSD's custom shot processor behavior, but doesn't go beyond that.

Hooks added:

hiero_customize_export_ui:
- create_shot_processor_widget
- get_shot_processor_ui_properties
- set_shot_processor_ui_properties
- create_transcode_exporter_widget
- get_transcode_exporter_ui_properties
- set_transcode_exporter_ui_properties
- create_audio_exporter_widget
- get_audio_exporter_ui_properties
- set_audio_exporter_ui_properties
- create_nuke_shot_exporter_widget
- get_nuke_shot_exporter_ui_properties
- set_nuke_shot_exporter_ui_properties
hiero_update_shot:
- create_filesystem_structure
- update_shotgun_shot_entity
hiero_update_cuts:
- allow_cut_updates
- create_cut_item
- get_cut_thumbnail

Example custom UI based on CBSD's example:

…sort of release. Just testing things as I go.

dashielb1010 · 2018-02-23T17:53:10Z

Wow this is great!
So cool that these hooks are becoming a reality. I'm very happy you've taken the time to incorporate them. Looking forward to testing them out when my schedule frees up! -Cheers!

thebeeland · 2018-02-23T22:18:18Z

info.yml

+
+    hook_update_cuts:
+        type: hook
+        description: ""


Whoops. Missed this one. I'll get a description in!

manneohrstrom · 2018-02-26T13:47:27Z

Adding josh as well - his input will be very valuable here given that he has been working extensively both with the publish2 UI extensions and the hiero exporter.

manneohrstrom

Added what i hope are some helpful comments! I think it would be great to be able to look at the sphinx docs for this as a next step. Having a full, practical (ideally cut n paste) example for the custom UI hook would be super awesome.

manneohrstrom · 2018-02-23T21:55:23Z

hooks/hiero_customize_export_ui.py

+            built by the associated get properties hook method.
+        """
+        return
+


whitespace :)

manneohrstrom · 2018-02-23T21:58:44Z

hooks/hiero_customize_export_ui.py

+HookBaseClass = sgtk.get_hook_baseclass()
+
+
+class HieroCustomizeExportUI(HookBaseClass):


this seems like a great opportunity to build these hooks like we did the shotgun write node and the publisher, with sphinx docs. See:

http://developer.shotgunsoftware.com/tk-nuke-quickreview/

http://developer.shotgunsoftware.com/tk-multi-publish2/

First pass done. See the comment below for more detail on a few caveats.

manneohrstrom · 2018-02-26T13:52:01Z

hooks/hiero_customize_export_ui.py

+        """
+        return None
+
+    def get_shot_processor_ui_properties(self):


i think we should follow the same patterns as we do in the publisher:
http://developer.shotgunsoftware.com/tk-multi-publish2/plugin.html#tk_multi_publish2.base_hooks.PublishPlugin.get_ui_settings

So how about

create_shot_processor_widget get_shot_processor_ui_settings set_shot_processor_ui_settings

Hmm...I was just doing this, and it feels odd. Everything related to this on the Hiero/NS end of things revolves around "properties" and not "settings." It's feeling really strange mixing the two nomenclatures interchangeably. Having the getter return "settings dictionaries" feels okay, generally speaking, but then having the setter provide a list of property objects from Hiero's API and calling those "settings" doesn't feel right.

Thoughts?

I'd say stick with Hiero/NS terms

I'm leaning that way, as well.

manneohrstrom · 2018-02-26T13:55:20Z

hooks/hiero_customize_export_ui.py

+                    label="Create Cut:",
+                    key="sgCreateCut",
+                    value=True,
+                    type=bool,


Why do we need to pass type? This feels wrong to me. When is this not type(value)?

Yeah...I just tried to think of an example of where it might be different than type(value), and I can't come up with anything reasonable. I'll ditch it!

manneohrstrom · 2018-02-26T13:56:13Z

hooks/hiero_customize_export_ui.py

+
+        Example Return Data:
+
+        .. code-block:: python


do we need both a syntax example and a real example. Maybe we can skip this one and keep the actual example below?

manneohrstrom · 2018-02-26T14:01:26Z

hooks/hiero_customize_export_ui.py

+            return [
+                dict(
+                    label="Create Cut:",
+                    key="sgCreateCut",


what is this sgCreateCut - what does it signify?

I used the kwarg name from Hiero's ui property constructor, but maybe we can make it a little more obvious by calling it "property_name" or "name" instead. Label is the human-readable name that's displayed in the UI, and key is the "system" name of the property.

Changed it to "name"

I'll also change the example to use a more verbose name

manneohrstrom · 2018-02-26T14:02:34Z

hooks/hiero_update_cuts.py

+        :rtype: dict or None
+        """
+        cut_item = self.parent.sgtk.shotgun.create("CutItem", cut_item_data)
+        self.parent.logger.info("Created CutItem in Shotgun: %s" % cut_item)


will the fact that we don't batch these guys really slow things down?
Could the hook take a list of cut items and do a batch?

I can look at the underlying logic and see what might be possible. Is it too slow now? This isn't changing the logic flow other than running it from a hook.

is this new or old code? I am not sure about current performance. But if you are running say a 200 shot conform, running create 200 times will definitely be slower than running a batch. But like you say, it depends on other factors, workflow ux etc. If this is old code and we are merely updating the docs, probably better to leave things the way they are!

manneohrstrom · 2018-02-26T14:03:00Z

hooks/hiero_update_cuts.py

+        try:
+            # See if we can find a poster frame for the sequence and
+            # turn that into a usable thumbnail.
+            thumbnail = hiero_sequence.thumbnail(hiero_sequence.posterFrame())


can we provide a link to a hiero API reference here?

why not do it as part of the sphinx docs?

manneohrstrom · 2018-02-26T14:03:48Z

hooks/hiero_update_shot.py

+        Handles creating the filesystem structure for the shot that
+        was exported. The preset properties dictionary is provided to
+        allow for the lookup of any custom properties that might have
+        been defined in other hooks, like can be achieved when using


a practical example to illustrate here would be great.

manneohrstrom · 2018-02-26T14:04:29Z

hooks/hiero_update_shot.py

+        self.parent.logger.debug(
+            "Creating file system structure for %s %s..." % (entity_type, entity_id)
+        )
+        self.parent.sgtk.create_filesystem_structure(entity_type, [entity_id])


keep in mind that the self.parent.logger in hooks will require a fairly modern core.

tk-nuke already requires v0.18.45

And looking deeper, tk-hiero-export requires v0.17.21 of tk-core, which looks like it contains the parent property for Hook.

If we're doing the hook docs and base hooks, it's going to need a really new core anyway. :)

What's the minimum required for that? I assume it's newer than 0.17.21, which means I should bump up the requirement in info.yml.

I believe 2 releases ago v0.18.124

NOTES: - Old-style hooks with execute methods must use a pass instead of raising a NotImplementedError in the base_hooks module. - The base_hooks module can't live under the tk_hiero_export module, because importing that module requires access to the hiero API, which isn't available when generating docs.

thebeeland · 2018-02-27T03:14:39Z

I've added a first pass of the sphinx docs setup. As noted in the commit message, there are a couple caveats when compared to publish2 and tk-nuke-quickreview:

The execute methods in old-style hooks have to pass instead of raising NotImplementedError.
The base_hooks can't live under the tk_hiero_export module, because importing from tk_hiero_export requires access to the hiero api, which isn't available at docs build time.
The docs for most of the older are VERY minimal, so it doesn't look great. We should probably include them anyway, though, and queue up a ticket for a future sprint to do a docs pass on those older hooks.

josh-t

All looks good. Biggest comment is to consider a simple, 3 method API for the custom UI hook, then connect instances of that to whatever types you want to customize via the config.

josh-t · 2018-02-26T14:31:43Z

python/tk_hiero_export/base.py

@@ -34,6 +36,84 @@ def setApp(cls, app):
    def app(self):
        return self._app

+    def _get_custom_properties(self, get_method):
+        """


missing docstring.

josh-t · 2018-02-26T14:31:52Z

python/tk_hiero_export/base.py

+        return self._custom_property_definitions[get_method]
+
+    def _get_custom_widget(self, parent, create_method, get_method, set_method, properties=None):
+        """


missing docstring.

josh-t · 2018-02-26T14:32:12Z

python/tk_hiero_export/sg_audio_export.py

        self._displayName = "Shotgun Audio Export"
        self._taskType = ShotgunAudioExporter

+    def populateUI(self, widget, exportTemplate):
+        """


missing docstring.

josh-t · 2018-02-27T13:47:04Z

docs/index.rst

+
+- Status of associated Shot entities can be updated
+- Nuke scripts can be written into the project's filesystem structure for each Shot that's processed
+- Sequence and Shot entity data can be updated in Shotgun


Might be worth pointing out that other parent entity types can be used as well (Episodes for example)

josh-t · 2018-02-27T14:16:51Z

hooks/hiero_customize_export_ui.py

+    """
+    def create_shot_processor_widget(self, parent_widget):
+        """
+        Builds and returns a custom widget to be embedded in the parent exporter.


Worth mentioning how it will fit into the layout?

josh-t · 2018-02-27T14:25:19Z

hooks/hiero_customize_export_ui.py

+        """
+        return
+
+    def create_audio_exporter_widget(self, parent):


I was thinking about a type argument as well, but I think you'd just end up with a big conditional that called out to other methods in the hook.

One idea that could simplify things a bunch would be to define a base hook that only has 3 methods (create widget and get/set properties). This could be used for any of the types, but you associate the type/hook at the config level. So if you need a custom audio and transcode hooks, you'd configure something like this:

custom_export_ui_plugins: - type: audio hook: "{config}/hooks/tk-hiero-export/custom_audio.py" - type: transcode hook: "{config}/hooks/tk-hiero-export/custom_transcode.py"

Now you can cut down on the boilerplate and just define the API that's used for all the various types. Would that work?

josh-t · 2018-02-27T14:31:11Z

hooks/hiero_update_shot.py

+        been defined in other hooks, like can be achieved when using
+        the hiero_customize_export_ui hook.
+
+        :param str entity_type: The entity type that was created or


If we're breaking this out into a hook, I'm wondering if we should deemphasize "Shot" to some extent. Maybe something as simple as renaming the hook/class/methods to not include the word "shot".

I'm leaning towards not changing this from "shot", simply because it's more or less paired with the existing hiero_get_shot hook. We can't easily rename that hook without potentially breaking client setups that make use of it, so I'm inclined to keep get/update shot hooks sharing nomenclature.

josh-t · 2018-02-27T14:32:35Z

hooks/hiero_update_shot.py

+        self.parent.logger.debug(
+            "Creating file system structure for %s %s..." % (entity_type, entity_id)
+        )
+        self.parent.sgtk.create_filesystem_structure(entity_type, [entity_id])


If we're doing the hook docs and base hooks, it's going to need a really new core anyway. :)

josh-t · 2018-02-27T14:37:50Z

python/tk_hiero_export/shot_updater.py

@@ -254,41 +260,61 @@ def taskStep(self):
            sg_shot['task_template'] = template

        # commit the changes and update the thumbnail
-        self.app.log_debug("Updating info for %s %s: %s" % (shot_type, shot_id, str(sg_shot)))
-        self.app.tank.shotgun.update(shot_type, shot_id, sg_shot)
+        self.app.execute_hook_method(


josh-t · 2018-02-27T14:37:59Z

python/tk_hiero_export/shot_updater.py


        # create the directory structure
-        self.app.log_debug("Creating file system structure for %s %s..." % (shot_type, shot_id))
-        self.app.tank.create_filesystem_structure(shot_type, [shot_id])
+        self.app.execute_hook_method(


josh-t · 2018-02-27T14:51:02Z

One more thing: You might consider creating hook instances throughout the app and keeping them persistent rather than the old way. That way you can instantiate them with the new base classes to get any default implementation, allowing clients to only override methods they're customizing.

You should inject the base classes when calling execute_hook_method.

josh-t · 2018-02-27T16:44:56Z

Another though... sorry...

Would be nice to have some sample/test implementation (looks like you've already written) that we could use for QA. The publish2 app does something similar (see here).

manneohrstrom · 2018-02-27T21:52:54Z

Thanks for the sphinx docs pass 1! Awesome!!! Josh made some great points about the overall flow, so I'll hold off further changes for now until you have reviewed that feedback or i might confuse things :)

About the old methods vs sphinx - definitely huge value to get them into sphinx even if they count have fully featured docs like the new ones - i think it would be great if we could at least get basic params and descriptions in as part of this PR - hopefully that won't take a huge amount of time?

This is very exciting!

manneohrstrom · 2018-03-01T10:11:19Z

One more thought - for the next round of review would be great if you could zip up and attach a sphinx doc build to the PR! Just in case it's tricky to build locally. (e.g. code imports hiero stuff etc).

thebeeland · 2018-03-02T01:10:40Z

After discussing it with Josh and Rob, we're not going to go the full publish2-style setup for how the hooks are configured. Instead, we're keeping the single hook for the custom export ui, but cleaning it up a bit. I've gone through and set base classes pointing back to the base_hooks (even for the old hooks), and I've put the no-op implementation of each for the create/get/set hook methods for custom ui into the base hook. I've then emptied out the hooks/hiero_customize_export_ui.py file, and linked to the (eventual) sphinx docs page in a comment. @manneohrstrom -- If you have any better suggestions around that last bit, please let me know!

I've also added some basic docs to the older hooks, and things look much better in sphinx docs as a result.

Back to you guys! @josh-t @manneohrstrom

josh-t

👍 Looks awesome. I think you will need to bump the core requirement to v0.18.124.

josh-t · 2018-03-02T02:34:44Z

hooks/hiero_update_shot.py

+        self.parent.logger.debug(
+            "Creating file system structure for %s %s..." % (entity_type, entity_id)
+        )
+        self.parent.sgtk.create_filesystem_structure(entity_type, [entity_id])


I believe 2 releases ago v0.18.124

manneohrstrom · 2018-03-05T12:18:29Z

For reference, here are the built docs:

docs.zip

manneohrstrom

Added some comments. Looks great! Where it is easy to add, it would be great with some simple doc example of any complex data structure (e.g. dicts). Also, where we are referring to hiero API stuff, it would be great with links to the hiero api if possible!

manneohrstrom · 2018-03-05T12:22:18Z

docs/index.rst

+
+.. py:currentmodule:: base_hooks
+
+Creating custom UI elements


I think it's better that we order the hooks according to frequency - start with the ones that are most likely to be used by users, and the most advanced ones at the end!

manneohrstrom · 2018-03-05T12:23:12Z

docs/index.rst

+
+During the export process a number things can happen:
+
+- Status of associated Shot entities can be updated


i would ❤️ if there was a flow chart diagram showing the process and included all the hooks. I think that would be a perfect overview for TDs to understand what is possible. happy to help putting this together if you need help.

Agreed, we can chat offline about this.

manneohrstrom · 2018-03-05T12:27:17Z

docs/index.rst

@@ -0,0 +1,106 @@
+Shotgun Hiero Export API reference, |release|


There is something weird about the doc build - not sure where to put the comment so adding it here - the TOC seems to contain the same items twice. I just realised that we have the same issue for the nuke review docs but not for the publish. Could you investigate and potentially update the nuke review docs too?

I'll ping you offline about this.

We have an internal ticket to look at this more closely. It's not immediately obvious why the ToC is wonky.

manneohrstrom · 2018-03-05T12:28:04Z

docs/index.rst

+- Nuke scripts can be written into the project's filesystem structure for each Shot that's processed
+- Sequence and Shot entity data can be updated in Shotgun
+- Cuts can be update to include new CutItems built from the exported sequences
+


Is there good hiero API documentation? Perhaps it's worth including a link here in the intro?

manneohrstrom · 2018-03-05T12:30:46Z

app.py

@@ -50,6 +50,9 @@
    ShotgunAudioExporterUI,
    ShotgunHieroObjectBase,
 )
+
+import base_hooks


Why the absolute import? What if this app runs next another app which also imports base hooks?

We typically very very rarely do absolute imports and if we do, we name things explicitly (e.g. the import tk_hiero_export above is slightly better but still unconventional!)

See the nuke review app for another way to do this: https://github.com/shotgunsoftware/tk-nuke-quickreview/blob/master/app.py#L30

manneohrstrom · 2018-03-05T12:34:25Z

hooks/hiero_pre_export.py


-        :param processor: Processor The is being used, in case distinguishing between
-                          differnt exports is needed.
+        :param processor: The processor object that is about to be started.


what is a processor? Can we link to hiero docs?

manneohrstrom · 2018-03-05T12:36:33Z

hooks/hiero_update_cuts.py

+        :rtype: dict or None
+        """
+        cut_item = self.parent.sgtk.shotgun.create("CutItem", cut_item_data)
+        self.parent.logger.info("Created CutItem in Shotgun: %s" % cut_item)


is this new or old code? I am not sure about current performance. But if you are running say a 200 shot conform, running create 200 times will definitely be slower than running a batch. But like you say, it depends on other factors, workflow ux etc. If this is old code and we are merely updating the docs, probably better to leave things the way they are!

manneohrstrom · 2018-03-05T12:36:53Z

hooks/hiero_update_cuts.py

+        try:
+            # See if we can find a poster frame for the sequence and
+            # turn that into a usable thumbnail.
+            thumbnail = hiero_sequence.thumbnail(hiero_sequence.posterFrame())


why not do it as part of the sphinx docs?

manneohrstrom · 2018-03-05T12:39:00Z

python/base_hooks/hiero_customize_export_ui.py

+# not expressly granted therein are reserved by Shotgun Software Inc.
+
+#  UI Hook
+# ===========================


nitpick: maybe drop this header. We don't normally have these kinds of things.

manneohrstrom · 2018-03-05T12:39:30Z

python/base_hooks/hiero_customize_export_ui.py

+
+class HieroCustomizeExportUI(HookBaseClass):
+    """
+    This class defines methods that can be used to customize the UI of the various


could we include a screenshot to illustrate as part of the docs?

Done. Also added some overview sphinx docs for customizing UI. Basic stuff, but it's a good, brief intro with an example screenshot.

…tk-hiero-export into 46368_custom_ui

…cessors. Various bug fixes.

Jeff Beeland added 2 commits February 16, 2018 18:11

WIP -- includes some custom test logic that won't be included in any …

d74d815

…sort of release. Just testing things as I go.

First pass at complete implementation of customizable UI elements.

aab2f45

thebeeland requested a review from manneohrstrom February 20, 2018 02:58

thebeeland commented Feb 23, 2018

View reviewed changes

info.yml Outdated

hook_update_cuts:

type: hook

description: ""

Copy link

Contributor Author

thebeeland Feb 23, 2018

Choose a reason for hiding this comment

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

Whoops. Missed this one. I'll get a description in!

manneohrstrom requested a review from josh-t February 26, 2018 13:46

manneohrstrom suggested changes Feb 26, 2018

View reviewed changes

Jeff Beeland added 3 commits February 26, 2018 18:04

Addresses most CR comments. More to come.

0c56d41

Adds and init for the base_hooks module.

bf46467

Corrects bugs introduced during CR changes.

7727f63

josh-t suggested changes Feb 27, 2018

View reviewed changes

Jeff Beeland added 2 commits February 28, 2018 17:29

Docstring updates for all of the older hooks.

760752d

Docs tweaks.

c99acc3

Jeff Beeland added 2 commits March 1, 2018 15:54

Fills in a couple missing docstrings.

d43ad12

Pass at cleaning up the hooks/base_hooks structure.

8137972

Adds a missing docstring.

61c99ae

josh-t reviewed Mar 2, 2018

View reviewed changes

josh-t approved these changes Mar 2, 2018

View reviewed changes

manneohrstrom approved these changes Mar 5, 2018

View reviewed changes

Jeff Beeland added 2 commits March 5, 2018 15:11

Refactors base_hooks imports. Docs tweaks.

ac541a0

Merge branch '46368_custom_ui' of https://github.com/shotgunsoftware/…

91e4851

…tk-hiero-export into 46368_custom_ui

Jeff Beeland added 3 commits March 5, 2018 17:12

Sphinx docs tweaks.

307d7de

Sphinx docs polish.

f095ba8

Adds tests fixture hook that includes custom UI for all exporters/pro…

704f3d5

…cessors. Various bug fixes.

thebeeland merged commit 3b6f442 into master Mar 14, 2018

thebeeland deleted the 46368_custom_ui branch March 14, 2018 22:25

thebeeland mentioned this pull request Mar 14, 2018

Added a possible approach to opening up the UIs in the exporter to a … #41

Closed

		HookBaseClass = sgtk.get_hook_baseclass()


		class HieroCustomizeExportUI(HookBaseClass):


		During the export process a number things can happen:

		- Status of associated Shot entities can be updated

		@@ -0,0 +1,106 @@
		Shotgun Hiero Export API reference, \|release\|

For #46368: Customizable export UI, plus additional hook methods for Shot/Cut updating. #42

For #46368: Customizable export UI, plus additional hook methods for Shot/Cut updating. #42

Conversation

thebeeland commented Feb 20, 2018 • edited

dashielb1010 commented Feb 23, 2018

Choose a reason for hiding this comment

manneohrstrom commented Feb 26, 2018

manneohrstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manneohrstrom Feb 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebeeland commented Feb 27, 2018

josh-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebeeland Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh-t commented Feb 27, 2018 • edited

josh-t commented Feb 27, 2018

manneohrstrom commented Feb 27, 2018

manneohrstrom commented Mar 1, 2018

thebeeland commented Mar 2, 2018

josh-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manneohrstrom commented Mar 5, 2018

manneohrstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebeeland commented Feb 20, 2018 •

edited

manneohrstrom Feb 26, 2018 •

edited

thebeeland Mar 1, 2018 •

edited

josh-t commented Feb 27, 2018 •

edited