PART IV: Clean up include directives and order under lib/mayaUsd #391

HamedSabri-adsk · 2020-03-27T12:17:22Z

Clean up include directives and orders under lib/mayaUsd

ppt-adsk

Awesome, massive job Hamed. I don't know how much was scripted and how much was brute force, but either way it's quite impressive!

Some of my comments are for my education, but I believe I've caught a few actual issues, so have a look.

ppt-adsk · 2020-03-27T18:09:51Z

lib/mayaUsd/render/pxrUsdMayaGL/hdImagingShapeUI.h


-PXR_NAMESPACE_OPEN_SCOPE
+// XXX: On Linux, some Maya headers (notably M3dView.h) end up indirectly


@HamedSabri-adsk you sure about this change? The comment specifically states that sdf/types.h should be included before M3dView.h, otherwise you may get problems on an X11 system.

Hey @ppt-adsk , great catch. I am curious about this too. Do we know if this is really the case or at least if it has been fixed in recent versions of API? I can put it back before M3dView.h if you guys think it is necessary.

@mattyjams Do you know the history about this?

I ran across this as well at some point - and since the problem is really more with a system header, X11/Xlib.h, I doubt this has gone away (at least not for us, since we're still using the same Linux distribution / version!) - it would only no longer be an issue if M3dView stopped including X11/Xlib.h. Even if it has, we'd have to put in a conditional include, at which point it seems simpler to just force the ordering.

Unfortunately, this would deviate from the "standard" ordering. Does anyone know how clang-format handles #includes with comments above them? I'm fairly certain it's intelligent about not reordering #includes inside of conditional blocks... but don't remember how it handles #include comments like this. (Worst case, we'd have to protect with // clang-format off.)

Alternatively - it's not too late to swap the order of pxr and maya - the ordering of pxr vs maya headers was pretty arbitrary, and if we have a known case where pxr needs to come first, that seems as good a reason as any to say pxr then maya. We'd have to alter doc/codingGuidelines.md, but that hasn't been released for long (and never on master branch), so I don't see that as a roadblock.

Yeah, this was definitely an issue at least with Maya 2018. I think this is what was happening there:

maya/M3dView.h included maya/MNativeWindowHdl.h

maya/MNativeWindowHdl.h included X11/Intrinsic.h

X11/Intrinsic.h included X11/Xlib.h

X11/Xlib.h did: #define Bool int

Taking a look at our Maya 2019 installations, it looks like maya/MNativeWindowHdl.h does not include X11/Intrinsic.h anymore, so this may no longer be an issue with 2019+, but it would still be an issue with 2018.

We already have a similar issue with GLEW headers needing to come before GL ones, so it doesn't particularly offend me to have to deviate from the convention here. As @elrond79 noted though, we'd want to make sure we can get clang-format to ignore it somehow.

Hmm, good to know that this shouldn't be a problem >= 2019! Maybe we can move it after, and add a note so we can remove the speical handling once we drop 2018 support?

ppt-adsk · 2020-03-27T18:24:38Z

lib/mayaUsd/ufe/StagesSubject.cpp

@@ -35,17 +36,17 @@
 #endif
 #endif

+#include <vector>
+
+#ifdef UFE_V2_FEATURES_AVAILABLE


Pretty sure this is wrong: by moving the UFE_V2_FEATURES_AVAILABLE you've made MSceneMessage.h et al. dependent on it, which is incorrect. I would move the unordered_map include just after line 36, and not be so fussed up about separating include headers by their origin: keeping related things together is also very important.

Oops!! good catch. will fix it soon.

Please see 210a2ab, c05eb7f

So, I can't see what the original change was... if it changed what was actually included, though, that's clearly not a good thing.

However, I would like to see all conditional includes moved after all non-conditional includes, which is what we agreed to in the style guidelines. That would make the include blook look more like this:

#include "StagesSubject.h" #include <vector> #include <maya/MSceneMessage.h> #include <maya/MMessage.h> #include <ufe/hierarchy.h> #include <ufe/path.h> #include <ufe/scene.h> #include <ufe/sceneNotification.h> #include <ufe/transform3d.h> #include <pxr/usd/usdGeom/tokens.h> #include <pxr/usd/usdGeom/xformOp.h> #include <mayaUsd/ufe/ProxyShapeHandler.h> #include <mayaUsd/ufe/UsdStageMap.h> #include <mayaUsd/ufe/Utils.h> #include "private/InPathChange.h" #ifdef UFE_V2_FEATURES_AVAILABLE #include <unordered_map> #include <ufe/attributes.h> #include <ufe/object3d.h> #if UFE_PREVIEW_VERSION_NUM >= 2010 #include <ufe/object3dNotification.h> #endif #endif

You've lost a bit of association of some of the conditional headers with non-conditional headers, but we've made it much easier to see at a glance which headers will be included under what conditions, which I personally view as more important. It's also easier to standardize, which I like.

ppt-adsk · 2020-03-27T18:28:14Z

lib/mayaUsd/ufe/UsdRotatePivotTranslateUndoableCommand.cpp


-#include "UsdRotatePivotTranslateUndoableCommand.h"
-#include "private/Utils.h"
+#include <mayaUsd/ufe/private/Utils.h>


This one looks wrong: if Utils.h is private and therefore not promoted, we shouldn't be including it with angle brackets.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:28:50Z

lib/mayaUsd/ufe/UsdRotateUndoableCommand.cpp

-#include "private/Utils.h"
+#include <mayaUsd/ufe/UsdRotateUndoableCommand.h>
+
+#include <mayaUsd/ufe/private/Utils.h>


Same here: private unpromoted file should use double quotes.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:29:13Z

lib/mayaUsd/ufe/UsdScaleUndoableCommand.cpp

-#include "Utils.h"
+#include <mayaUsd/ufe/UsdScaleUndoableCommand.h>
+
+#include <mayaUsd/ufe/private/Utils.h>


Private file, double quotes.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:31:09Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-#include "private/InPathChange.h"
+#include <mayaUsd/ufe/UsdUndoRenameCommand.h>
+
+#include <mayaUsd/ufe/private/InPathChange.h>


Private file, should not be promoted, include with double quotes.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:32:03Z

lib/mayaUsd/ufe/private/CMakeLists.txt

+# -----------------------------------------------------------------------------
+# promoted headers
+# -----------------------------------------------------------------------------
+mayaUsd_promoteHeaderList(HEADERS ${headers} SUBDIR ufe/private)


I don't think we should be promoting these headers.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:32:38Z

lib/mayaUsd/ufe/private/Utils.cpp

@@ -14,16 +14,16 @@
 // limitations under the License.
 //

-#include "Utils.h"
+#include <mayaUsd/ufe/private/Utils.h>


Should revert this change, should include with double quotes.

Fixed in fb592d1

ppt-adsk · 2020-03-27T18:47:42Z

lib/usd/schemas/CMakeLists.txt

@@ -180,7 +180,7 @@ target_link_libraries(${TARGET_PYTHON_NAME}
 # run-time search paths
 # -----------------------------------------------------------------------------
 if(IS_MACOSX OR IS_LINUX)
-    mayaUsd_init_rpath(rpath "${PROJECT_NAME}")
+    mayaUsd_init_rpath(rpath mayaUsd)


For my education, why is this hard-coded as mayaUsd?

Since we restructured lib sub-directories, ${PROJECT_NAME} no longer returns "mayaUsd" and rather "maya-usd" which would be wrong. I can make a global variable at the super project level. I also don't like the hard-coded names.

@ppt-adsk
Please see more info on this here:
#389 (comment)

ppt-adsk · 2020-03-27T18:48:49Z

lib/usd/schemas/MayaReference.h

@@ -17,22 +17,22 @@
 #define MAYAUSD_SCHEMAS_GENERATED_MAYAREFERENCE_H

 /// \file mayaUsd_Schemas/MayaReference.h
+#include <mayaUsd_Schemas/api.h>


Just wondering, could the directory have been mayaUsd/schemas, or is there specific thinking that went into calling it mayaUsd_Schemas?

It's a separate shared library (not part of mayaUsd). We have some cleanup to do in the actual name, for now it was just matching how translators are named. Good news, _ is not allowed, so we will have to rename it at one point.

Thanks for the info.

HamedSabri-adsk · 2020-03-27T21:14:56Z

Awesome, massive job Hamed. I don't know how much was scripted and how much was brute force, but either way it's quite impressive!

Some of my comments are for my education, but I believe I've caught a few actual issues, so have a look.

@ppt-adsk , Merci Monsieur, Je vous remercie pour vos gentil mots.

My understanding was that we would go with full-path+<> for both inside and outside projects. @kxl-adsk correct me if I am wrong.

Regarding the "promoted header", we have always applied this mechanism inside the project both for private ( files that don't need to be installed ) and public ( files that are installed ).
Sorry for the short answer but let's chat offline and I am more than happy to walk you through the thought process.

Having said that, I believe there is a bug that you pointed out. I shouldn't to be installing below directory. I will fix it.

ufe
 private
    Utils.h
    InPathChange.h

…me when in the same folder. Private headers may live in sub-directories, but they should never be included using "." or ".." as part of a relative paths. For example: #include "privateUtils.h" #include "pvt/helperFunctions.h"

ppt-adsk

Changes from last 3 commits look good. Waiting on the last changes (X11-related include order, incorrect WANT_UFE includes), but otherwise looking good.

pmolodo

Again, thanks for this massive effort!

Like Part III, there's also an issue here with associated headers, as @seando-adsk pointed out. I've marked a few examples, but didn't find all instances. Reading the codingGuidelines.md, I'm realizing that it didn't make this as clear as it could have - the example clearly places the associated header as @seando-adsk and I would like, but the preceding description doesn't note an exception for associated headers for the public-header-style rule.

We also might want to clarify that private headers should come in a group after everything else (except conditionals) - ie:

8. Private headers
9. Condtional includes

Other than that, the only thing I noticed that needed changing was the conditional includes, that @ppt-adsk also commented on. I left my own take in that thread.

Finally - you said that you folks traditionally always promoted private as well as public headers. Now that we've switched to a convention where all all private headers must in in the same dir or a subdir of the files including them, I don't think this should be necessary anymore... and it will make things clearer (and simplify the CMakeLists.txt a bit) if the set of installed headers is generally the same as the set of promoted (ie, we don't promote private). Thoughts?

lib/usd/hdMaya/adapters/adapter.cpp

lib/usd/hdMaya/adapters/adapterDebugCodes.cpp

lib/usd/hdMaya/adapters/adapterRegistry.cpp

mattyjams

Thanks again for all of this @HamedSabri-adsk!

No real significant feedback beyond what @ppt-adsk and @elrond79 have already identified. I stopped commenting on the cosmetic stuff assuming that clang-format will eventually address anything we haven't already caught.

The one thing I think might still be needed is the include ordering for M3dView.h/X11 if we want to continue to support Maya 2018.

mattyjams · 2020-04-01T01:07:24Z

lib/mayaUsd/fileio/chaser/chaser.h

@@ -16,16 +16,14 @@
 #ifndef PXRUSDMAYA_CHASER_H
 #define PXRUSDMAYA_CHASER_H

-/// \file usdMaya/chaser.h


Would be good to leave these Doxygen tags if we can. You can truncate it to just this:

/// \file

Same goes for all of the files below.

lib/mayaUsd/fileio/chaser/chaser.h

lib/mayaUsd/fileio/chaser/chaserRegistry.h

lib/mayaUsd/fileio/instancedNodeWriter.cpp

lib/mayaUsd/fileio/instancedNodeWriter.h

lib/mayaUsd/fileio/shading/shadingModePxrRis.cpp

lib/mayaUsd/fileio/shading/shadingModePxrRis_rfm_map.h

lib/mayaUsd/render/pxrUsdMayaGL/batchRenderer.cpp

lib/mayaUsd/render/pxrUsdMayaGL/batchRenderer.h

mattyjams · 2020-04-01T01:47:02Z

lib/mayaUsd/render/pxrUsdMayaGL/hdImagingShapeUI.h


-PXR_NAMESPACE_OPEN_SCOPE
+// XXX: On Linux, some Maya headers (notably M3dView.h) end up indirectly


Yeah, this was definitely an issue at least with Maya 2018. I think this is what was happening there:

maya/M3dView.h included maya/MNativeWindowHdl.h

maya/MNativeWindowHdl.h included X11/Intrinsic.h

X11/Intrinsic.h included X11/Xlib.h

X11/Xlib.h did: #define Bool int

Taking a look at our Maya 2019 installations, it looks like maya/MNativeWindowHdl.h does not include X11/Intrinsic.h anymore, so this may no longer be an issue with 2019+, but it would still be an issue with 2018.

We already have a similar issue with GLEW headers needing to come before GL ones, so it doesn't particularly offend me to have to deviate from the convention here. As @elrond79 noted though, we'd want to make sure we can get clang-format to ignore it somehow.

…code guideline.

… inside the project should be included using angle brackets

pmolodo

Ok, looks good to me after the latest fixes!

ppt-adsk

Great job Hamed, impressive amount of work.

mattyjams

Looks good to me! I noted a bunch more stuff, but the majority of it should be covered by clang-format, and as with #390 (review), I don't think any of it should block the merge.

Thanks again. @HamedSabri-adsk for all of the great effort here. Very much appreciated! :)

lib/mayaUsd/render/pxrUsdMayaGL/batchRenderer.cpp

lib/mayaUsd/render/pxrUsdMayaGL/batchRenderer.h

lib/mayaUsd/fileio/importData.h

lib/mayaUsd/fileio/primUpdaterRegistry.cpp

lib/mayaUsd/fileio/shading/shadingModeExporterContext.h

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

lib/mayaUsd/ufe/Utils.h

lib/mayaUsd/ufe/wrapUtils.cpp

lib/mayaUsd/utils/util.cpp

…4002/clean_up_part_4

This fleshes out the comment explaining why sdf/types.h needs to be included before M3dView.h for Maya 2019 and earlier based on the discussion in this pull request: Autodesk#391 (comment)

Fix include directives and order under lib/mayaUsd.

dfc4e31

HamedSabri-adsk requested review from pmolodo, mattyjams and ppt-adsk March 27, 2020 12:17

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

ppt-adsk requested changes Mar 27, 2020

View reviewed changes

Hamed Sabri added 3 commits March 28, 2020 08:46

feedback: Fix header inclusion.

210a2ab

put back the missing #ifdef.

c05eb7f

HamedSabri-adsk requested a review from ppt-adsk March 29, 2020 00:14

ppt-adsk reviewed Mar 30, 2020

View reviewed changes

pmolodo suggested changes Apr 1, 2020

View reviewed changes

lib/usd/hdMaya/adapters/adapter.cpp Outdated Show resolved Hide resolved

lib/usd/hdMaya/adapters/adapterDebugCodes.cpp Outdated Show resolved Hide resolved

lib/usd/hdMaya/adapters/adapterRegistry.cpp Outdated Show resolved Hide resolved

mattyjams suggested changes Apr 1, 2020

View reviewed changes

Hamed Sabri added 2 commits April 1, 2020 19:10

UPDATE: include directives and orders under lib/mayaUsd according to …

886119f

…code guideline.

UPDATE lib/mayaUsd: All included public header files from outside and…

db250b3

… inside the project should be included using angle brackets

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

pmolodo approved these changes Apr 3, 2020

View reviewed changes

ppt-adsk approved these changes Apr 3, 2020

View reviewed changes

mattyjams approved these changes Apr 6, 2020

View reviewed changes

Merge branch 'sabrih/MAYA-104002/clean_up_part_1' into sabrih/MAYA-10…

c4ffc1e

…4002/clean_up_part_4

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

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

mattyjams mentioned this pull request Apr 6, 2020

PART III: Clean up include directives and order under lib/usd #390

Merged

HamedSabri-adsk mentioned this pull request Apr 7, 2020

Address remaning feedbacks for lib/mayaUsd #407

Merged


		PXR_NAMESPACE_OPEN_SCOPE
		// XXX: On Linux, some Maya headers (notably M3dView.h) end up indirectly

PART IV: Clean up include directives and order under lib/mayaUsd #391

PART IV: Clean up include directives and order under lib/mayaUsd #391

Conversation

HamedSabri-adsk commented Mar 27, 2020

ppt-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 27, 2020

ppt-adsk left a comment

Choose a reason for hiding this comment

pmolodo left a comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmolodo left a comment

Choose a reason for hiding this comment

ppt-adsk left a comment

Choose a reason for hiding this comment

mattyjams left a comment

Choose a reason for hiding this comment

HamedSabri-adsk Mar 28, 2020 •

edited

Loading