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

Publishing reviewable attributes enhancements #24

Merged

Conversation

jakubjezek001
Copy link
Member

Changelog Description

Instance attributes for reviewable items with interactivity involve two key elements for proper distribution: setting either a reviewable track or simply flagging for later extraction by transcoding tools. Either of the option will interact with the other.

Additional info

closes #21

Testing notes:

  1. Open the Hiero testing project and create a clip with the reviewable track selected.
  2. After instances are created, set the reviewable track to <none>. You'll see a new attribute for the review switch.
  3. Use the switch to activate the review. Notice that the option to select a 'reviewable track' will disappear.

@jakubjezek001 jakubjezek001 self-assigned this Nov 6, 2024
@jakubjezek001 jakubjezek001 linked an issue Nov 6, 2024 that may be closed by this pull request
@jakubjezek001 jakubjezek001 added sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition labels Nov 6, 2024
@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Nov 7, 2024

It is a littble bit weird that the checkbox hides the enum, and the same enum hides the checkbox. Only one of them should do it...

It should either always show checkbox, and in that case selection of review track is mandatory (without < none >).
Or allow to select review track with < none >, and the checkbox is shown when is not set to < none >, this solution does not make sense to me, if I choose review track, I want review, or (the checkbox is duplication of the < none > value)?

jakubjezek001 and others added 3 commits November 7, 2024 16:05
Update EnumDef label from "reviewTrack" to "reviewableTrack".

WIP on (no branch): Update EnumDef label for reviewableTrack.
…ttribute names, default values, and logic for reviewable sources.
Remove redundant reviewable flag distribution logic and update handling of review-related data in plugin files.
- Refactored logic for setting reviewable source visibility based on changes.
- Simplified logic for setting visibility of reviewable source based on review value.
Changed the comparison operator from "is not" to "!=" for checking if the review source is different from the default.
Adjusted tooltip description for selecting source of reviewable files.
@MustafaJafar
Copy link

Is this the intended result? if yes, tell me to approve.
Animation_33

@jakubjezek001
Copy link
Member Author

Is this the intended result? if yes, tell me to approve.

yes please, shane the label size of the hidden attribute is alwais breaking the look. @iLLiCiTiT is there perhaps a way to define the size of column even by hidden attributes so it is not jumpy?

@BigRoy
Copy link
Contributor

BigRoy commented Nov 11, 2024

yes please, shane the label size of the hidden attribute is alwais breaking the look. @iLLiCiTiT is there perhaps a way to define the size of column even by hidden attributes so it is not jumpy?

In this case maybe just 'disabling' instead of hiding could be sufficient?

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Nov 11, 2024

I vote for disabling it instead of changing visibility. Because I don't know how to do it with label...

Copy link
Contributor

@robin-ynput robin-ynput left a comment

Choose a reason for hiding this comment

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

Tested locally and confirmed it works with the UI update.
I agree with previous reviewers that making the toggle disable the track selection instead of hiding it would be better imo.

Also very picky, but for UI/UX consistency, should we replicate the same UI between create attributes and instance attributes selection ? Currently we have 1 combo list with < none > entry in the creator then 1 checkbox + 1 combolist in the instance UI.

client/ayon_hiero/api/plugin.py Outdated Show resolved Hide resolved
@@ -192,23 +224,36 @@ def get_instance_attr_defs(self):
)
]
if self.product_type == "plate":
instance_attributes.extend([
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure, this means we do not want to let the user edit vSyncOn and vSyncTrack on the instance anymore ?
I think I still do not get how exactly those 2 features are connected. The way I understood it was:

  • reviewableSource + review : control if a review h264 product gets created through OIIO and FFmpeg along the plate and from which source
  • vSyncOn + vSyncTrack: define the hero track (main track) to construct a vertical synchronization.. not exactly sure what come after that

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is quite confusing I agree:

vSync attributes does not need to be user editable on instances. Those changes were not propagated anywhere and also this would be quite difficult to prapagate them in first place. It is much more easy if user will recreate those instances from scratch.

Reviewable flaging is now doing following. Review attribute is saying if the plate needs to have acompanied reviwable representation and reviableSource is only saying what should be used for the representation creation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also very picky, but for UI/UX consistency, should we replicate the same UI between create attributes and instance attributes selection ? Currently we have 1 combo list with < none > entry in the creator then 1 checkbox + 1 combolist in the instance UI.

This is intentional. It would not make sense to have acctive review attribute with reviewableSource set to '< none >`.

In the PublishClip class, implemented a check to maintain backward compatibility for the "reviewableSource" tag metadata by handling the "reviewTrack" key appropriately when present in the data.
@jakubjezek001 jakubjezek001 merged commit 825e39a into develop Nov 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AY-7104_reviewable flag distribution
5 participants