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

EMSUSD-550 - Control the Types of Objects to Export #3338

Merged
merged 7 commits into from
Oct 2, 2023

Conversation

samuelliu-adsk
Copy link
Collaborator

No description provided.

self._ValidateLight()
self._ValidateCamera()
self._ValidateMeshCamera()
self._ValidateMeshLight()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need to test the case of export all three, that is already covered by other unit tests

@samuelliu-adsk samuelliu-adsk force-pushed the samuelliu-adsk/EMSUSD-550/export_types branch from 566dbc0 to 455f5ae Compare September 20, 2023 18:21
@samuelliu-adsk samuelliu-adsk marked this pull request as ready for review September 20, 2023 18:23
@@ -385,6 +385,39 @@ bool UsdMayaWriteJobContext::_NeedToTraverse(const MDagPath& curDag) const
// If we're not going to create a prim at curDag, then we do not need to
// traverse.
return false;
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the if returns, I'd not put the extra check in an else to avoid nesting. Other previous checks are not nested inside elses.

MStatus status;
MObject obj = shapeDagPath.node();
const MFnDependencyNode depFn(obj, &status);
if (!status) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IDK if failure to cerate a MFnDependencyNode should be treated as failure. A node not being a MFnDependencyNode is not an error, it just that the node does not support the API.

It should just skip the type check.


const std::string mayaTypeName(depFn.typeName().asChar());

if (mArgs.excludeExportTypes.count(TfToken("Meshes")) != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were the name of types part of the design? I ask because I find that using "Meshes" instead of "mesh" for a type name feels wrong. The type should not be plural.

In any cases, the string comparison should be case-insensitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea so I used "Meshes" to keep it consistent with the UX design. It designed to use "Meshes" and "Cameras" etc. But yea I can make it case-insensitive


if (stringArrayContains("geometry", $sectionNames)) {
$excludeExportTypes = mayaUsdTranslatorExport_SetSuboptionsFromCheckBox($excludeExportTypes, "", "Meshes", "exportMeshesCheckBox");
Copy link
Collaborator

Choose a reason for hiding this comment

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

The flag is passed explicitly here, so changing from "Meshes" to "mesh" seem feasible without modifying UI shown to teh user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know if that will confuse the user.. Like they see "Meshes" in the UI but when use it in command it changes to "mesh". They won't know it unless they read extra documents to get that information...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But yea its definitely feasible

Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk 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 aside from the few comments I left.

@@ -406,15 +404,18 @@ bool UsdMayaWriteJobContext::_NeedToTraverse(const MDagPath& curDag) const

const std::string mayaTypeName(depFn.typeName().asChar());

if (mArgs.excludeExportTypes.count(TfToken("Meshes")) != 0) {
if ((mArgs.excludeExportTypes.count(TfToken("Meshes")) != 0)
|| (mArgs.excludeExportTypes.count(TfToken("meshes")) != 0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be cleaner and more general to lowercase both strings and compare those.

pierrebai-adsk
pierrebai-adsk previously approved these changes Sep 26, 2023
@samuelliu-adsk samuelliu-adsk added adsk Related to Autodesk plugin import-export Related to Import and/or Export labels Sep 27, 2023
pierrebai-adsk
pierrebai-adsk previously approved these changes Sep 28, 2023
@samuelliu-adsk samuelliu-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 28, 2023
@@ -0,0 +1,123 @@
#!/usr/bin/env mayapy
#
# Copyright 2016 Pixar
Copy link
Collaborator

Choose a reason for hiding this comment

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

New file by you should be Copyright by Autodesk and have current year.

Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

Please fix the copyright in your new test file. Once you do, just mark as ready for merge (no re-run of the preflight needed).

@seando-adsk seando-adsk removed adsk Related to Autodesk plugin ready-for-merge Development process is finished, PR is ready for merge labels Sep 29, 2023
@samuelliu-adsk samuelliu-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 29, 2023
@seando-adsk seando-adsk merged commit 4c3c857 into dev Oct 2, 2023
9 checks passed
@seando-adsk seando-adsk deleted the samuelliu-adsk/EMSUSD-550/export_types branch October 2, 2023 13:42
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