search

ynput / ayon-houdini

Houdini addon for AYON
Apache License 2.0
11 stars 9 forks source link

Use `rop.opengl` family instead of `review` #148

Closed MustafaJafar closed 1 week ago

MustafaJafar commented 3 weeks ago

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 https://github.com/ynput/ayon-core/pull/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.)
BigRoy commented 3 weeks ago

~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.

MustafaJafar commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

This PR requires https://github.com/ynput/ayon-core/pull/994 first

MustafaJafar commented 2 weeks ago

This PR requires ynput/ayon-core#994 first

Thank you, I've updated the PR description.

BigRoy commented 1 week ago

@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 commented 1 week ago

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?