-
Notifications
You must be signed in to change notification settings - Fork 14
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
createModel (FBX) #234
base: develop
Are you sure you want to change the base?
createModel (FBX) #234
Conversation
@robertokker If this code aligns 90% with the other one. Could we put the creators in the same python file and inherit one from the other, so that 99% of the code is shared? I really hate duplicating stuff :D @antirotor thoughts? |
@BigRoy yeah i will try to put it into 1 file, and hust have the option to adjust the "productType" as i think that should work. |
Also, here's a reference :D |
Yes, exactly - refactor the filename to It does show the potential issue with Creators being so tightly coupled to the product type instead of a creator possibly being of multiple product types. Which has also come up a lot with "generic publishing" where we have some automated creator that can generate any type of product type. The system (as @iLLiCiTiT kindly pointed out) was never designed for a Creator to target more than one product type - they were very tightly coupled by design. But it popping up here and there may hint that at some point there could be reasons to avoid that tight coupling. Tagging @philippe-ynput @antirotor just in case. |
OK i'll see the point indeed, makes a lot more sense and is a lot cleaner. |
…h the guidance of Roy. With guidline the create_usd. We now have only 1 create_FBX to controll model and static mesh vatriants
combined create_modelFBX and create_staticMesh to 1 piece of code wit…
identifier = "io.openpype.creators.houdini.staticmesh.fbx" | ||
label = "staticMesh (FBX)A" | ||
product_type = "staticMesh" | ||
icon = "cube" |
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.
What's the reason for the icon change?
icon = "cube" | |
icon = "fa5s.cubes" |
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.
@BigRoy the model abc also had cube as icon, so i think i copied it from there.
@robertokker I've pushed some comments (and directly committed them too since they were mostly cosmetic). You can use the comments to see my reasoning, and hope there isn't any issue with me pushing them here. Just left the icon change on static mesh creator open as a comment - any reason to not stick to the icon it had originally? And had one comment about |
@BigRoy thanks for the commit and the help. Tommorow i'll look at the model and pointcloud as they are also a full duplicate ;) |
Co-authored-by: Roy Nieterau <[email protected]>
Co-authored-by: Roy Nieterau <[email protected]>
Also, noticed it just now - can you make sure the |
|
||
class CreatorModelFBX(BaseSettingsModel): | ||
enabled: bool = SettingsField(title="Enabled") | ||
default_variants: list[str] = SettingsField( | ||
title="Default Products", | ||
default_factory=list, | ||
) | ||
|
||
|
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.
In this case this extra settings model is not needed. It's exactly like the one above -and there isn't a reason for a new unique on here. Note also how the other settings entries below, multiples of them use the CreatorModel
. It's like the base model for Creators, it's not specific to the model creator. ;)
class CreatorModelFBX(BaseSettingsModel): | |
enabled: bool = SettingsField(title="Enabled") | |
default_variants: list[str] = SettingsField( | |
title="Default Products", | |
default_factory=list, | |
) |
CreateModelFBX: CreatorModelFBX = SettingsField( | ||
default_factory=CreatorModelFBX, |
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.
CreateModelFBX: CreatorModelFBX = SettingsField( | |
default_factory=CreatorModelFBX, | |
CreateModelFBX: CreatorModel= SettingsField( | |
default_factory=CreatorModel, |
Changelog Description
copied the static mesh creator to make the productType "model" instead of staticMesh.
This so it aligns better with fbx exports from blender etc in the whole pipeline.
Also added 2 new options to the creator so you don;t have to go into the rop to change them al the time.
1-ASCII format (as blender can only read ASCII fbx files this is now a option in the creator .
2- single frame (this sets the fbx node to single frame or to frame range.
Additional review information
Detailed information of the changes made to the product or service, providing an in-depth description of the updates and enhancements. This can include technical information, code examples and anything else needed for the review of the PR.