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

Clean-up and fix Alembic creator attributes #208

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

Conversation

BigRoy
Copy link
Contributor

@BigRoy BigRoy commented Dec 27, 2024

Changelog Description

  • Remove write_color_sets and write_face_sets from creators where relevant.
    • Note: For model creator these are maintained, see the todo in code because there is special logic surrounding the model product type's creator toggle for "Write Face Sets" since Support to create face sets for materials in Extract Alembic #37 by @moonyuet however it overlaps heavily with the "write face sets" toggle on the actual Extractor which is a completely separate toggle and currently for this model instance's face sets logic to work it is required for BOTH toggles to be enabled.
  • Fix Create Animation and Create Pointcache defaults from settings for Include Parent Hierarchy and Include User Defined Attributes creator attributes. Previously the settings would not define the defaults at all.
  • Add tooltip (and TODO) for attr and attrPrefix creator attributes for model product type - because those are essentially defined by the Alembic extractor already and hence currently only influence the USD export of the model product type (which in essence is disabled by default)

Additional review information

n/a

Testing notes:

  1. Writing color sets and writing face sets with model, pointcache and animation instances should still behave as intended.
  2. Defaults in settings for Create animation and Create pointcache should be applied, in particular for "Include Parent Hierarchy" and "Include User Defined Attributes".
  3. The Model product "attr" and "attrPrefix" should be somewhat clearer as to what it applies to. it should only apply to USD export, because Alembic export already exposes its own attribute definition for it.
  4. The model product 'write face sets' should still behave as intended as per Support to create face sets for materials in Extract Alembic #37

…lt values from settings for Animation and Pointcache Creators
…levant to maya usd export (however, add TODO and tooltip elaborating that it only influences that)
@BigRoy BigRoy added the type: bug Something isn't working label Dec 27, 2024
@BigRoy BigRoy self-assigned this Dec 27, 2024
@moonyuet
Copy link
Member

I did some simple test and checked the code. Looks good and works as expected.
image
But I do have a question whether there is some use case for writing face sets when we export the assets in animation or point cache(Maybe this is for @LiborBatek). It seems that it is used only for exporting the model product from maya to unreal (or SP) for the current stage.

@BigRoy
Copy link
Contributor Author

BigRoy commented Dec 30, 2024

But I do have a question whether there is some use case for writing face sets when we export the assets in animation or point cache(Maybe this is for @LiborBatek). It seems that it is used only for exporting the model product from maya to unreal (or SP) for the current stage.

I agree - there may be merit in it; however, I think it should be a separate PR and I'm thinking it should move from Creator option to the Extractor option and become its own dedicated sub-toggle of writeFaceSets so that you can write face sets regularly without the enforced face shader assignments, and then can enable the "force shader assignment to per face assignment for writing face sets" separately.

I'm just not sure how well that is currently exposed to the model product type to make that really easy to add as optional toggle that way - but that way the logic could be shared between all product types, etc.

Either way, it can be a separate PR - it's new functionality that doesn't exist currently regardless for the animation and pointcache product type.

@moonyuet
Copy link
Member

moonyuet commented Jan 2, 2025

But I do have a question whether there is some use case for writing face sets when we export the assets in animation or point cache(Maybe this is for @LiborBatek). It seems that it is used only for exporting the model product from maya to unreal (or SP) for the current stage.

I agree - there may be merit in it; however, I think it should be a separate PR and I'm thinking it should move from Creator option to the Extractor option and become its own dedicated sub-toggle of writeFaceSets so that you can write face sets regularly without the enforced face shader assignments, and then can enable the "force shader assignment to per face assignment for writing face sets" separately.

I'm just not sure how well that is currently exposed to the model product type to make that really easy to add as optional toggle that way - but that way the logic could be shared between all product types, etc.

Either way, it can be a separate PR - it's new functionality that doesn't exist currently regardless for the animation and pointcache product type.

Indeed. We can create a new issue for that if there is any need for that functionality.

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

LGTM.

I have experienced once some cosmetic glitch on legacy model instance in the Publisher UI as seen below...however once recreated, all good. The default values for various properties been tested and propagation happening without issues too.
Screenshot 2025-01-03 134831

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants