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

Address remaining feedback in lib/usd and plugin/adsk #408

Conversation

HamedSabri-adsk
Copy link
Contributor

Hey guys,

This PR fixes conditional orders in lib/usd and plugin/adsk.

Only 6 files :)

@HamedSabri-adsk HamedSabri-adsk added the build Related to building maya-usd repository label Apr 7, 2020
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! Just noted two places where hdMaya and mayaUsd headers should be combined into one group.

lib/usd/hdMaya/adapters/proxyAdapter.cpp Outdated Show resolved Hide resolved
lib/usd/hdMaya/delegates/proxyDelegate.cpp Outdated Show resolved Hide resolved
Hamed Sabri added 2 commits April 6, 2020 20:55
- Fix private headers in translator promoted as public
- Fix grouping order
@HamedSabri-adsk
Copy link
Contributor Author

@mattyjams see c6eecc5 that addresses all your feedback in PART III.

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 noted a few more things, but nothing that should block any of the open PRs. I think we should probably stop at this point and merge in everything we have, and we can take another pass through afterwards. It's becoming too difficult to cross reference all of the notes across many open pull requests each with multiple commits.

@@ -31,7 +31,7 @@
#include <mayaUsd/utils/colorSpace.h>
#include <mayaUsd/utils/util.h>

#include <mayaUsd_Translators/meshWriter.h>
#include "meshWriter.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

meshWriter.h to me is the related header for this file, so I'd think it should go at the top in a group of its own.

#include <mayaUsd_Translators/jointWriter.h>
#include <mayaUsd_Translators/meshWriter.h>
#include "jointWriter.h"
#include "meshWriter.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

meshWriter.h is the related header so it should go at the top.

@@ -19,7 +19,7 @@
#include <pxr/pxr.h>
#include <pxr/usd/usdGeom/mesh.h>

#include <mayaUsd_Translators/meshWriter.h>
#include "meshWriter.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

meshWriter.h is the related header so it should go at the top.

@@ -44,6 +35,15 @@
#include "importTranslator.h"
#include "ProxyShape.h"

#include <mayaUsd/utils/undoHelperCommand.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

This include is not part of a conditional and should stay with the mayaUsd group above.

@HamedSabri-adsk HamedSabri-adsk removed the request for review from pmolodo April 7, 2020 19:26
@HamedSabri-adsk HamedSabri-adsk merged commit ddec783 into sabrih/MAYA-104002/clean_up_part_1 Apr 7, 2020
@HamedSabri-adsk HamedSabri-adsk deleted the sabrih/sabrih/address_lib_usd_plugin_adsk_remaning_issues branch April 7, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Related to building maya-usd repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants