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

Integrate AE work from sandbox into dev #142

Merged
merged 2 commits into from
Dec 3, 2019

Conversation

seando-adsk
Copy link
Collaborator

MAYA-102108 - Integrate AE work from sandbox into dev

  • Bringing over the new AE templates for USD types.
  • Also brought over a fix to the mayaUsdProxyShape AE template.

Added missing pieces from original merge of refactoring_sandbox into dev.

Changes based on comments from PR: #91

  • For these include file changes below mostly made the change only in 'lib/ufe'
    ** #include "mayaUsd/xxx" -> #include <mayaUsd/xxx>
    ** #include "maya/xxx" -> #include <maya/xxx>
    ** #include "ufe/xxx" -> #include <ufe/xxx>
    ** #include "pxr/xxx" -> #include <pxr/xxx>
  • reserve memory for vector

* Bringing over the new AE templates for USD types.
* Also brought over a fix to the mayaUsdProxyShape AE template.
…dev.

Changes based on comments from PR: #91
* For these include file changes below mostly made the change only in 'lib/ufe'
** #include "mayaUsd/xxx" -> #include <mayaUsd/xxx>
** #include "maya/xxx" -> #include <maya/xxx>
** #include "ufe/xxx" -> #include <ufe/xxx>
** #include "pxr/xxx" -> #include <pxr/xxx>
* reserve memory for vector
@seando-adsk seando-adsk requested a review from ppt-adsk November 29, 2019 17:13
@seando-adsk
Copy link
Collaborator Author

@robthebloke / @mattyjams - Can you please review this PR? It brings in the Attribute Editor templates (from refactoring_sandbox) and also address some of the concerns from: #91

I mostly focused on the comments about files in the "lib/ufe" folder.

@kxl-adsk kxl-adsk requested a review from mattyjams November 29, 2019 20:40
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.

The code looks good to me! Thanks Sean!

We don't currently have a PR build of Maya deployed that supports UFE V2, so I can't test it myself, but the changes make sense. I'm still a little unsure about our conventions for quotes vs. angle braces on header includes, but I punted on commenting on those assuming we'd address that later.

@seando-adsk
Copy link
Collaborator Author

@mattyjams In the other PR you commented on how we were including some headers. We agree with you that all things external (such as maya/ufe/pxr) should be included using <>. So I addressed a bunch of those, but mostly only in the lib/ufe folder. We'll tackle the rest of the folders later once we've merged more of refactoring_sandbox into dev.

@kxl-adsk kxl-adsk merged commit 5d5e2b7 into dev Dec 3, 2019
@kxl-adsk kxl-adsk deleted the donnels/MAYA-102108/integrate_ae_work_into_dev branch December 3, 2019 15:18
@mattyjams
Copy link
Contributor

Thanks @seando-adsk! Yeah, I definitely appreciate the follow-up from that earlier PR.

I had initially been thinking of it as quotes for "system" headers and angle braces for "non-system", but I'm realizing now that "system" is too ill-defined and your external vs. internal criteria makes more sense. Looking through this PR, I think two things caught my eye:

  • The pxr includes were changed to angle braces, but I actually think that's a good change. I'd gotten used to seeing these with quotes when core USD and our Maya code were in the same project, but with that no longer the case, angle braces are more appropriate. We'll just need to apply that to the rest of the code base over time.
  • There are a few includes of mayaUsd/mayaUsd.h which use angle braces which should maybe use quotes instead.

Sorry to be so pedantic about the little stuff. I'm just trying to help figure out the conventions we want to follow as we're all getting more active working in the dev branch.

Thanks again!

pmolodo pushed a commit to LumaPictures/maya-usd that referenced this pull request Jan 22, 2020
…e_not_defined

Post fix for earlier changes I made in lib/CMakeLists
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