PART V: clean up mayaUsdPlugin #392

HamedSabri-adsk · 2020-03-27T12:19:37Z

mayaUsdPlugin promotion headers are now created under mayaUsdPlugin and not mayaUsd
clean up include order and directive

…eLists.txt. - fix build/usage requirements for mayaUsdUtils and mayaUsd_Translators, testMayaUsdUI - don't rely on ${PROJECT_NAME} because of the folder restructure.

… and not mayaUsd - clean up include order and directive

pmolodo

So, as I discussed in part 4, I think that headers that aren't installed should be considered private, not promoted, and included with private-header syntax. Otherwise, what is considered a private header?

Also, as with last 2 parts, were associated header changes.

There's also the issue of what's considered 7. Your project’s headers vs 4. Other libraries’ headers - I go into more detail in one of my notes below.

plugin/adsk/plugin/ProxyShape.cpp

plugin/adsk/plugin/ProxyShape.h

plugin/adsk/plugin/CMakeLists.txt

pmolodo · 2020-04-01T00:24:08Z

plugin/adsk/plugin/ProxyShape.h


+#include <mayaUsd/base/api.h>


What requires this header? A quick search of the directory didn't show anything using MAYAUSD_CORE_*, MAYAUSD_MACROS_* or MAYAUSD_TEMPLATE_*.

plugin/adsk/plugin/exportTranslator.cpp

plugin/adsk/plugin/plugin.cpp

pmolodo · 2020-04-01T00:39:32Z

plugin/adsk/plugin/plugin.cpp

-#include "importTranslator.h"
-#include "exportTranslator.h"
-#include "ProxyShape.h"
+#include <mayaUsd/base/api.h>


plugin/adsk/plugin/plugin.cpp

pmolodo · 2020-04-01T00:40:15Z

plugin/adsk/plugin/plugin.cpp


 #include <mayaUsd/utils/undoHelperCommand.h>
 #if defined(WANT_QT_BUILD)
-#include <mayaUsd/ui/cmds/USDImportDialogCmd.h>
+#include <mayaUsdUI/ui/cmds/USDImportDialogCmd.h>
 #endif

 #if defined(WANT_UFE_BUILD)
 #include <mayaUsd/ufe/Global.h>
 #endif


Conditional includes at end

plugin/adsk/plugin/plugin.cpp

mattyjams

I agree with all of @elrond79's feedback.

If the private headers have to be promoted so that they end up in the build tree, then I'm ok with that, but we should definitely be including them using double-quotes and file-relative paths. Only publicly available headers should be in angle braces.

HamedSabri-adsk · 2020-04-01T12:48:49Z

I read through all of the comments in all 5 PRs and hopefully I haven't missed anything.

Thank you @elrond79 @mattyjams @ppt-adsk @seando-adsk again for all the great and detailed feedback.

Header order, blank spaces

My original goal in this large PR was to eliminate things that clang-format couldn't handle (eliminating relative paths) and I honestly spent little on cosmetics and let clang-format later catch them. I do also understand that we have cases where clang-format can't be smart enough to handle certain ordering.

If people are Ok, I can spend a bit more time on some of the cosmetic ordering so at least it is as close as possible to the coding guideline since I am have some changes to make in this area anyways. Any thoughts?

Associated headers

I am glad we all have the same understanding/agreement. Internally we have an action item to clarify this a bit more in the coding guideline document.

Header promotion

I do agree with all the comments around not promoting private headers. One of the main reasons we have the private header files promoted is just so that they simply end up in the build tree and help including them using the full path inside their own project (both in associated cpp or any other files).

#include "mayaUsd/render/vp2RenderDelegate/draw_item.h"
vs simply
#include "draw_item.h"

To sum it up, we have 2 solutions:

A- don't promote private headers ( headers that don't installed ) and include them using quotes and no full-path which would also simplifies the CMakelists.txt
e.g

draw_item.cpp
#include "draw_item.h"

basisCurves.cpp
#include "draw_item.h"

B- promote private headers ( headers that don't installed ) and include them using quotes and full-path

#include "mayaUsd/render/vp2RenderDelegate/draw_item.h"

Any Thoughts?

kxl-adsk · 2020-04-01T13:50:17Z

Re Header promotion - applying guidelines is always a great test and I agree we should reword the guideline for private header files to be more explicit about no full-path. Let's do this once we merge these 5 big PRs.

For your question, option A is aligned with our guidelines.

pmolodo · 2020-04-01T18:05:07Z

Sounds good - made a PR to clarify guideline: #400

…code guideline.

pmolodo · 2020-04-03T18:26:27Z

The new changes look good - I marked most of my comments as resolved. Are still a few items left unadressed though - the use of <mayaUsd/base/api.h>, and the location of the conditional includes (which I don't think will be automatically moved by clang-format).

HamedSabri-adsk · 2020-04-03T19:00:58Z

The new changes look good - I marked most of my comments as resolved. Are still a few items left unadressed though - the use of <mayaUsd/base/api.h>, and the location of the conditional includes (which I don't think will be automatically moved by clang-format).

Thanks. I will address whatever is left which shouldn't be as many once we have these 5 PRs merged.

mattyjams

Looks good to me! Just one cosmetic note, but likely addressed by clang-format.

Thanks @HamedSabri-adsk!

mattyjams · 2020-04-06T20:25:51Z

plugin/adsk/plugin/importTranslator.cpp

 #include <mayaUsd/fileio/jobs/jobArgs.h>
 #include <mayaUsd/fileio/jobs/readJob.h>
-#include <mayaUsd/fileio/shading/shadingModeRegistry.h>
 #include <mayaUsd/fileio/jobs/writeJob.h>
-
-#include "pxr/base/gf/interval.h"
-#include "pxr/base/vt/dictionary.h"
+#include <mayaUsd/fileio/shading/shadingModeRegistry.h>


The mayaUsd group here should move to the bottom after the pxr group.

Following the merge of PRs Autodesk#388 through Autodesk#392, the header reorganization created some new sites where M3dView.h was being included earlier than sdf/types.h. This change applies an existing pattern of including sdf/types.h early to work around this issue and fix compiling for Maya 2019 (and earlier).

… GL. (#392)

Hamed Sabri added 4 commits March 27, 2020 07:32

- All subdirectories under lib/mayaUsd and lib/usd get their own CMak…

5d11e6f

…eLists.txt. - fix build/usage requirements for mayaUsdUtils and mayaUsd_Translators, testMayaUsdUI - don't rely on ${PROJECT_NAME} because of the folder restructure.

Fix include directives and order under lib/usd.

49028a2

Fix include directives and order under lib/mayaUsd.

dfc4e31

- mayaUsdPlugin promotion headers are now created under mayaUsdPlugin…

9d6b5fe

… and not mayaUsd - clean up include order and directive

HamedSabri-adsk added the build Related to building maya-usd repository label Mar 27, 2020

HamedSabri-adsk mentioned this pull request Mar 27, 2020

hdMaya: Adopt the include directive/order according to coding guideline. #379

Closed

pmolodo suggested changes Apr 1, 2020

View reviewed changes

mattyjams suggested changes Apr 1, 2020

View reviewed changes

pmolodo mentioned this pull request Apr 1, 2020

codingGuidelines.md: clarify some details about headers #400

Merged

UPDATE: include directives and orders under plugin/adsk according to …

3324c61

…code guideline.

HamedSabri-adsk requested review from pmolodo and mattyjams April 3, 2020 17:23

pmolodo mentioned this pull request Apr 3, 2020

PART II: CMake clean ups under lib/mayaUsd and lib/usd #389

Merged

HamedSabri-adsk merged commit fc11cb0 into sabrih/MAYA-104002/clean_up_part_1 Apr 6, 2020

HamedSabri-adsk deleted the sabrih/MAYA-104002/clean_up_part_5 branch April 6, 2020 20:25

mattyjams approved these changes Apr 6, 2020

View reviewed changes

mattyjams mentioned this pull request Apr 9, 2020

add pre-emptive includes of sdf/types.h before M3dView.h for Maya 2019 #414

Merged

ppt-adsk pushed a commit that referenced this pull request May 30, 2023

HYDRA-283 : Fix crash when going back and forth between VP2 and Hydra…

c70dffd

… GL. (#392)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PART V: clean up mayaUsdPlugin #392

PART V: clean up mayaUsdPlugin #392

HamedSabri-adsk commented Mar 27, 2020

pmolodo left a comment

pmolodo Apr 1, 2020

pmolodo Apr 1, 2020

pmolodo Apr 1, 2020

mattyjams left a comment

HamedSabri-adsk commented Apr 1, 2020

kxl-adsk commented Apr 1, 2020

pmolodo commented Apr 1, 2020

pmolodo commented Apr 3, 2020

HamedSabri-adsk commented Apr 3, 2020

mattyjams left a comment

mattyjams Apr 6, 2020

PART V: clean up mayaUsdPlugin #392

PART V: clean up mayaUsdPlugin #392

Conversation

HamedSabri-adsk commented Mar 27, 2020

pmolodo left a comment

Choose a reason for hiding this comment

pmolodo Apr 1, 2020

Choose a reason for hiding this comment

pmolodo Apr 1, 2020

Choose a reason for hiding this comment

pmolodo Apr 1, 2020

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Apr 1, 2020

kxl-adsk commented Apr 1, 2020

pmolodo commented Apr 1, 2020

pmolodo commented Apr 3, 2020

HamedSabri-adsk commented Apr 3, 2020

mattyjams left a comment

Choose a reason for hiding this comment

mattyjams Apr 6, 2020

Choose a reason for hiding this comment