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.

Sign up for GitHub

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

Jump to bottom

[mtoh] Refactor settings / defaultRenderGlobals storage #758

Merged
merged 3 commits into from
Oct 2, 2020
Merged

[mtoh] Refactor settings / defaultRenderGlobals storage #758

merged 3 commits into from
Oct 2, 2020

Conversation

marsupial
Copy link
Contributor

There are a few issues with the current render-settings / defaultRenderGlobals, that make it difficult for a delegate to merge USDRenderSettings and HdDelegate::SetRendererSetting. The larger goal of editing a USDRenderSettings and applying a delegates schema aren't dealt with here, as the PR seems large enough just to address the current issues:

  • Largest issue is that if a delegate provides a USD-namespaced setting, mtoh will error out when trying to set it.
  • If any two delegates have the same setting (because they can't namespace) then they will stomp one another.
  • User state isn't honored when not saved into a scene, this is problematic for jumping between files as the user may have the delegate up in a specific way optimized for the task.
  • Updates to a single setting incur a large cost of synchronizing all settings across all delegates.
  • Excessive copies of MtohRenderGlobals, when one is more than sufficient.
  • Changes from editing values from via Attribute Editor do not make it to the delegate.
  • Need proper layout/grouping in the Attribute Editor as well.

mtoh-ae-before
mtoh-hydra-group
mtoh-groups-expaned

@kxl-adsk kxl-adsk added the mtoh Related to legacy Maya to Hydra plugin. label Sep 1, 2020
@kxl-adsk kxl-adsk requested a review from pmolodo September 1, 2020 13:22
pmolodo
pmolodo suggested changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@pmolodo pmolodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good - I definitely like the overall goal here, so glad to have this PR, and thank you!

Just a few small typos / comments / questions...

@kxl-adsk
Copy link

@marsupial Have a look as well at the build errors with MacOS - https://github.com/Autodesk/maya-usd/pull/758/checks. You can get build logs by downloading the artifact associated with the check.

rzulak added 2 commits September 17, 2020 14:46
[mtoh] Refactor settings / defaultRenderGlobals storage:
4e13a51 
         ADD: Accept USD namespaced properties.
         ADD: -renderer flag to mtoh command/
         ADD: Save and restore user state via optionVars.
         FIX: Delegates with settings named the same pushing their changes to one-another.
         FIX: Change of one setting incurring a full reload / set of all settings.
         FIX: Have changes set via AttributeEditor get pushed to delegates.
         FIX: Multiple copies of MtohRenderGlobals exiting in disparate places.
         FIX: Ugly/non-existant layout for Hydra setting in the AttributeEditor.
         FIX: Mtoh possibly inserting extra settings with the same name as a delegates.
Address comments
e280a2c
@marsupial marsupial force-pushed the PR/mtoh-render-settings branch from 9751d8e to e280a2c Compare September 17, 2020 23:33
@marsupial
Copy link
Contributor Author

marsupial commented Sep 17, 2020 •
edited
Loading

Resolved all the 'simple' stuff as I went through them, but let the longer comments open

@kxl-adsk
Copy link

@marsupial I'm trying to get a sense of where we are with this PR? Your last comment mentions that there are still unresolved comments. As well, did you fix mac build errors mentioned here - #758 (comment)

@marsupial
Copy link
Contributor Author

They've all been addressed as far as I'm aware.

@kxl-adsk
Copy link

@pmolodo Anything is still missing before you can approve this PR?

pmolodo
pmolodo suggested changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@pmolodo pmolodo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically there - all the more significant changes were made, just a typo, missed rename, and a few more instances of braceless-ifs left.

pmolodo
pmolodo approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@pmolodo pmolodo 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!

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 2, 2020
@kxl-adsk kxl-adsk merged commit 36e3113 into Autodesk:dev Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattyjams mattyjams mattyjams left review comments

@kxl-adsk kxl-adsk kxl-adsk left review comments

@pmolodo pmolodo pmolodo approved these changes

Assignees

@ecp-maya-devops-adsk ecp-maya-devops-adsk

Labels
mtoh Related to legacy Maya to Hydra plugin. ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marsupial @kxl-adsk @pmolodo @mattyjams @ecp-maya-devops-adsk