Chore: Make sure implicit thumbnail in higher priority over the thumbnail source

moonyuet commented 1 week ago

Changelog Description

When users trying to create screengrab thumbnails and publish along with the review product type, it will error out the intgerate.py due to the duplicate of the thumbnail representation data. This PR is to make sure the extract thumbnail is always at the higher priority than the extract thumbnail sources

Additional info

please test on some other hosts with thumbnail extractors in review family, maybe similar bug is hit when you publish both screengrab and review family

Testing notes:

Launch Maya/Max
Create Review Instance
Create Screengrab in publisher tab
Publish

ynbot commented 1 week ago

Task linked: AY-5562 Maya: duplicate thumbnail integration error

iLLiCiTiT commented 1 week ago

Wouldn't be better to check if already has thumbnail source instead of changing order?

E.g. if you drag & drop thumbnail to instance before publish, it is already known...

BigRoy commented 1 week ago

Wouldn't be better to check if already has thumbnail source instead of changing order?

I wanted to say something similar - this feels like we're hiding the root cause of the issue somehow. It seems like you're trying to offset it compared to this plug-in and that maybe with your change that now we're not publishing the added screenshot in publisher at all but are forcing the max and maya extractors to take precedence - which sounds like the opposite of what we want as well?

Shouldn't whatever the user provides as thumbnail override take precedence no matter what?

iLLiCiTiT commented 1 week ago

are forcing the max and maya extractors to take precedence - which sounds like the opposite of what we want as well?

They're taking precedence? In that case it is not right. Dropped thumbnails to publisher should be used if are available over automatically generated thubmanils.

moonyuet commented 1 week ago

@iLLiCiTiT @BigRoy already implemented the changes in a4335f7

m-u-r-p-h-y commented 1 week ago

Shouldn't whatever the user provides as thumbnail override take precedence no matter what?

definitively not.

If there are automatically generated thumbnails from image sequences (playblast etc.) this should have priority over user provided one. That should only be used for any other cases where automatically generated ones are unavailable.

for example:

BigRoy commented 1 week ago

definitively not.

If there are automatically generated thumbnails from image sequences (playblast etc.) this should have priority over user provided one. That should only be used for any other cases where automatically generated ones are unavailable.

I disagree and personally agree with @iLLiCiTiT here. If the user explicitly provides a thumbnail they are doing that for a reason to set that explicitly. What's the use case where someone provides a thumbnail explicitly but wants it ignored/discarded regardless? (What obvious thing am I missing?)

m-u-r-p-h-y commented 1 week ago

The original problem this PR should solve is a bug when an instance ends up with two thumbnails (for example review instance, with explicitly set thumb) This should be fixed

Detail: duplicate key value violates unique constraint "representation_unique_name_on_version"
DETAIL:  Key (version_id, name)=(9ce0cbb0-32ec-11ef-9424-3003c82d0673, thumbnail) already exists..

Other discussions about the thumbnails

it is not clear from the publishing window which instance will generate its own thumbnail and which not (review, even maya workfile! etc.)
because it is not clear from the above point some users may think it is expected for them to provide the thumbnail
if you set Context thumbnail which seems like a parent of all instances, should it override all thumbnails of all instances?

to fix all the doubts I would suggest making it clear if the thumbnail will be generated automatically and only then allowing users to override it (so the screengrab window will only appear after my informed decision I need to override it)

Context thumbnail, should only update instances without any thumbnail (not all children)

BigRoy commented 1 week ago

if you set Context thumbnail which seems like a parent of all instances, should it override all thumbnails of all instances?

Ah, yes - that's a valid point. In the case you add the thumbnail to the context as opposed to the instance then maybe the expected behavior differs because then I might expect the instance implicit thumbnail to take preference. @iLLiCiTiT thoughts?

iLLiCiTiT commented 5 days ago

if you set Context thumbnail which seems like a parent of all instances, should it override all thumbnails of all instances?

No it should not, but I don't think it does. If you set thumbnail on context it does use the thumbnail only if instance does not have any (if it is not true then we should fix that). But if user explicitly set thumbnail on instance, it should prevent to create the auto-generated one.

@moonyuet please wait until this is resolved... This needs decision before implementation.

iLLiCiTiT commented 17 hours ago

Please move PR to https://github.com/ynput/ayon-maya

ynput / ayon-core