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

MAYA-106486 Import UV set mappings #902

Merged
merged 5 commits into from
Nov 12, 2020

Conversation

JGamache-autodesk
Copy link
Collaborator

Applies uvChooser nodes whenever the selected UV is not the default UV.
Will merge specialized materials that were created on export solely to handle UV set names mismatches.

Applies uvChooser nodes whenever the selected UV is not the default UV.
Will merge specialized materials that were created on export solely to handle UV set names mismatches.
Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

This overall looks good to me, but I'm not sure about the layer spelunking we're doing in _IsMergeableMaterial(). Am I missing something there? Can you help me understand why we're only considering the root layer? I would have thought we'd want to consider the fully composed stage instead.

lib/mayaUsd/fileio/translators/translatorMaterial.cpp Outdated Show resolved Hide resolved
lib/usd/translators/meshReader.cpp Show resolved Hide resolved
Comment on lines 74 to 77
const PcpPrimIndex& primIndex = shadeMaterial.GetPrim().GetPrimIndex();
PcpNodeRef primRoot = primIndex.GetRootNode();
SdfPath primPath = primIndex.GetPath();
const SdfLayerHandle& layer = primRoot.GetLayerStack()->GetLayerTree()->GetLayer();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we're trying to find the root layer here. Don't we want to be considering the fully composed stage? Materials could be added or specialized, or opinions could be expressed, across multiple layers.

}
};

layer->Traverse(primPath, testFunction);
Copy link
Contributor

@mattyjams mattyjams Nov 6, 2020

Choose a reason for hiding this comment

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

Along the lines of the above comment, it seems like we'd want a stage-based traversal here rather than a layer-based one.

lib/mayaUsd/fileio/translators/translatorMaterial.cpp Outdated Show resolved Hide resolved
lib/usd/translators/shading/usdUVTextureReader.cpp Outdated Show resolved Hide resolved
# Conflicts:
#	lib/usd/translators/shading/usdFileTextureWriter.cpp
#	lib/usd/translators/shading/usdUVTextureReader.cpp
Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

I think the SdfLayer::Traverse() currently at the bottom of _IsMergeableMaterial() could be replaced with something less expensive. We obviously can't assume that the USD we're importing came from Maya, so it seems very possible (and probably likely) that we might be importing a specialized material with potentially lots of scene description authored on or underneath it.

Comment on lines 88 to 109
UsdPrimCompositionQueryArc specializationArc = arcs[1];
SdfPath primPath = shadeMaterial.GetPath();

// Check that the specialization arc contains only opinions on varname inputs:
bool retVal = true;
auto isVarnameOpinion = [&](const SdfPath& path) {
if (path == primPath) {
return;
}
if (path.GetParentPath() != primPath || !path.IsPrimPropertyPath()) {
retVal = false;
return;
}
std::vector<std::string> splitName = SdfPath::TokenizeIdentifier(path.GetName());
// We allow only ["inputs", "<texture_name>", "varname"]
if (splitName.size() != 3 || splitName[0] != _tokens->inputs.GetString()
|| splitName[2] != _tokens->varname.GetString()) {
retVal = false;
}
};

specializationArc.GetIntroducingLayer()->Traverse(primPath, isVarnameOpinion);
Copy link
Contributor

Choose a reason for hiding this comment

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

The layer-based traversal at the bottom here has the potential to be way more expensive than it needs to be. At the point where we start the traversal, all we know is that we're looking at a specialized material. The root of the traversal is the primSpec in the layer that introduced the specialize arc. That layer may also have an arbitrary number of properties and/or namespace children authored for that Material, and we're going to end up visiting all of those paths when really we know whether or not we should stop after inspecting only the Material primSpec and its properties.

With the caveat that this is untested, this is kind of what I'm thinking for this, beginning right after we verify that there are only two arcs:

    // This is a little more robust than grabbing a specific arc index.
    UsdPrimCompositionQuery::Filter filter;
    filter.arcTypeFilter = UsdPrimCompositionQuery::ArcTypeFilter::Specialize;
    query.SetFilter(filter);
    arcs = query.GetCompositionArcs();
    const UsdPrimCompositionQueryArc specializationArc = arcs.front();

    const SdfLayerHandle layer = specializationArc.GetIntroducingLayer();
    const SdfPrimSpecHandle primSpec = layer->GetPrimAtPath(shadeMaterial.GetPath());

    // If the primSpec that specializes the base material introduces other
    // namespace children, it can't be merged.
    if (!primSpec->GetNameChildren().empty()) {
        return false;
    }

    // Check that the only properties authored are varname inputs.
    for (const SdfPropertySpecHandle& propSpec : primSpec->GetProperties()) {
        const SdfPath propPath = propSpec->GetPath();

        const std::vector<std::string> splitName =
            SdfPath::TokenizeIdentifier(propPath.GetName());
        // We allow only ["inputs", "<texture_name>", "varname"]
        if (splitName.size() != 3u ||
                splitName[0u] != _tokens->inputs.GetString() ||
                splitName[2u] != _tokens->varname.GetString()) {
            return false
        }
    }

    return true;

Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @JGamache-autodesk!

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 12, 2020
@kxl-adsk kxl-adsk merged commit a83cfc1 into dev Nov 12, 2020
@kxl-adsk kxl-adsk deleted the t_gamaj/MAYA-106486/import_uvset_mappings branch November 12, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
import-export Related to Import and/or Export ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants