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

[hdMaya] add Maya-to-Hydra plugin and scene delegate #231

Merged
merged 22 commits into from
Feb 6, 2020

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Jan 31, 2020

This migrates the Maya-to-Hydra work from refactoring_sandbox to here.

This corresponds to the latest on usd_0_20_02_refactoring_sandbox:

67378d7

(If that commit is merged into refactoring_sandbox, the dev commit this comes off of (ec6750b) is first merged into refactoring_sandbox, then merging this into refactoring_sandbox can be done as a do-nothing merge.)

Copy link

@kxl-adsk kxl-adsk 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...left few comments, but didn't bother repeating them at every occurrence of the same pattern (this will make it easier to discuss).

Key things:

  • we don't have support for selection without UFE - is this ok? (see comment in lib/usd/hdMaya/delegates/proxyDelegate.cpp)
  • we still have quite a lot code for 1901 and 1905

@pmolodo pmolodo force-pushed the pr/hdmaya/maya_to_hydra_dev branch from e459338 to 73730b7 Compare February 4, 2020 00:26
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 4, 2020

Latest push was just to rebase against latest dev (to make it easier for me to build). No functional changes.

Starting to review / address all the notes - thank you for all the feedback!

@kxl-adsk
Copy link

kxl-adsk commented Feb 4, 2020

@elrond79 Tested this branch on Windows and MacOS (compiled in strict mode). Got following errors on MacOS:

/git/usd/maya-usd/lib/usd/hdMaya/adapters/materialAdapter.cpp:85:6: error: unused variable '_PreviewShader' [-Werror,-Wunused-variable]
auto _PreviewShader = []() -> const _ShaderSourceAndMeta& {
     ^

/git/usd/maya-usd/lib/usd/hdMaya/adapters/proxyAdapter.cpp:53:16: error: unused variable 'USD_UFE_RUNTIME_NAME' [-Werror,-Wunused-const-variable]
constexpr auto USD_UFE_RUNTIME_NAME = "USD";


/git/usd/maya-usd/lib/render/mayaToHydra/shadingModeExporter.cpp:184:35: error: using the result of an assignment as a condition without parentheses [-Werror,-Wparentheses]
                if (surfaceOutput = material.GetSurfaceOutput()) {
                    ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/git/usd/maya-usd/lib/render/mayaToHydra/shadingModeExporter.cpp:184:35: note: place parentheses around the assignment to silence this warning
                if (surfaceOutput = material.GetSurfaceOutput()) {
                                  ^
                    (                                          )
/git/usd/maya-usd/lib/render/mayaToHydra/shadingModeExporter.cpp:184:35: note: use '==' to turn this assignment into an equality comparison
                if (surfaceOutput = material.GetSurfaceOutput()) {
                                  ^
                                  ==

Attaching patch which allowed me to build successfully.
strict_mode_mtoh.txt

Paul Molodowitch added 11 commits February 4, 2020 12:24
- reword the "What's it good for" section of the docs
- add nurbsCurve and phong to list of supported nodes
...this is related to another set of selection changes, for which
we have not made a PR yet
was arbitrarily chosen, presumably in an attempt to prevent users from
crashing if they chose too high a value

However, any value that's high enough that no user would conceiveably ever
need more is also high enough that it will crash some users systems.
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 5, 2020

@elrond79 Tested this branch on Windows and MacOS (compiled in strict mode). Got following errors on MacOS:

...

Attaching patch which allowed me to build successfully.
strict_mode_mtoh.txt

Didn't follow your patch exactly (some of the code was needed, depending on USD version), but I think I should have addressed all the compilation warnings / errors:

9a1bd28

@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 5, 2020

So, when I pushed this last batch of commits, I thought I had addressed (or at least discussed) all the comments - until I noticed that github had collapsed some of them! Doh!

Ah well, I'll get the rest of them soon...

@kxl-adsk
Copy link

kxl-adsk commented Feb 5, 2020

I resolved the conversations that were addressed. Maybe this is what you see as "collapsed"?

Paul Molodowitch added 5 commits February 4, 2020 17:26
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 5, 2020

I resolved the conversations that were addressed. Maybe this is what you see as "collapsed"?

Alas, no - Github does this thing where if a code review is long, it says something like:

5 hidden conversations
Load more…

... which you have to click on to see the rest. Wish it didn't do that, personally.

Anyway - thanks for marking those resolved!

Paul Molodowitch added 3 commits February 4, 2020 18:11
was currently disabled - in order to restore non-ufe
selection, we will need to:

- add a new interface to query selection paths from the base proxy shape
  (ie, something that functions as the combined result of AL's proxy->
  selectedPaths and proxy->selectionList.paths())
- implement on all derived proxies
- re-add / implement this removed code (ie, the PopulateSelectedPaths
  override that takes an MSelectionList)
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 5, 2020

Question - something I've wondered before. Is it acceptable to do MPlug.asInt() if the actual attribute is a short? (This is being done with mesh.smoothLevel.)
What about .asFloat()?

@kxl-adsk
Copy link

kxl-adsk commented Feb 5, 2020

The underlying value is stored as something much bigger (64bit IIRC), so I don't see any reason to change it.
Making it a float would be rather surprising since a smooth level doesn't support fractional values.

@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 5, 2020

The asFloat didn't actually pertain to this situation - I was just wondering if it does some sort of auto-conversion, or just blindly interprets the int bits as a float.

Sounds like it blindly interprets...

Anyway, just pushed removal of an outdated USD_VERSION_NUM check which I somehow missed...

@kxl-adsk
Copy link

kxl-adsk commented Feb 5, 2020

We are almost there...just one new compilation error to fix (fails both on windows and macOS):

/git/usd/maya-usd/lib/render/mayaToHydra/plugin.cpp:43:20: error: constexpr variable 'envVarSize' must be initialized by a constant expression
    constexpr auto envVarSize = strlen(envVarSet) + 1;
                   ^            ~~~~~~~~~~~~~~~~~~~~~
/git/usd/maya-usd/lib/render/mayaToHydra/plugin.cpp:43:33: note: non-constexpr function 'strlen' cannot be used in a constant expression
    constexpr auto envVarSize = strlen(envVarSet) + 1;

gcc will accept it, but clang + vc++ won't
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 6, 2020

Doh - I should have known not to trust that constexpr to be cross-platform compatible after the problems with the other one. Just changed it to const

@kxl-adsk
Copy link

kxl-adsk commented Feb 6, 2020

Perfect. I already run tests with this change.

@kxl-adsk kxl-adsk merged commit 30e633a into Autodesk:dev Feb 6, 2020
@pmolodo
Copy link
Contributor Author

pmolodo commented Feb 6, 2020

Awesome, thanks!

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