Local render AOVs review frameStartHandle and frameEndHandle fix #77

timsergeeff · 2024-08-26T20:59:20Z

Changelog Description

when we create new instances for each aov code forgets to add frameStartHadnle and frame EndHandle for new instance so new representation will not publish

Additional info

My setup was houdini+redhift+animated focal lenght so code fail here

Testing notes:

try creating project with animated focal lenght
try to publish
see error

It turns out that code dont parse frameStartHadnle and frame EndHandle data to newly created per aov instance so code fails

BigRoy

Thanks @timsergeeff

I pushed some code cosmetic changes.

@MustafaJafar can you review whether this fixes the issue?

Ensure you can reproduce the bug without the PR
Ensure this PR fixes the bug and still produces frame ranges and frame range data on the published renders as should be intended.

Note that frameStart and frameEnd include the handles (that was already the case) and handleStart and handleEnd are not propagated to the AOV instances. I have no idea whether rendering to the farm does preserve the "handles" data for renders, but I'm quite sure that local rendering will never use the current folder/task entity's handles even if CollectAssetHandles is enabled for the instance. Even though that would then be an existing bug - it may be worth also testing that now so we can create a follow up issue to track that if it's an issue.

moonyuet · 2024-08-28T05:48:46Z

I tested with some weird frame range data.
991 as frame start, 1004 as frame end
1 as handle start and 4 as handle end
The calculation is correct

MustafaJafar · 2024-08-28T09:45:34Z

Side note:
Could you please pick a recommended branch name ?
Although, develop works fine but it's not recommended.

MustafaJafar · 2024-08-28T11:51:56Z

client/ayon_houdini/plugins/publish/collect_local_render_instances.py

+            families = ["render.local.hou"]
+            if instance.data.get('review'):
+                families.append("review")


I'm not sure what do these lines fix ?

For information:
CollectLocalRenderInstances was made to mimic what happens on the farm.
if you submitted any render to farm via AYON, check the created json file, you will find that all instances have family review but not all of them have the tag review. and plugins should be able to filter based on the family and the tag.
Here's one from my side mantra_ropTest_metadata.json

Therefore, I'd say let's revert this part.

Feel free to take over and push into the branch. As maintainer of the repo you should be able to push into the branch of the PR.

I got an error about failed to push.
and got this hint:

hint: Updates were rejected because a pushed branch tip is behind its remote

and I'll avoid any push/pull commands because this PR is from a develop branch!

@timsergeeff can you report what issue you faced with the review family still being present for an instance that had instance.data["review"] disabled?

According to my testings: wo this code change all of files produced by (in my case) redshift rop will be getting review tag nomatter do i swith review toggle or not because when we create new instance per render product from reddhift_rop family we always tag it as review so even if i have several rops and only one with review turn on we get all products from all rops to have revirew and in my case i have a ton of posts in kitsu from all redhist rops

and I'll avoid any push/pull commands because this PR is from a develop branch!

sorry git is pretty new for me how can i change branch and what exactley is a problem? (just for my future understanding)

According to my testings: wo this code change all of files produced by (in my case) redshift rop will be getting review tag nomatter do i swith review toggle or not because when we create new instance per render product from reddhift_rop family we always tag it as review so even if i have several rops and only one with review turn on we get all products from all rops to have revirew and in my case i have a ton of posts in kitsu from all redhist rops

@MustafaJafar can you test this scenario and figure out what is causing that instead? Is it that Kitsu logic may be disregarding whether representations have a 'review' tag? or?

Also, to me personally the code that @timsergeeff makes sense - why would it need review family if instance.data["review"] = False is what I wonder.

According to my testings: wo this code change all of files produced by (in my case) redshift rop will be getting review tag nomatter do i swith review toggle or not because when we create new instance per render product from reddhift_rop family we always tag it as review so even if i have several rops and only one with review turn on we get all products from all rops to have revirew and in my case i have a ton of posts in kitsu from all redhist rops

As I've mentioned earlier #77 (comment).
I'm only matching the results of the farm scripts, and please check the provided json file. and I believe it deserves its own PR because this PR was made to solve a particular issue.

FYI, we have review family and review tag..
I don't know why farm publishing add review family to all of the AOVs. but that's totally a different issue in my opinion.

and I'll avoid any push/pull commands because this PR is from a develop branch!

sorry git is pretty new for me how can i change branch and what exactley is a problem? (just for my future understanding)

you need to create a new branch. but maybe next time. please refer to PR name recommendations https://community.ynput.io/t/i-m-new-to-ayon-where-to-start/1070#how-to-collaborate-as-a-developer-14

for reference, it should actually be as the suggestion below, similar to setting the review tag in the plugin.
keep in mind instance.data.get('review') will still add the review family for all the AOV instances. unlike the preview variable should only pick the AOVs that follows the regex pattern specified in the settings.

Suggested change

families = ["render.local.hou"]

if instance.data.get('review'):

families.append("review")

families = ["render.local.hou"]

if preview:

families.append("review")

MustafaJafar

Adding frameStartHandle and frameEndHandle works.

For reference:
There's also an alternative fix to add a fallback value here in CollectHoudiniReviewData.

MustafaJafar

Could we apply these changes ?

MustafaJafar · 2024-08-28T12:14:20Z

client/ayon_houdini/plugins/publish/collect_local_render_instances.py

+            families = ["render.local.hou"]
+            if instance.data.get('review'):
+                families.append("review")


Suggested change

families = ["render.local.hou"]

if instance.data.get('review'):

families.append("review")

MustafaJafar · 2024-08-28T12:14:39Z

client/ayon_houdini/plugins/publish/collect_local_render_instances.py

                "productType": product_type,
                "family": product_type,
                "productName": product_name,
                "productGroup": product_group,
-                "families": ["render.local.hou", "review"],
+                "families": families,


Suggested change

"families": families,

"families": ["render.local.hou", "review"],

sorry i dont get it why we dont want peview toggle to work?
or maybe we need to be in separate PR?

We do want the behavior to work - the question is whether your fix is the 'correct' approach to resolving it or that it's maybe hiding another issue in another area.

ohh sorry now i see

but i feel like transfering families cant be bad or worse then hardcoding them

and i understand that somewhere we have tags but as someone that thee mount in setting everything up i met tags so rare (maybt i'm just dumb haha) but famalies is everywhere so kitsu just ignoring tags and using unly familyes as i remember when i was digging in collect kitsu family

If that's the case in Kitsu - then admittedly the bug is in Kitsu. Because there may be multiple representations of which only one is a reviewable - in such case the other should still be ignored. ;)

Anyway, PRs to Kitsu are welcome if you know how to fix that.

i want to add product name filtering! but i think that here changes i make nessasry to be here because why do we need families that was not in original instance?

MustafaJafar · 2024-09-07T01:44:44Z

Hey, @timsergeeff
Thanks a lot for your contribution.
I'm closing this PR in favor of another PR #93 I've just created which pick few commits from your PR (which keeps your contribution to the repo)..
Regarding the issue of the review family, we still need a debug session to

Explore potential bugs that resultant of having the additional review family.
Explore potential solutions.
Test that locally and on farm.

timsergeeff added 2 commits August 26, 2024 23:53

frameStartHandle and frameEndHandle to insnace fix

63c14a7

review tag not to be hardcoded

a0412e8

BigRoy changed the title ~~frameStartHandle and frameEndHandle to insnace fix~~ Local render AOVs review frameStartHandle and frameEndHandle fix Aug 27, 2024

BigRoy self-assigned this Aug 27, 2024

BigRoy added the type: bug Something isn't working label Aug 27, 2024

BigRoy added 3 commits August 27, 2024 11:44

Remove debug logs

d8650fd

Cosmetics

5a34b2b

Cosmetics - remove frame data closer to other frame data

6974d19

BigRoy requested review from MustafaJafar and BigRoy August 27, 2024 09:47

BigRoy approved these changes Aug 27, 2024

View reviewed changes

BigRoy requested a review from moonyuet August 27, 2024 21:19

moonyuet approved these changes Aug 28, 2024

View reviewed changes

MustafaJafar added the community Issues and PRs coming from the community members label Aug 28, 2024

MustafaJafar reviewed Aug 28, 2024

View reviewed changes

MustafaJafar requested changes Aug 28, 2024

View reviewed changes

Merge branch 'develop' into develop

1308ab8

timsergeeff requested a review from MustafaJafar August 30, 2024 09:28

Merge branch 'develop' into develop

8cffb2e

MustafaJafar mentioned this pull request Sep 7, 2024

Bugfix: local render remove redundant keys and add missing keys #93

Merged

MustafaJafar closed this Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local render AOVs review frameStartHandle and frameEndHandle fix #77

Local render AOVs review frameStartHandle and frameEndHandle fix #77

timsergeeff commented Aug 26, 2024 •

edited

Loading

BigRoy left a comment

moonyuet commented Aug 28, 2024

MustafaJafar commented Aug 28, 2024 •

edited

Loading

MustafaJafar Aug 28, 2024 •

edited

Loading

BigRoy Aug 28, 2024

MustafaJafar Aug 28, 2024

BigRoy Aug 28, 2024

timsergeeff Aug 28, 2024

timsergeeff Aug 28, 2024

BigRoy Aug 28, 2024

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar left a comment

MustafaJafar left a comment •

edited

Loading

MustafaJafar Aug 28, 2024

MustafaJafar Aug 28, 2024

timsergeeff Aug 28, 2024 •

edited

Loading

BigRoy Aug 28, 2024

timsergeeff Aug 29, 2024 •

edited

Loading

BigRoy Aug 29, 2024

timsergeeff Aug 29, 2024

MustafaJafar commented Sep 7, 2024

	families = ["render.local.hou"]
	if instance.data.get('review'):
	families.append("review")

	"families": families,
	"families": ["render.local.hou", "review"],

Local render AOVs review frameStartHandle and frameEndHandle fix #77

Local render AOVs review frameStartHandle and frameEndHandle fix #77

Conversation

timsergeeff commented Aug 26, 2024 • edited Loading

Changelog Description

Additional info

Testing notes:

BigRoy left a comment

Choose a reason for hiding this comment

moonyuet commented Aug 28, 2024

MustafaJafar commented Aug 28, 2024 • edited Loading

MustafaJafar Aug 28, 2024 • edited Loading

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

MustafaJafar Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

MustafaJafar Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

MustafaJafar Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

MustafaJafar left a comment

Choose a reason for hiding this comment

MustafaJafar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsergeeff Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsergeeff Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaJafar commented Sep 7, 2024

timsergeeff commented Aug 26, 2024 •

edited

Loading

MustafaJafar commented Aug 28, 2024 •

edited

Loading

MustafaJafar Aug 28, 2024 •

edited

Loading

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar Sep 3, 2024 •

edited

Loading

MustafaJafar left a comment •

edited

Loading

timsergeeff Aug 28, 2024 •

edited

Loading

timsergeeff Aug 29, 2024 •

edited

Loading