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

createModel (FBX) #234

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

createModel (FBX) #234

wants to merge 12 commits into from

Conversation

robertokker
Copy link

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.

@BigRoy
Copy link
Contributor

BigRoy commented Mar 3, 2025

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.

@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 BigRoy self-assigned this Mar 3, 2025
@robertokker
Copy link
Author

@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.
the only thing is it loads in the creator####.py so maybe rename than the staticMesh to FBX creator?

@MustafaJafar MustafaJafar self-requested a review March 3, 2025 14:17
@MustafaJafar MustafaJafar added the community Issues and PRs coming from the community members label Mar 3, 2025
@MustafaJafar
Copy link
Contributor

@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. the only thing is it loads in the creator####.py so maybe rename than the staticMesh to FBX creator?

Also, here's a reference :D
https://github.com/ynput/ayon-houdini/blob/develop/client/ayon_houdini/plugins/create/create_usd.py

@BigRoy
Copy link
Contributor

BigRoy commented Mar 3, 2025

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.
the only thing is it loads in the creator####.py so maybe rename than the staticMesh to FBX creator?

Yes, exactly - refactor the filename to create_fbx.py but don't rename the old Creator class name of course, but just do something with inheritance.

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.

@robertokker
Copy link
Author

OK i'll see the point indeed, makes a lot more sense and is a lot cleaner.

robertokker and others added 2 commits March 3, 2025 16:18
…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"
Copy link
Contributor

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?

Suggested change
icon = "cube"
icon = "fa5s.cubes"

Copy link
Author

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.

@BigRoy
Copy link
Contributor

BigRoy commented Mar 3, 2025

@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 get_publish_families that would actually be a bug currently on the static mesh creator.

@robertokker
Copy link
Author

@BigRoy thanks for the commit and the help.

Tommorow i'll look at the model and pointcloud as they are also a full duplicate ;)

@BigRoy
Copy link
Contributor

BigRoy commented Mar 4, 2025

Also, noticed it just now - can you make sure the create_FBX.py filename remains lowercase, create_fbx.py.

Comment on lines +12 to +20

class CreatorModelFBX(BaseSettingsModel):
enabled: bool = SettingsField(title="Enabled")
default_variants: list[str] = SettingsField(
title="Default Products",
default_factory=list,
)


Copy link
Contributor

@BigRoy BigRoy Mar 5, 2025

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. ;)

Suggested change
class CreatorModelFBX(BaseSettingsModel):
enabled: bool = SettingsField(title="Enabled")
default_variants: list[str] = SettingsField(
title="Default Products",
default_factory=list,
)

Comment on lines +98 to +99
CreateModelFBX: CreatorModelFBX = SettingsField(
default_factory=CreatorModelFBX,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
CreateModelFBX: CreatorModelFBX = SettingsField(
default_factory=CreatorModelFBX,
CreateModelFBX: CreatorModel= SettingsField(
default_factory=CreatorModel,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Issues and PRs coming from the community members
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants