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

Use rop.opengl family instead of review #148

Merged
merged 7 commits into from
Nov 13, 2024

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Oct 29, 2024

Changelog Description

Use rop.opengl family instead of review in review plugins that expect only a review using opengl nodes.
Also, remove unnecessary logic that is used to skip instances that are not using opengl.

Additional review information

This PR is used as a show case to test ynput/ayon-core#994
without it the attributes applied to rop.opengl won't be visible in the publisher. in this case it'd be the validate

with develop branch with the core PR
image image

Testing notes:

  1. Launch Houdini via AYON.
  2. Create a render (with review toggle enabled) and review instances.
  3. Publish, (You shouldn't notice any difference, other than opengl plugins only work with Review (OpenGL) products.)

@MustafaJafar MustafaJafar added the type: enhancement Improvement of existing functionality or minor addition label Oct 29, 2024
@MustafaJafar MustafaJafar self-assigned this Oct 29, 2024
@BigRoy
Copy link
Contributor

BigRoy commented Oct 29, 2024

This PR is used as a show case to test ynput/ayon-core#973
e.g. if your core addon is switched to the mentioned PR, then the Validate Review Colorspace toggle will only be found on review instances. (I'm an idiot, it will be the same if your core is switched to develop as review toggle is only affecting the run time AOV instance created by render.)

Actually - I don't think you are an idiot. I'm pretty sure you'll need that PR for the optional toggle for the review e.g. to show up for the instance, because without that PR it would not - but would by default only list it for if family == product type.

Copy link
Contributor

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Yes - lovely. Much cleaner.

I'm not sure about the family opengl because I personally want to make it clearer that that family is not a product type but specifically targetting a node or ROP or alike. So something like rop.opengl might make more sense for being clear what it's targeting.

Anyway, the exact things we want do with the names in families are still up for discussion for which I believe a meeting will happen somewhere in next two days. So let's wait for that to decide.

Aside of that 🪄 Great stuff!

@MustafaJafar MustafaJafar changed the title Use opengl family instead of review Use rop.opengl family instead of review Oct 29, 2024
@MustafaJafar
Copy link
Contributor Author

So something like rop.opengl might make more sense for being clear what it's targeting.

I think both are clear since opengl is well known that it's a node type. But why not making it clearer, so let's use rop.opengl.

@BigRoy
Copy link
Contributor

BigRoy commented Oct 29, 2024

I think both are clear since opengl is well known that it's a node type. But why not making it clearer, so let's use rop.opengl.

Well, sort of. opengl could also just mean 'we're targeting anything opengl globally'. Or what if we need to target an Alembic ROP? Or a USD ROP? Suddenly alembic and usd becomes rather confusing, so rop.alembic and rop.usd would make it clear that we're expecting it to be a ROP node with access to LOPs stage or alike whereas we may also have just e.g. global file.usd targeting to target ANYTHING that happens to generate a usd file. Anyway - that's mostly initial ideas in my head, definitely still has flaws and needs some discussion and brainstorm for further decisions.

@BigRoy
Copy link
Contributor

BigRoy commented Nov 8, 2024

This PR requires ynput/ayon-core#994 first

@MustafaJafar
Copy link
Contributor Author

This PR requires ynput/ayon-core#994 first

Thank you, I've updated the PR description.

@BigRoy
Copy link
Contributor

BigRoy commented Nov 12, 2024

@MustafaJafar can you add "bump minor" label and also add the 'upcoming' ayon-core release as required dependency here: https://github.com/ynput/ayon-houdini/blob/develop/package.py#L9

That'd be >=1.0.8 then when it's released.

Also, feel free to mark it ready for review after that too.

@MustafaJafar MustafaJafar marked this pull request as ready for review November 12, 2024 16:55
Copy link
Contributor

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

I think this PR works - however, I cannot get passed this validation:

image

Repair sets it to <USE_DISPLAY_NAME> but then still it does not pass.

Ayon-core 1.0.8 is released so if it works as intended, feel free to merge.
If the repair is broken for the review plug-in I suppose we can fix that in separate PR.

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.

Looks good for me.

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Nov 13, 2024

I think this PR works - however, I cannot get passed this validation:

image

Repair sets it to <USE_DISPLAY_NAME> but then still it does not pass.

Ayon-core 1.0.8 is released so if it works as intended, feel free to merge. If the repair is broken for the review plug-in I suppose we can fix that in separate PR.

It works on my side.
Animation_35


I don't recall we have that string value in Houdini <USE_DISPLAY_NAME>.
I was able to find it in core here
ehmm. I don't know how to replicate it. could it be a separate issue?

@MustafaJafar MustafaJafar merged commit 0f94942 into develop Nov 13, 2024
1 check passed
@MustafaJafar MustafaJafar deleted the enhancement/use_opengl_family_instead_of_review branch November 13, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants