-
Notifications
You must be signed in to change notification settings - Fork 12
Render: Add support for local rendering #217
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
base: develop
Are you sure you want to change the base?
Render: Add support for local rendering #217
Conversation
…208-yn-0280-blender-add-support-for-local-render
…space is correctly assigned to the relevant AOV instance
LiborBatek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to see the viewport itself? Not sure how trivial that really is in Blender. 🤔
@LiborBatek can you provide a publish log? |
|
@BigRoy yep, sure... here you have it. |
+ Add local render instance "review" family if it has it, instead of overriding completely so that Extract Review still runs to allow generation of reviewables and thumbnails
|
@LiborBatek thanks - that was trickier than I expected. I had a typo (missing comma) that essentially meant some things didn't transfer as I expected they did. Also, "review" family I accidentally discarded. I fixed that, but then noticed that regular But now, it should work. But can you please test whether new and existing Review instances still work as expected? @antirotor @iLLiCiTiT I added you as reviewers because I'd love some code bashing on this. I have a feeling we can do this in a cleaner manner than I'm currently able to. Tips are welcome! Note that some of the logic like "Collect Local Render Instances" should be able to overlap a bit with ynput/ayon-maya#383 and I'd like to work towards a way where we can unify it in such a way that perhaps it could even be a base collector in core. |
|
@BigRoy I was able to render locally and the images successfully got integrated... there was a strange behaviour tho when rendering on farm as it was collecting wrong version for the product (v004 instead of v006), Im really not sure if its connected to this PR or rather issue in the |
Hard to tell - I'll just wait until you're confident to approve 👼 |
LiborBatek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have investigated the aformentioned versioning issue and its not tied to this PR.
So my conclusion been its all working well for both local/farm rendering!
Good to go I guess!
| data["families"] = self.get_publish_families() | ||
| return data | ||
|
|
||
| def get_publish_families(self) -> list[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add new families in collection phase instead? What is the point of storing them to instance metadata when they can be added during publishing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need the families to expose the existing plug-ins, etc.
Otherwise the reverse would be true that we'd need to essentially implement an override to get_attribute_defs for all the plug-ins to target an unknown value that would still need to be defined by the creator to begin with? Why not use families which is what the actual plug-ins are targeting their process for to begin with?
The same logic of get_publish_families exists in Maya and Houdini.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise the reverse would be true that we'd need to essentially implement an override to get_attribute_defs for all the plug-ins to target an unknown value that would still need to be defined by the creator to begin with?
Where you'd need to implement the override?
The same logic of get_publish_families exists in Maya and Houdini.
Doesn't mean it is good. The fact that the data are not stored to the scene is one of the reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't mean it is good. The fact that the data are not stored to the scene is one of the reasons.
It's intentionally not stored to the scene - it's runtime data based on the Creator. It's not a if this, then that. If it's this creator, then it's always those families. I don't see why it should be stored anywhere - it's the creator saying I'm producing this.
Where you'd need to implement the override?
Extract Review in core for example?
We have a Creator here that generates a "review" product type.... which means it's "review" family, but that Creator's extractors then trigger whenever "review" family is present. However, if I have another instance, that just happens to want to also go through Extract Review it needs that family too, but do not trigger the other Extractor.
We essentially have the conflict that a single Creator can't target explicitly a few things, like "trigger reviews please" and "this supports XYZ" and that productType == family by default on the creators. Setting explicit families from the creators makes sure so that we can take that control nicely, and it requires no logic changes elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract Review in core for example?
That plugin doesn't have get_attribute_defs. So, why it has to be defined in create phase? The only real reason that might make sense is because other addons need to do something about them in create phase, even then it is wrong, but acceptable. But in this case I don't see any reason why it has to be defined in create phase. families is publish concept, or rather pyblish concept, has nothing to do with create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let me get it straight. You're saying, remove this.
Instead, keep CreateReview in blender as review product-type. Then on early CollectorOrder add a plug-in that does:
class CollectReview(...):
def process(self, instance):
instance.data["families"].append("playblast")Then make all those extractor plug-ins target e.g. "playblast" family.
If any of those plug-ins then would need to expose attributes they need to implement get_attr_defs to figure out whether some instance is this special "review" / "playblast" family?
Just so that all the other plug-ins for that Creator do not apply to anything else, like e.g. a render with review family?
I feel like with the publishing ecosystem families is what we have to tag whether a certain plug-in targets that instance. So if I want my instance to be targeted by anything that targets "review" I'd want to be able to say it does - just so that if reviewables have any customizable options in the UI, they get presented to the user - also for renders?
I'm confused why you're so keen on avoiding the default behavior of actually targeting something based on families?
Also, we should really start differentiating product type versus families. 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is any of the plugins you're talking about not in blender addon?
The big power comes with get_attr_defs_for_instance not with get_attr_defs. get_attr_defs is lazy solution that caused all of this mess. You can very specifically target attribute definitions for a very specific instance which is 100% better than using families (list of strings) which is pyblish process concept.
Right now create plugin defines data for publish plugin to show attributes, create plugin has to know about the publish plugin and vica versa. So why are not the attributes defined on the create plugin directly? If not then why does the create plugin has to know about the publish plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll need to have a call here, because I don't see this work the other way either:
Right now create plugin defines data for publish plugin to show attributes, create plugin has to know about the publish plugin and vica versa. So why are not the attributes defined on the create plugin directly? If not then why does the create plugin has to know about the publish plugin?
If I have a generic plug-in that would only apply to say anything that triggers a local render, or anything that generates USD files. (e.g. a model product type creator generating usd files). What can the plug-in rely on to query for in get_attr_defs_for_instance? Seems like we'd still need to be storing some metadata somewhere that says, "I'll be usd data!"? And if so, why not use families because those are also the exact thing that defines whether the plug-ins process will trigger in the first place? It seems to me like in 99% of the cases whether I want to show the attributes is 1:1 the same as whether the plug-in will trigger for an instance... which in pyblish, is the families. Even if get_attr_defs_for_instance would use some other metadata, I still need the family just so the plug-in ends up triggering?


Changelog Description
Render: Add support for local rendering
Additional review information
Fix #208
Testing notes: