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-106079 Skip Looks scopes #985

Merged
merged 3 commits into from
Dec 9, 2020
Merged

Conversation

JGamache-autodesk
Copy link
Collaborator

Skip importing non-empty UsdScope that only contains UsdShade items.

Skip importing non-empty UsdScope that only contains UsdShade items.
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.

Just some minor suggestions, but otherwise looks good to me!

Comment on lines 33 to 46
// If this is a Looks/Materials scope that contains only UsdShade nodes, just skip.
bool hasLookData = false;
bool hasNonLookData = false;
for (auto const& child : usdPrim.GetChildren()) {
if (child.IsA<UsdShadeNodeGraph>() || child.IsA<UsdShadeShader>()) {
hasLookData = true;
} else {
hasNonLookData = true;
break;
}
}
if (hasLookData && !hasNonLookData) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, but a few somewhat cosmetic notes:

  • Though Looks is unfortunately still the default for the material scope name, I think we'd like to transition away from using that term, and really we're just interested in finding any UsdGeomScope prims with authored scene description below them that is only shading information.
  • It might be a bit more robust to check whether the child prim is compatible with UsdShadeConnectableAPI rather than looking specifically at its node graph-ness or shader-ness.

So with those two small tweaks, something like this?:

    // If this scope contains only UsdShade nodes, just skip.
    bool hasShadingData = false;
    bool hasNonShadingData = false;
    for (const auto& child : usdPrim.GetChildren()) {
        if (UsdShadeConnectableAPI(child)) {
            hasShadingData = true;
        } else {
            hasNonShadingData = true;
            break;
        }
    }
    if (hasShadingData && !hasNonShadingData) {
        return false;
    }

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 8, 2020
@kxl-adsk kxl-adsk merged commit 1439297 into dev Dec 9, 2020
@kxl-adsk kxl-adsk deleted the t_gamaj/MAYA-106079/skip_looks branch December 9, 2020 11:25
dj-mcg added a commit to dj-mcg/maya-usd that referenced this pull request Feb 12, 2022
Fixes a bug where UsdLuxLight prims underneath a Scope
were not being imported properly (if the Scope only had light prims
underneath it).

The import logic for "Scope" uses a heuristic to see if that scope is a
"Looks" or "Materials" scope.  It did this by checking if any of the
children nodes were UsdShadeConnectableAPI.  Unfortunately, lights (and
other non-shading prims) can be UsdShadeConnectableAPI.

This new heursistic checks for Material prims.  Note, this was more or
less the original version that Autodesk proposed in:
Autodesk#985
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