-
Notifications
You must be signed in to change notification settings - Fork 202
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-102918 Adding a basic Export Translator to the MayaUSD plugin #216
Conversation
… : at the end of some labels for consistency.
|
||
MStringArray filteredTypes; | ||
// Get the options | ||
if ( optionsString.length() > 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the spaces between the brackets and the content? I don't see this style in the rest of this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was only because I did most of the work in one editor that was automatically enforcing a style, and the flipped to another for a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually...it was like this because that's also how it is in the Pixar plugin code I copied it from.
|
||
#include <string> | ||
|
||
PXR_NAMESPACE_USING_DIRECTIVE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not super familiar with this but why are we using PXR_NAMESPACE_USING_DIRECTIVE here while the importer uses PXR_NAMESPACE_OPEN_SCOPE ?
You're also not using PXR_NAMESPACE_OPEN_SCOPE in the .h file while the importer does. Which one is the way to go? It'd be nice to follow the same pattern on both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the importer is incorrect. So far both import and export are basically copied from the Pixar plugin and I think the source for the ADSK importer was copy/pasted from Pixar and then edited a bit but the Pixar namespace wasn't changed. PXR_NAMESPACE_OPEN_SCOPE puts the class into the pxr namespace but the ADSK plugin has it’s own. I also copy/pasted a lot of what the Pixar plugin has in its export code, but when I went with the MayaUSD namespace for the exporter I had to either add a “pxr::” to the start of some of the functions or just use that macro to add a “using pxr”.
@@ -37,20 +109,138 @@ global proc int mayaUsdTranslatorExport (string $parent, | |||
// resultCallback ( string $optionsString ) | |||
// | |||
// Returns: | |||
// 1 if successful. | |||
// 1 if successfull. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is wrong
// The optionsString is of the form: | ||
// varName1=value1;varName2=value2;... | ||
// | ||
// Parameters: | ||
// $parent - the elf parent layout for this options layout. It is | ||
// always a scrollLayout. | ||
// $action - the action that is to be performed with this invocation of this proc. | ||
// $action - the action that is to be performed with this invokation of this proc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...I didn't even notice these two "typo" fixes. I didn't change those myself but I'll revert them both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just like the spacing above, these two typos actually came over from the equivalent code in the Pixar plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments
There was a problem hiding this 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
entries.push_back("startTime=1"); | ||
entries.push_back("endTime=1"); | ||
entries.push_back("frameStride=1.0"); | ||
defaultOptions = TfStringJoin(entries, ";"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lordy loo! Talk about convoluted! Just use a std::ostringstream
, and build the string in one go. Better yet, use our code generators that handle all of this boiler plate stuff for you ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I guess instead of a vector of strings with a join at the end we could use ostringstream. I’ll take a look at simplifying this one.
One general comment to make about using any of the AL utility code… At this point we have moved a lot of the Pixar translator code down into the core lib, so pretty much all of the options defined down there are still exactly how Pixar has had them, and this Default Dictionary stuff is all Pixar right now, but not much of the AL code has been moved there yet. I don’t think anyone is avoiding using AL code but a lot it isn’t available just yet in the shared area.
$currentOptions = mayaUsdTranslatorExport_AppendFromCheckbox($currentOptions, "exportInstances", "exportInstancesCheckBox"); | ||
$currentOptions = mayaUsdTranslatorExport_AppendFromCheckbox($currentOptions, "exportVisibility", "exportVisibilityCheckBox"); | ||
$currentOptions = mayaUsdTranslatorExport_AppendFromCheckbox($currentOptions, "mergeTransformAndShape", "mergeTransformAndShapeCheckBox"); | ||
$currentOptions = mayaUsdTranslatorExport_AppendFromCheckbox($currentOptions, "stripNamespaces", "stripNamespacesCheckBox"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, we've banned people writing boiler plate MEL code here. It takes too much dev effort, it's bug prone, and it's usually trivial to auto generate the code. For reference, this is the method that generates our import/export dialogs:
And here's an example of how we're specifying the options:
https://github.com/Autodesk/maya-usd/blob/dev/plugin/al/lib/AL_USDMaya/AL/usdmaya/fileio/ExportTranslator.h#L74
All of our AE templates, menus, command dialogs, export dialogs are all autogenerated. The code generators are all within our mayautils lib here:
https://github.com/Autodesk/maya-usd/tree/dev/plugin/al/mayautils/AL/maya/utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boilerplate is when you write/copy the exact same chunk of code over and over again. Extracting a chunk of code into its own function and then calling that function in multiple places is the opposite of “boilerplate code” is it not? That is what’s going on here, each of the checkbox’s or float fields call the exact same function with their option/control names passed in.
I’ve actually had to debug a bit of the C++ auto-generated MEL in the AL plugin and I’ll admit that I wasn’t a fan of that experience. I’m not the original author of most of this so I don’t have a really strong opinion about a lot of it (almost everything here comes from the Pixar plugin), but I’m definitely not anti-MEL and wouldn’t want to move all of this into compiled C++ for no real benefit. E.g. everything in PluginTranslatorOptions.cpp is actually MEL…a developer had to write all of that in MEL, debug all of that in MEL, before copying it into C++, adding a ton of casts, making sure they got all of their strings quoted correctly, before compiling it. I’m not sure I’d agree with that approach being easier and less bug prone. I’m sure that once it’s done it’s pretty much never going to change again, but I think it’s the same for the functions here.
The one thing I would like to change is to move some of this into a shared file because I think there are exact copies of all of this in the import .mel file.
Now I have to say Rob that it’s quite ironic that you and I are talking about MEL today because I just spent an hour looking at your web page. A Maya user had hacked together a script based on some snippets they grabbed from your MEL tutorials and couldn’t figure out how to make it all work so they contacted our Support for help. At least I’m assuming it’s your page…I can’t imagine there being more than one Maya expert named RobTheBloke 😊
timeInterval, std::set<double>(), frameStride); | ||
PXR_NS::UsdMayaJobExportArgs jobArgs = PXR_NS::UsdMayaJobExportArgs::CreateFromDictionary( | ||
userArgs, dagPaths, timeSamples); | ||
for (unsigned int i=0; i < filteredTypes.length(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop condition depends on a DSO exported function. Take a local copy.
for (unsigned int i=0, filteredTypes.length(); i < n; ++i)
|
||
// Convert selection list to jobArgs dagPaths | ||
UsdMayaUtil::MDagPathSet dagPaths; | ||
for (unsigned int i=0; i < objSelList.length(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop condition depends on a DSO exported function. Take a local copy.
for (unsigned int i=0, n = objSelList.length(); i < n; i++)
continue; | ||
} | ||
|
||
std::string argName(theOption[0].asChar()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you taking a copy? MString already has support for == "textString".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it was originally done this way because it's used with the VtDictionary in the final 'else' case, which needs the std::string.
MStringArray optionList; | ||
MStringArray theOption; | ||
optionsString.split(';', optionList); | ||
for(int i=0; i<(int)optionList.length(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loop condition depends on a DSO exported function. Take a local copy of the length.
const MString &optionsString, | ||
MPxFileTranslator::FileAccessMode mode ) { | ||
|
||
std::string fileName(file.fullName().asChar(), file.fullName().length()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const?
Adjusted some of the UI options to match design spec. Changed the wording of a few options. Changed one from a checkbox to an option menu. Added annotations to most of the options. Added a new frameSample field in the UI to set multi-sample option and made it settable in the exporter.
#include <string> | ||
#include <set> | ||
#include <sstream> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following our new convention of putting these at the top.
@@ -57,6 +61,8 @@ UsdMayaExportTranslator::writer(const MFileObject &file, | |||
double frameStride = 1.0; | |||
bool append=false; | |||
|
|||
std::set<double> frameSamples; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A default/empty set was always passed in down below, now parsing the input options to see if the user set them in the UI.
@@ -184,25 +203,23 @@ UsdMayaExportTranslator::GetDefaultOptions() | |||
static std::string defaultOptions; | |||
static std::once_flag once; | |||
std::call_once(once, []() { | |||
std::vector<std::string> entries; | |||
std::ostringstream optionsStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to an ostringstream as suggested.
Apologies for leaving this one open for so long. It was originally intended for a Maya PR but there were issues that paused the integration and I temporarily moved on to other things. I think I have made all requested changes now, but in the meantime I did add one new option + made some changes to align with the design spec which you will see here. The new option is a text field line to allow users to type in multi-sample #’s, which is an “advanced” option that people are probably not used to setting in any UI’s right now. Once we add actual commands for Import/Export and people can script this behavior (which I think is how people have been doing it with the Pixar plugin) then we might come back to this UI, but for now we wanted something in to allow people to test the behavior. Can you take another look @pilarmolinalopez and @robthebloke ? Thx! |
Adding a new Export Translator for the MayaUSD plugin. Contains a minimal set of export options plus the UI to set them. For the most part this is a direct copy of the Pixar options with only minor adjustments to how they show up in the export options window, which was easy enough to do since we have based the current read/write jobs in the core library on the Pixar plugin.