Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Multiverse: fixed composition write, full docs, cosmetics #3178

Merged
merged 26 commits into from
Jun 8, 2022

Conversation

pberto
Copy link
Contributor

@pberto pberto commented May 13, 2022

Brief description

Finalized Multiverse integration.

Description

  • updating labels for the various tools as per discussed
  • first version of the documentation on website
  • added jcube to op contributors on main page of website
  • Adding support for choosing .usd/.usda/.usdz.
  • fixed empty composition due to relative paths
  • formatting / cosmetics

Documentation

This PR includes also documentation.

Notes

We will add a Multiverse Look creator at the next PR.

@pberto pberto changed the title Feature/multiverse Multiverse: fixed composition write, full docs, cosmetics May 13, 2022
@@ -129,13 +131,19 @@ def parse_overrides(self, instance, options):

return options

def get_file_format(self, instance):
Copy link
Member

@iLLiCiTiT iLLiCiTiT May 13, 2022

Choose a reason for hiding this comment

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

You call get_file_format but it does not return a thing. Instead, it is changing attribute of object so if instance does have empty fileFormats it will use scene_type of previous instance.

Also not sure what the code should do? If it would be rewritten to simplified version:

instance_file_format = ["usd", "usda"]
file_formats = ["usd", "usda", "usdz"]
some_range = range(len(file_formats)) # that's the same as [0, 1, 2]

# This will never be true as the list with file formats won't be in iter of itegers
if instance_file_format in some_range:
    # You change attribut on the class instead of returning it
    # + it would crash if it would get here because it's using
    #    a list to get item from list (TypeError)
    self.scene_type = file_formats[instance_file_format]

It kinda looks like you wanted to use for instead of if? But that's anti-pythonic way of looping through list. Also only last item would be used in that case it would be better to use:

# Backwards compatibility for instances without fileFormat
file_format = instance.data.get("fileFormat")
# Check if is set
if file_format:
    # return last item of the list
    return file_format[-1]
# Return default scene type
return self.default_file_format

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would be instance.data["fileFormat"] be a list or tuple in the first place - that sounds confusing?

Why can't it be this?

file_format = instance.data.get("fileFormat",
                                self.default_file_format)

Or even a hardcoded backwards compatible default:

file_format = instance.data.get("fileFormat", 
                                # Backwards compatibility
                                "usd")

With this simplification it doesn't need its own get_file_format method. Nonetheless, like mentioned before.. please return the value instead of storing it on the class if you do end up using a defined method for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the delay. For simplicity, I did your suggestion of using that get method.

@@ -199,10 +201,10 @@ def process(self, instance):
instance.data["representations"] = []

representation = {
'name': 'usd',
'ext': 'usd',
'name': self.scene_type,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@iLLiCiTiT would you feel it's wrong if name would always be usd no matter the extension? That way updating in the manager can be done even if file format changed in-between. Thoughts?

@BigRoy
Copy link
Collaborator

BigRoy commented May 16, 2022

We will add a Multiverse Look creator at the next PR.

It might be confusing that the current documentation does already mention Multiverse Looks (e.g. in screenshots). Do you have an ETA on that next PR?

This extracts the look information from the UsdCompoundShape - it generates
"OP look" compatible data structures and lets the rest of the OP publish
as normal. It also generates a .usda file with the overrides containing
material assignments as needed.
… TDL's

generated by 3delight. During collection, we check to see if the TDL's exist
beside the original texture, and if they do they get added to the collection.

Validation of these will depend on the publish intent and whether
`expectMipMap` is True in the publish set.

If mipmaps are expected and the intent is correct, the mipmaps will be
published along with the original file, which will accelerate rendering,
since 3dl will find that TDL and use it instead of having to generate it.
…stance.

Renamed `expectMipMap` to `publishMipMap` to make it more clear.
Added `linear` & `auto` to possible color spaces.
- better docstrings for pyblish UI
- updated images
- re-organized Maya content in website by splitting plugin docs and
  sidebar menu (existing links are not broken since the same entry point
  doc exists)
@pberto
Copy link
Contributor Author

pberto commented May 31, 2022

@mkolar please note I refactored a bit the Maya docs as the page was too long. I personally think this new organization is easier to read for the user and is also faster to find content.

@mkolar
Copy link
Member

mkolar commented May 31, 2022

@mkolar please note I refactored a bit the Maya docs as the page was too long. I personally think this new organization is easier to read for the user and is also faster to find content.

Thank you having a look

@m-u-r-p-h-y
Copy link
Member

The link in the documentation is not valid

image

@pberto
Copy link
Contributor Author

pberto commented Jun 2, 2022

That is because we did not release 7.1 yet but it will be released in the next days so that link be valid and you should not worry about it, you can safely merge it :)

@mkolar
Copy link
Member

mkolar commented Jun 6, 2022

@pberto just for completeness I'll add information we discussed in private conversation.

We're happy to merge this, but please first change the family names to be prefixed with mv. We're working towards better modularity and also on native USD integration, that will be most probably structured a little differently than your data, so we'd like to avoid any confustions and make sure we don't have to resort in family renaming once this is in production. If we come to the point that OP would be generating USD based families with your suggested names across the board, it will be trivial to add their compatibility to your loaders and publishers, just by adding the new family to it. But let's cross that bridge when we get to it.

Other than that it's looking like great work! Thank you

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 7, 2022

but please first change the family names to be prefixed with mv

Are we sure? The data is just regular usd data that is generated in the end. Nothing Multiverse about that - except for the published multiverse looks that is.

To load the multiverse USDs into Houdini I'd then have to allow them to load mvUsd data instead of regular USD data. This will spread much further than we'd probably like?

I admit the Loaders themselves (so that loaded content can be managed correctly in the future) are MultiverseUsdLoader, etc. but they already are. 👍

I can see however how the Extractor might run into a conflict with the family if another pops up - but I think that's much easier to solve in the future than fixing published families.

Or did you intend to say that we'd still try to actually register the versions as usd family?

@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

Or did you intend to say that we'd still try to actually register the versions as usd family?

We're actually very wary about USD family in general. It goes very much against the grain of what families should be in the first place. model layout rig animation. Families define the type of data, not it's format. The exceptions are generic families that a host can define as a fallback. For example mayaScene or nukeNodes. Those are also not validatable.

It is very much true that if we prefix them, then other USD loaders should be aware of them, but I'm not sure it's a good ideal to now define usdComposition, USD and usdOverride as global families either. Adding a family to a loader can always be extracted into it's settings, as we do in many places.

There is a large discussion required (which btw we'll be invoking within days most probably) on how to work with USD across the board with OP. I have a feeling that the first steps will be as representation of some families (Layout represented as USD, model represented as USD and so on).

@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

MV structures the USD data in certain way that makes sense from it's perspective and even though they are just native USD files, it's kinda like ditching model family and instead we start using alembic instead. Yes it's universal, but it removes all structure.

@mkolar
Copy link
Member

mkolar commented Jun 7, 2022

Houdini implementation at the moment is a good example of such mixup. We have usdCamera which makes no sense. It should be Camera family that has USD representation, albeit generic USD scene family will be immensely useful as fallback, the same way as mayaScene is. The question is, what to call is USD, usdScene, usdStage? And it doesn't really fit what usdCompositionandusdOverrides` are as they have very specific structure that could be validated

@BigRoy
Copy link
Collaborator

BigRoy commented Jun 7, 2022

Thanks for the follow up - I understand. 👍 Definitely worth a good brainstorm on other potential approaches for this across the whole "family". (Pun intended.)

@mkolar mkolar self-requested a review June 7, 2022 11:46
Copy link
Member

@mkolar mkolar left a comment

Choose a reason for hiding this comment

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

please check comment in #3178 (comment)

@mkolar mkolar linked an issue Jun 7, 2022 that may be closed by this pull request
@pberto
Copy link
Contributor Author

pberto commented Jun 8, 2022

please first change the family names to be prefixed with mv.

@mkolar as I said in private, no problem, we can change family names. Whats important is that the representation stays "usd" -- which we both agreed on.

`usdComposition`->`mvUsdComposition`, `usdOverride`->`usdOverride`,
specifically within multiverse files. I have left the `usd` family in
`integrate_new.py` because that is being used in a bunch of different
places (eg: houdini's host integration), and have just added `mvUsd` as
a new family.
@dmo-j-cube
Copy link
Contributor

I have changed names for multiverse related families from usdX to mvUsdX - but have left usd intact since it's also been used in other places (and was already there before we pushed our first commit)

Copy link
Member

@mkolar mkolar left a comment

Choose a reason for hiding this comment

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

Thank you guys. This is great and I think we can squeeze it into 3.11.0

@mkolar mkolar added the type: feature Larger, user affecting changes and completely new things label Jun 8, 2022
@mkolar mkolar merged commit c7d2354 into ynput:develop Jun 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature Larger, user affecting changes and completely new things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiverse: First Support
7 participants