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

[moonyuet]

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:

1. Launch Maya/Max
2. Create Review Instance
3. Create Screengrab in publisher tab
4. Publish

[ynbot]

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

[iLLiCiTiT]

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]

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]

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]

@iLLiCiTiT @BigRoy already implemented the changes in a4335f7

[m-u-r-p-h-y]

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]

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]

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]

• 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]

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]

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

[BigRoy]

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

We actually this same issue just now. Some more details:

Detail: duplicate key value violates unique constraint "representation_unique_name_on_version"
DETAIL:  Key (version_id, name)=(02095dc2-3eab-11ef-9ab2-6cb3114ff9ea, thumbnail) already exists..

Traceback (most recent call last):
  File "C:\Users\Maqina-02\AppData\Local\Ynput\AYON\dependency_packages\ayon_2404022047_windows.zip\dependencies\pyblish\plugin.py", line 527, in __explicit_process
    runner(*args)
  File "C:\Users\Maqina-02\AppData\Local\Ynput\AYON\addons\core_0.4.1-dev.1\ayon_core\plugins\publish\integrate.py", line 171, in process
    six.reraise(*sys.exc_info())
  File "C:\Program Files\Ynput\AYON 1.0.3-dev.1\dependencies\six.py", line 719, in reraise
    raise value
  File "C:\Users\Maqina-02\AppData\Local\Ynput\AYON\addons\core_0.4.1-dev.1\ayon_core\plugins\publish\integrate.py", line 158, in process
    self.register(instance, file_transactions, filtered_repres)
  File "C:\Users\Maqina-02\AppData\Local\Ynput\AYON\addons\core_0.4.1-dev.1\ayon_core\plugins\publish\integrate.py", line 351, in register
    op_session.commit()
  File "C:\Program Files\Ynput\AYON 1.0.3-dev.1\dependencies\ayon_api\operations.py", line 701, in commit
    self._con.send_batch_operations(
  File "C:\Program Files\Ynput\AYON 1.0.3-dev.1\dependencies\ayon_api\server_api.py", line 6575, in send_batch_operations
    raise FailedOperations((
ayon_api.exceptions.FailedOperations: Operation "30cf66d7-a46d-409d-9fff-6c91410617be" failed with data:
{
   ... removed due to confidential data ...
}
Detail: duplicate key value violates unique constraint "representation_unique_name_on_version"
DETAIL:  Key (version_id, name)=(02095dc2-3eab-11ef-9ab2-6cb3114ff9ea, thumbnail) already exists..

So, still relevant to ayon-maya. Also mentioned this in this issue here which may be related?

[BigRoy]

@dee-ynput @mkolar @iLLiCiTiT @m-u-r-p-h-y

This PR was closed due to addons separator - yet it's still relevant to Maya (and I assume also Max). The bigger question is though what the behavior should be (and it may influence other hosts as well).

In these scenarios, what do we feel should be the expected outcome?

1. In the Publisher UI the artist sets a thumbnail for the Context and publishes a review instance. Should the review instance get the thumbnail as set on the context, or use its own?
2. In the Publisher UI the artist sets a thumbnail for the Review instance and publishes the review instance. Should the review instance use that thumbnail, or still generate a thumbnail of its own?
   • If using its own, should it have a disabled thumbnail field in the Publisher UI?

@iLLiCiTiT previously said:

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.

This needs decision before implementation.

In production I've noticed that artists don't initially understand that setting the thumbnail on an instance is any different than setting it on the context. They are usually just going through publisher UI and at some point set a thumbnail (which in some cases by mistake is targeting one instance only, instead of the context which flows through to all). This may be good to keep into consideration since it may not be obvious to the artist that the thumbnail is per instance or context and they are unaware that it might be any different.

[dee-ynput]

And the reviewables add a level of complexity on what thumbnail we should see where.

We should have a global discussion about it. Pipeline+Prod track.

Then we'll know how to handle this situation.

[jakubjezek001]

Just a gentle reminder @moonyuet , this needs to be moved into dedicated repositories.