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

Iterating maya underworld when exporting #135

Conversation

sirpalee
Copy link
Contributor

@sirpalee sirpalee commented Jan 5, 2017

Description of Change(s)

The change adds the ability to iterate objects in the Maya underworld during USD export from Maya.

Fixes Issue(s)

Objects are not exported from the underworld. This happens if somebody writes an exporter plugin for imagePlane.

@sirpalee sirpalee changed the title Iterating maya underworld Iterating maya underworld when exporting Jan 5, 2017
@pmolodo
Copy link
Contributor

pmolodo commented Feb 3, 2017

Squashed the three commits into one, for cleaner history

Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

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

Hi @sirpalee, @elrond79, our sets team had a few code review notes that I've added below.

There was one larger issue though that we wanted to talk about. With this change, the exporter for NurbsSurface is now exporting additional prims for the transform and shape nodes from the underworld. We have a test case that we're hoping to clean up and release so you can see the differences yourselves, but here's an example of an exported USD file with this change:

def NurbsPatch "SphereCubic"
{
# All of the below prims come from the underworld and were not being exported before.
def "SphereCubicShape"
{
def Xform "projectionCurve11"
{
def NurbsCurves "projectionCurve11_1"
{
# ...attributes for curve ...
}
}
# more Xform and Curves prims
}

}

We're not sure this is desirable -- we typically don't export prims beneath gprims like this. We're wondering if other people would actually want these prims exported, or if we need to add some kind of opt-in mechanism so that your image plane exporter would export things from the underworld, but other exporters would not by default.

@@ -215,11 +217,24 @@ bool usdWriteJob::beginJob(const std::string &iFileName,
// This dagPath IS one of the arg dagPaths. It AND all of its
// children should be included in the export.
curLeafDagPath = curDagPath;
} else if (!MFnDagNode(curDagPath).hasParent(curLeafDagPath.node())) {
// This dagPath is not a child of one of the arg dagPaths, so prune
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we restore this comment with the appropriate modifications for the underworld?

// it and everything below it from the traversal.
itDag.prune();
continue;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about simplifying this block by pulling the logic in this else clause into a static helper function in this cpp file, like:

static bool
_HasParent(const MDagPath &dagPath, const MDagPath& dagPathParent)
{
    // code from else block here
}

and then restoring the else-if statement like:

else if (!_HasParent(curDagPath, curLeafDagPath)) {
    // comment goes here
    itDag.prune();
    continue;
}

// We may want to have another option that allows us to drop namespace's
// instead of making them part of the path.
std::replace( usdPathStr.begin(), usdPathStr.end(), ':', '_'); // replace namespace ":" with "_"
// Replacing characters because of underworld (transform|shapeNode->|underWorldNode)
// Because maya inserts "->|", we can safely eliminate both the "-" and ">"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the case that we just want to strip off the first "->" we see (reading from left-to-right), and everything afterwards? If so, would the following be simpler?

    usdPathStr = std::string(usdPathStr, 0, usdPathStr.find("->"))

Copy link
Contributor Author

@sirpalee sirpalee Mar 21, 2017

Choose a reason for hiding this comment

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

No, we want to get rid of -> and keep the rest.

I cleaned up the implementation a bit, first doing the removal then the replaces, kept the iterators and added a bit of extra comment to clarify it further. What do you think, is it easier to follow?

std::string usdPathStr(dagPath.fullPathName().asChar());
// We are keeping the iterators around, to avoid extra memory allocations.
// Replacing characters because of underworld (transform|shapeNode->|underWorldNode).
// Maya inserts "->|", we can safely eliminate both the "-" and ">", and keep the rest.
auto itBegin = usdPathStr.begin();
auto itEnd = std::remove(itBegin, usdPathStr.end(), '-');
itEnd = std::remove(itBegin, itEnd, '>');
// Replacing MDagPath separators with the USD ones.
std::replace(itBegin, itEnd, '|', '/');
// We may want to have another option that allows us to drop namespace's
// instead of making them part of the path.
std::replace(itBegin, itEnd, ':', '_');
return std::string(itBegin, itEnd);

@sirpalee
Copy link
Contributor Author

@sunyab I wasn't able to reproduce the nurbs issue locally (we are not much of a nurbs users), so I'm looking forward to the example. The rest of the issues were fixed and I pushed back a change. (I'll squash them later on, but keeping the changes separate for now)

@pmolodo
Copy link
Contributor

pmolodo commented Apr 7, 2017

I rebased / collapsed our changes, and pushed a fresh / clean copy. Look here for the full history:

https://github.com/LumaPictures/USD/tree/tg/simple_features/iterating_maya_underworld
LumaPictures@5794dc3

These are objects which maya be parented below shapes, like imagePlanes below
camera shapes
@jtran56
Copy link

jtran56 commented Jun 2, 2017

Filed as internal issue #147123.

@pmolodo
Copy link
Contributor

pmolodo commented Apr 9, 2018

So - was looking to fix up our PR to solve your issue (the nodes are created by projecting nurbs curves onto a nurbs surface, apparently). Decided the best approach was to limit the exported underworld nodes to be only image planes, since thats what we needed it for... and for various technical reasons dealing with how instances are handled*, it ends up simplifying things if we only need to worry about image planes / underworlds beneath cameras.

I actually went about implementing this... however, I realized that this PR then becomes useless, without our accompanying changes to export image planes! Since we're not quite ready to make a PR for that yet, I'm going to close this PR for now. We may reopen it (or open a new one) when / if we make a PR for our image plane stuff.

For reference, the changes to only export image planes from the underworld can be found here:

https://github.com/LumaPictures/USD/tree/tg/simple_features/iterating_maya_underworld2

Most of of the meat of it is in this commit: LumaPictures@5334f10

*Not sure if anyone else cares about the technical details, but for my own reference if nothing else: Initially I wanted to go with an opt-in approach for exporting types of nodes from the underworld, with a user-configurable option saying which node types you wanted. However, it turned out that handling this correctly is difficult, because of instances. Currently, instances are only exported from maya for shape nodes... because we then never have to worry about exporting instance hierarchies, ie, cases where there are nodes parented beneath the node that's being instanced. While both maya and usd obviously support this, it would significantly complicate the exporter to be able to handle this properly. If we allow exporting of any type of underworld node, we effectively need to solve this problem, as we no longer have the guarantee that shape nodes don't have children. However, if we say that the only type of underworld nodes we export are image planes underneath cameras, it sidesteps the issue because cameras are currently hard-coded to not support instancing on export.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants