Address remaning feedbacks for lib/mayaUsd

[HamedSabri-adsk]

Hi @elrond79 and @mattyjams ,

First of all thank you for your patience, detailed and thoughtful feedback, and very generous comments. It is really appreciated.

As you have noticed, I've merged PR 5,4,3,2 to 1 to make it easier to see where things are with all these new changes.

I am now creating 2 small PRs (one for lib/mayaUsd and one for lib/usd) to address the renaming issues that were brought up in previous PRs review.

The changes are not that many and hopefully this should get us closer to the finishing line:

Changes in this PR :

• fix conditional orders based on coding guideline
• fix unnecessary promotion of private headers
• fix "" vs <> for a few public header
• fix grouping/sorting

NOTE: @mattyjams I didn't address your concern around shaderFragments.h in this PR since it requires a little bit of change. I totally agree with your comment and will have it fixed once this big PR goes in.

[mattyjams]

The changes here look good to me!

Just noted one thing, and then I think there are a handful of things for lib/mayaUsd still left from my last review of #391. I went through that list and resolved everything that was addressed.

Thanks @HamedSabri-adsk!

[HamedSabri-adsk]

The changes here look good to me!

Just noted one thing, and then I think there are a handful of things for lib/mayaUsd still left from my last review of #391. I went through that list and resolved everything that was addressed.

Thanks @HamedSabri-adsk!

@mattyjams see 1439d2e . shaderFragments.h is indeed a private header and doesn't have to be promoted.

[mattyjams]

@mattyjams see 1439d2e . shaderFragments.h is indeed a private header and doesn't have to be promoted.

Sorry @HamedSabri-adsk! You actually had this right already. Since we have a cpp file in mayaUsd/nodes including a header from mayaUsd/render, that header needs to be public, so it should be getting both promoted and installed. I think we'd want to not only revert 1439d2e, but also make sure that we install ${headers} in the vp2ShaderFragments/CMakeLists.txt.
#391 (comment)

[HamedSabri-adsk]

Sorry @HamedSabri-adsk! You actually had this right already. Since we have a cpp file in mayaUsd/nodes including a header from mayaUsd/render, that header needs to be public, so it should be getting both promoted and installed. I think we'd want to not only revert 1439d2e, but also make sure that we install ${headers} in the vp2ShaderFragments/CMakeLists.txt.
#391 (comment)

Hey @mattyjams , no worries but I still agree with your original comment that shaderFragments.h is a private header. Here is why:

vp2ShaderFragments is not really a target and rather just a sub-directory in mayaUsd echo system.

shaderFragments.h is only used inside mayaUsd target ( e.g mayaUsd\nodes\proxyShapePlugin.cpp ) and doesn't really have to be promoted in order to be used. mayaUsd/nodes is not also a target and rather another sub-directory in mayaUsd echo system.

[mattyjams]

Sorry @HamedSabri-adsk! You actually had this right already. Since we have a cpp file in mayaUsd/nodes including a header from mayaUsd/render, that header needs to be public, so it should be getting both promoted and installed. I think we'd want to not only revert 1439d2e, but also make sure that we install ${headers} in the vp2ShaderFragments/CMakeLists.txt.
#391 (comment)

Hey @mattyjams , no worries but I still agree with your original comment that shaderFragments.h is a private header. Here is why:

vp2ShaderFragments is not really a target and rather just a sub-directory in mayaUsd echo system.

shaderFragments.h is only used inside mayaUsd target ( e.g mayaUsd\nodes\proxyShapePlugin.cpp ) and doesn't really have to be promoted in order to be used. mayaUsd/nodes is not also a target and rather another sub-directory in mayaUsd echo system.

Heh, in that case, I retract my original comment (#390 (comment)). :)

In #390, I had thought that we were mistakenly promoting shaderFragments.h because all I saw there were the changes to the CMakeLists.txt. Then when I looked at #391, I realized that it was indeed a public header and then understood why the CMakeLists.txt changes in #390 were there (#391 (comment)).

Having a cpp file in mayaUsd/nodes reach across directories to include a header from mayaUsd/render is not kosher to me unless that header is public (promoted and installed, and included with angle braces and the full path). Whether the build system treats them as distinct targets or not, from a code perspective they are distinct components.

As it stands right now, we're violating our coding guidelines in how we include the header:
#include "render/vp2ShaderFragments/shaderFragments.h"

And we also have to add ${CMAKE_CURRENT_SOURCE_DIR} as an include directory to make that work, which we've decided we don't want to do.

[HamedSabri-adsk]

@mattyjams I am happy to revert the change for the sake of unblocking us and we can discuss this in future PRs.

I hope this helps.

[HamedSabri-adsk]

@mattyjams see 5d3e007.

I believe this concludes all I wanted to cover in this PR and hopefully didn't muddy the water.