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

Support new flipbook node for reviews #167

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Nov 8, 2024

Changelog Description

resolve #85

This PR enhances the current review plugins to add support for the new flipbook node.
image

Additional Info

Flipbook node behaves a lot similar to opengl node.
one interesting change is the Color Correction setting which can choose (color space) or (display and view).
which may change a bit the color space validation and collection to properly support the flipbook node.

Testing notes:

  1. Create review instance using Review creator
  2. Try different node types
  3. Publish Reviews

@MustafaJafar MustafaJafar added the sponsored This is directly sponsored by a client or community member label Nov 8, 2024
@MustafaJafar MustafaJafar self-assigned this Nov 8, 2024
@MustafaJafar
Copy link
Contributor Author

Should this PR adds support for publishing reviews from LOPs?
Adding this for reference ynput/OpenPype#4890

@BigRoy
Copy link
Contributor

BigRoy commented Nov 8, 2024

Should this PR adds support for publishing reviews from LOPs?
Adding this for reference ynput/OpenPype#4890

I'd say that yes, we should fix that... but it doesn't need to be part of this PR. Instead I see that irrelevant to this because it's already relevant to the OpenGL ROP too. I thought we had an issue for that, but I can't find it 🤦

@MustafaJafar
Copy link
Contributor Author

I thought we had an issue for that, but I can't find it 🤦

We have this one #23

@BigRoy
Copy link
Contributor

BigRoy commented Nov 10, 2024

I thought we had an issue for that, but I can't find it 🤦

We have this one #23

Yup, saw that one - but unfortunately it's not about supporting cameras IN LOPS - but that is requesting to support lopimportcamera which is still at geometry level.


Created issue: #170

@BigRoy
Copy link
Contributor

BigRoy commented Nov 13, 2024

Let's see if we can get this one #148 merged first - so that this PR is slightly simplified and works with the changes made there.

@moonyuet
Copy link
Member

I slightly tested on the PR and find out that it's not working for the elder version of Houdini. Shall we introduce the condition so that users would know it only supports for Houdini 20.5? Or we can do it with some scripts in elder version?
I find a simple script from this website for creating flipbook animation for the elder version of Houdini: https://www.sidefx.com/docs/houdini/anim/flipbook.html

@BigRoy
Copy link
Contributor

BigRoy commented Nov 13, 2024

I slightly tested on the PR and find out that it's not working for the elder version of Houdini. Shall we introduce the condition so that users would know it only supports for Houdini 20.5?

We could enforce the enabled state of the Creator to False in an older version of Houdini than 20.5?

Or we can do it with some scripts in elder version?
I find a simple script from this website for creating flipbook animation for the elder version of Houdini: https://www.sidefx.com/docs/houdini/anim/flipbook.html

No please. That flipbook would just be the OpenGL review we already have.

{"ociocolorspace": review_color_space}
)

log.debug(
"'OCIO Colorspace' parm on '{}' has been set to "
"the view color space '{}'"
.format(opengl_node, review_color_space)
.format(node.path(), review_color_space)
Copy link
Member

Choose a reason for hiding this comment

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

just wondering why you don't use node (hou.Node) here as the previous code used opengl_node which happened to be hou.Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for consistency, I turned this one into node.path() instead of just node
other log messages are using node.path()

@moonyuet
Copy link
Member

moonyuet commented Nov 19, 2024

When testing in 19.5, the flipbook option does not show up as expected. I will test on 20.5 later after the download
image

Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

I tested with 20.5 and find out that the repair action is not repairing the colorspace param as expected and the validator keeps blocking me to publish.
The video would show you why and it is related to the regex form of the ocio colorspace in houdini i believe. i.e. '<USE_DISPLAY_NAME>'

validate_colorspace_repair_issue_in_flipbook.mp4

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Nov 19, 2024

The video would show you why and it is related to the regex form of the ocio colorspace in houdini i believe. i.e. '<USE_DISPLAY_NAME>'

I remember Roy mentioned it somewhere!
Unfortunately, I don't know how to replicate it. I can only find the token here in the code.

Also from your screen record, I can find the value of the colorspace is set to the display.
image
Here's how the same colorspace name appear in the list.
image

@moonyuet
Copy link
Member

The video would show you why and it is related to the regex form of the ocio colorspace in houdini i believe. i.e. '<USE_DISPLAY_NAME>'

I remember Roy mentioned it somewhere! Unfortunately, I don't know how to replicate it. I can only find the token here in the code.

I re-test again and it seems that I haven't turned on the color configuration in Ayon setting, meaning that It only happens when the OCIO color management is not enabled and using the default Houdini ocio configuration. @BigRoy, just want to hear your thought on this. We can create an issue to resolve this later(I dont think it should be solved here but instead the separate PR.)

@moonyuet moonyuet self-requested a review November 20, 2024 09:55
Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

Publish successfully in Houdini 20.5
image

@MustafaJafar
Copy link
Contributor Author

I re-test again and it seems that I haven't turned on the color configuration in Ayon setting, meaning that It only happens when the OCIO color management is not enabled and using the default Houdini ocio configuration. @BigRoy, just want to hear your thought on this. We can create an issue to resolve this later(I dont think it should be solved here but instead the separate PR.)

I was able to reproduce it in develop branch with opengl node.
image

Actually, I can pass the validation when choosing something manually from the drop down menu. (even if I disable the color management toggle in Houdini addon settings)
image

@MustafaJafar MustafaJafar added the type: enhancement Improvement of existing functionality or minor addition label Nov 20, 2024
@MustafaJafar MustafaJafar merged commit fb03bd0 into develop Nov 20, 2024
1 check passed
@moonyuet
Copy link
Member

I re-test again and it seems that I haven't turned on the color configuration in Ayon setting, meaning that It only happens when the OCIO color management is not enabled and using the default Houdini ocio configuration. @BigRoy, just want to hear your thought on this. We can create an issue to resolve this later(I dont think it should be solved here but instead the separate PR.)

I was able to reproduce it in develop branch with opengl node. image

Actually, I can pass the validation when choosing something manually from the drop down menu. (even if I disable the color management toggle in Houdini addon settings) image

But the users may expect that the repair action would fix it. We can actually inform the users somewhere in the docs to talk about this limitation, and we need to figure out what to do to fix it eventually if there is somebody who runs on this issue.

@MustafaJafar MustafaJafar deleted the enhancement/85-ay-6687_houdini-205-support-new-flipbook-node-for-reviews branch November 20, 2024 10:50
@MustafaJafar
Copy link
Contributor Author

I've created an issue for it.
#179

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-6687_Houdini 20.5: Support new "Flipbook" node for reviews
3 participants