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

Convert Flame creators to new publisher #33

Open
wants to merge 46 commits into
base: develop
Choose a base branch
from

Conversation

robin-ynput
Copy link
Contributor

@robin-ynput robin-ynput commented Nov 5, 2024

Changelog Description

resolve #10
Make flame creators use the Creator API and new widget. Publishable products are now listed as individual instances following a similar logic than Hiero and Resolve.

flame_ayon

Done:

Testing notes:

In Flame:

  • Create a timeline with clips associated to movie and img-sequence media
  • From AYON Menu (right click)
  • Create and publish product(s) of type shot, audio, plate and review from there

Tested locally on Flame 2024.2

@robin-ynput robin-ynput added type: enhancement Improvement of existing functionality or minor addition sponsored This is directly sponsored by a client or community member bump minor labels Nov 5, 2024
@robin-ynput robin-ynput self-assigned this Nov 5, 2024
@robin-ynput robin-ynput marked this pull request as ready for review November 11, 2024 23:57
default_variant = "Main"

def collect_instances(self):
@staticmethod
def _get_project_workfile_filepath():
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be function somehwere in ayon_flame.api? (I have no context so maybe not)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This hopefully will remain very specific to create_workfile so I think it's better here.

To provide more context, ideally we would have used the Flame WireTap SDK to handle this, but because of this issue: #34 , we had to go with a side-car json file implementation instead.

@jakubjezek001
Copy link
Member

The PR looks solid to me. After a short testing session, I have the following notes:

  • We need to ensure the Reviewable source distribution matches the Hiero workflow 100%. Currently, it sets all vertically aligned instances as review ON, but it shouldn't. Also, we're missing an option for Clip media as a reviewable source.
  • In Hiero, if an instance is removed from the Publisher's panel, it also removes the related clip marker. In Flame, it only removes nested data from the marker. The question is whether this might confuse users, and if we should also remove the markers.

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

The PR look already great but I have to request some changes regarding the Reviewable source distribution enhancements.

})

# add reviewable source to plate if shot has it
if sub_instance_data.get("reviewTrack") != "< none >":
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, as suggested I'll look into this as a separate PR.

Comment on lines +462 to +468
EnumDef(
"reviewTrack",
label="Use Review Track",
tooltip="Generate preview videos on fly, if "
"'< none >' is defined nothing will be generated.",
items=['< none >'] + gui_tracks,
),
Copy link
Member

Choose a reason for hiding this comment

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

To make it aligned with Hiero here is also another difference https://github.com/ynput/ayon-hiero/blob/develop/client/ayon_hiero/plugins/create/create_shot_clip.py#L485-L489

Notice that the attribute is called reviewableSource for a reason. Please take a look at how the flow of the attribute is designed. It is then distributed to the publishing context and used here to determine whether we use review track or clip media for reviewable transcoding downstream. https://github.com/ynput/ayon-hiero/blob/develop/client/ayon_hiero/plugins/publish/collect_plates.py#L34-L47

This of course means that there will be lot of changes and perhaps it would be good to separate them into new PR which would be based on this PR and targeted back to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, as suggested I'll look into this as a separate PR.

Comment on lines +229 to +232
if current_sequence is not None:
gui_tracks = get_video_track_names(current_sequence)
else:
gui_tracks = []
Copy link
Member

Choose a reason for hiding this comment

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

Another distinque difference is that in Hiero we have moved from List[str] towards List[dict] so we are able to improve labeling for better user communication. Look here https://github.com/ynput/ayon-hiero/blob/develop/client/ayon_hiero/plugins/create/create_shot_clip.py#L211-L216

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, as suggested I'll look into this as a separate PR.

@robin-ynput
Copy link
Contributor Author

@jakubjezek001 I have started a draft PR aside for the review work:
https://github.com/ynput/ayon-flame/pull/36/files this is still WIP.

For the marker comment, I couldn't find any API to delete markers from within Flame hence the empty marker for now.
I looked at the API docs, the forums and the community pages... do you have any idea how this is done ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AY-6253_Conversion creators in new publisher
4 participants