OpenPype
OpenPype copied to clipboard
ynput
Houdini: Implement review family with opengl node
Brief description
Implements a bare minimum viable product for #2720
Description
Uses the opengl ROP node to produce PNG images.
Additional info
Required:
- [x] Working with Extract Review
- [x] Working with Extract burnins
- [ ] Expose some crucial default settings?
- [ ] Allow resolution of 0, 0 to represent "take resolution from asset data"?
Nice-to-haves:
- [ ] Add Attach To functionality (also see discussion)
- [ ] Maya has a "delete images" option.
- [ ] Potentially might need default settings in OpenPype settings for specific Display Options on the ROP node, etc. (Probably not needed for minimum viable product of the feature)
- [ ] Might need a validator for the file extension - it's assuming it remains set to
.pngcurrently. - [ ] Selected nodes should be filtered to 'cameras' and '/obj' level paths to go into
cameraandscenepathparm. - [ ] The Houdini logic for "Collect Frames" currently requires a period (
.) before the frame numbers - something to keep an eye out for. (Probably best as separate issue/PR) - [ ] Might need refactoring after #3046
Testing notes:
- Launch Houdini
- Create review instance
- Set the "object path" and "camera" to something correct (needs to be done manually for now)
Might need refactoring after https://github.com/pypeclub/OpenPype/pull/3046
As I am looking at the code, only minimal.
What are your thoughts about dealing with the resolution? In Maya, it is rather complex thing - getting resolution from asset but allowing you to override it through OP Settings and even Maya viewport settings.
What are your thoughts about dealing with the resolution? In Maya, it is rather complex thing - getting resolution from asset but allowing you to override it through OP Settings and even Maya viewport settings.
@antirotor With this commit I've made it so that the resolution by default is set on the OpenGL rop and does not just inherit it from the camera it's rendering so it's more explicit.
Other than that I was thinking of potentially having it similar to maya. The integer fields that allow to override resolution on the OpenGL node actually allow negative values (go figure!). So I was thinking to do similar to maya that if -1 was found as value that it'd fall back to use asset resolution. So basically match the Maya behavior.
Would you prefer different?
About a feature like "attachTo" in maya. What would integrators and publish plug-ins need with regards to instance.data (or context.data?) for me to be able to do the same in Houdini?
About a feature like "attachTo" in maya. What would integrators and publish plug-ins need with regards to
instance.data(orcontext.data?) for me to be able to do the same in Houdini?
With the old publisher it takes just a subset name since there is no way to attach reviews to already published subsets. With the new one, we could maybe do something similar, but there are now ids there that are more robust and in the future we could extend it - find if the connected node to review node is publish instance or loaded instance and either add review to existing version or add to the new one.
Another question is how to deal with 'isolate' feature to create review just for the connected data or for the whole scene. But that's maybe something for the future PRs.
With the old publisher it takes just a subset name since there is no way to attach reviews to already published subsets. With the new one, we could maybe do something similar, but there are now ids there that are more robust and in the future we could extend it - find if the connected node to review node is publish instance or loaded instance and either add review to existing version or add to the new one.
For now:
instance.data["attachTo"] = "other_subset_name"
Check, thanks!
Not sure how attach to technically works down the line - but somehow I'd have expected to also be able to attach a review to multiple subsets. But probably since I haven't used the feature yet I might just be overthinking things.
Another question is how to deal with 'isolate' feature to create review just for the connected data or for the whole scene. But that's maybe something for the future PRs.
OpenGL can specify what "objects" to render - so isolating certain parts of your scene is totally doable. You can also explicitly exclude certain objects, etc. Would leave that up to the artist to set up for now if they'd like to tweak it. That would be ok?
basically same question as with #2604 - we need to convert it to new publisher and Houdini will benefit from this even in this state greatly. So, should we merge it into our branch and continue "transform" to the new publisher there?
I've made this work as barebones as it was but now with the new publisher.
I tried implementing this on the creator:
def get_instance_attr_defs(self):
return [
BoolDef("override_resolution",
label="Override resolution",
tooltip="When disabled the resolution set on the camera "
"is used instead.",
default=True),
NumberDef("resx",
label="Resolution Width",
default=1280,
minimum=2,
decimals=0),
NumberDef("resy",
label="Resolution Height",
default=720,
minimum=2,
decimals=0),
NumberDef("aspect",
label="Aspect Ratio",
default=1.0,
minimum=0.0001,
decimals=3)
]
But as I've mentioned on discord a few times it just doesn't seem like the Houdini integration works in that regard. I started getting errors on refresh/reset and gave up again.
Additionally I would've liked it the HoudiniCreator plug-in allowed me to "intercept" certain attribute definitions on how they'd be read and imprinted into the scene easily. We might want to add a dedicated method to allow interception certain variables too. So that e.g. these attributes shown here could be actually stored and set using the dedicated attributes that the Houdini node itself already has for it. That would make much more sense.
@antirotor Any idea how we could go about making this production-ready and workable?
@fabiaserra You mentioned you were interested in generating "reviews" from Houdini. This was a prototype that I believe worked. I've pulled in latest develop branch so I'd say give it a test run for some feedback.
@antirotor I'm not sure how valid these notes addressed to you still are but if there's also someone on OP team able to give this a test run that'd be welcome!
@fabiaserra You mentioned you were interested in generating "reviews" from Houdini. This was a prototype that I believe worked. I've pulled in latest develop branch so I'd say give it a test run for some feedback.
@antirotor I'm not sure how valid these notes addressed to you still are but if there's also someone on OP team able to give this a test run that'd be welcome!
Amazing, thank you @BigRoy !
Additionally I would've liked it the HoudiniCreator plug-in allowed me to "intercept" certain attribute definitions on how they'd be read and imprinted into the scene easily. We might want to add a dedicated method to allow interception certain variables too. So that e.g. these attributes shown here could be actually stored and set using the dedicated attributes that the Houdini node itself already has for it. That would make much more sense.
We might solve it by slightly changing what imprint accepts and use ch() to link "native" parameters to ours. That would be probably the easiest way to implement it.
We might solve it by slightly changing what imprint accepts and use ch() to link "native" parameters to ours. That would be probably the easiest way to implement it.
It would be for both imprint and loading the data.
So I've updated it again and done some test runs with fixes. I've also added focal length burnin support like @tokejepsen did with #4531 for Maya.
This PR should put it in a first rough usable state. However do still take a look at the Nice-to-haves list on the initial post and let me know if there's anything there that should be crucial to getting this PR merged.
@antirotor @fabiaserra it would be gold if any of you could test this out.
The last three commits accidentally implemented the fix of PR #4739 as well - it shouldn't be a problem, but just so you know. My software apparently didn't switch branches quickly enough while I was already committing the fixes. 🐌
@tokejepsen What do you think? Should I update the maya_burnin default preset to also operate on Houdini host and call it the focallength_burnin instead since this PR now also supports the animated focal length?
@tokejepsen What do you think? Should I update the maya_burnin default preset to also operate on Houdini host and call it the focallength_burnin instead since this PR now also supports the animated focal length?
Yeah, sounds good. Also update the docs :)
Works as expected after cleaning up of the enum in the creator's settings. The updated PR can export image sequences with different image formats. Also it has a similar option of deleting images as if maya.
Basically the user can choose whether they want to keep image sequences or not. By default, the sequences are removed after publishing and only the video is in the related publish folder.
Not sure if it is a good idea(it's just personal opinion) - I think the selected node option should mainly allow user selecting the camera for the playblast, as it seems the camera is mandatory for opengl, while the scene path is always left as /obj/ by default , When you hit render, it will yell at you if there is no existing camera while it won't yell if the scene path is left as /obj/ , there should be always validation for the scene path to make sure the user won't keep it empty or set a wrong path.
Not sure if it is a good idea(it's just personal opinion) - I think the selected node option should mainly allow user selecting the camera for the playblast, as it seems the camera is mandatory for opengl, while the scene path is always left as /obj/ by default , When you hit render, it will yell at you if there is no existing camera while it won't yell if the scene path is left as /obj/ , there should be always validation for the scene path to make sure the user won't keep it empty or set a wrong path.
Yes, agree that workflow could be better.
Our houdini artist actually mentioned he'd prefer the default scenepath to always be /obj and then Candidate Objects which defaults to * in houdini to be cleared (so it doesn't show anything by default) but then in Force Objects it'd add your selection (whatever in your selection is not a camera) so that what you had selected will always be rendered even if at some point in time the user turns off the display flag.
That'd make the rop node way more explicit and not dependent on display flags of your scene at any point in time. Thoughts?
Not sure if it is a good idea(it's just personal opinion) - I think the selected node option should mainly allow user selecting the camera for the playblast, as it seems the camera is mandatory for opengl, while the scene path is always left as /obj/ by default , When you hit render, it will yell at you if there is no existing camera while it won't yell if the scene path is left as /obj/ , there should be always validation for the scene path to make sure the user won't keep it empty or set a wrong path.
Yes, agree that workflow could be better.
Our houdini artist actually mentioned he'd prefer the default scenepath to always be
/objand then Candidate Objects which defaults to*in houdini to be cleared (so it doesn't show anything by default) but then in Force Objects it'd add your selection (whatever in your selection is not a camera) so that what you had selected will always be rendered even if at some point in time the user turns off the display flag.That'd make the rop node way more explicit and not dependent on display flags of your scene at any point in time. Thoughts?
Yes, I agree that we should make sure the rop node is independent on display flags.
Test it just now. Nice, the camera params of the opengl nodes would be set to the path of the selected camera, while the force objects params would be set to the paths of the selected nodes. I just checked again on the nice-to-have in this PR, and find out you want to use 0, 0 to represent resolution taken from asset data...And somehow the validator does not allow this happened. I double check the opengl node in Houdini and find out it actually works if the user playblast in 0, 0 resolution. Maybe it is good for testing if the playblast works before the full review playblast as I know some scene can still be huge even there is a cache. I am not quite sure about the image format validator though.
And somehow the validator does not allow this happened. I double check the opengl node in Houdini and find out it actually works if the user playblast in 0, 0 resolution. Maybe it is good for testing if the playblast works before the full review playblast as I know some scene can still be huge even there is a cache.
I can't really see a production case where you'd want to actually end up playblasting a 0x0 resolution. I think with the validator for now is fine. Just need to add documentation I think.
@antirotor this should be ready. Could you take a look? @tokejepsen any chance you could take a peek too and maybe check the documentation?
Yeah, good point. Should be moved to here; https://openpype.io/docs/project_settings/settings_project_global#publish-plugins
@tokejepsen I'll be moving that in a separate PR. There is no Extract Burnin documentation for the OP v3 it seems but only pype v2. Adding that here would make this PR more bloated than needed and I expect that the documentation might get some more feedback going and I'd rather get this PR in separately from that. Does that mean you could approve this PR?
I've created this separate PR for the Extract Burnin settings documentation: https://github.com/ynput/OpenPype/pull/4765
I created issue [focalLenght](https://github.com/ynput/OpenPype/issues/4789) for Toke's comment.
Looking into code, I don't think it should be applicable for Houdini, imho
Works fine! The only problem (not related to this PR and probably partially solved by https://github.com/ynput/OpenPype/pull/4700) is that the ROP node is not handling gamma correction very nice - I had to manually set gamma to 2.2 when rendering to pngs otherwise the resulting video would be dark. This would be handled by OCIO better, so I suppose it isn't of much concern
Thanks, that does sounds like something we can track in a separate dedicated issue to resolve. 👍