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

Make VP2RenderDelegate enabled by default #1065

Merged

Conversation

kxl-adsk
Copy link

We have been enabling VP2RenderDelegate via mod file for a very long time now. It's time to switch the behavior and have this rendering solution be the default one.

The new MAYAUSD_DISABLE_VP2_RENDER_DELEGATE env variable can be used to switch back to the legacy drawing.

@kxl-adsk kxl-adsk added the vp2renderdelegate Related to VP2RenderDelegate label Jan 12, 2021
Copy link
Contributor

@mattyjams mattyjams left a comment

Choose a reason for hiding this comment

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

This works for me, but I have a slight preference for the env setting to be called MAYAUSD_ENABLE_VP2_RENDER_DELEGATE and be enabled by default instead. I can let that go if you feel strongly otherwise though.

Comment on lines -51 to +53
VP2_RENDER_DELEGATE_PROXY,
MAYAUSD_DISABLE_VP2_RENDER_DELEGATE,
false,
"Switch proxy shape rendering to VP2 render delegate.");
"Disable VP2RenderDelegate from handling rendering of the proxy shape. Switch to legacy mode");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm all for "namespacing" the env setting by prepending the MAYAUSD_ prefix, so no issue with the name of this variable being changed.

I do have a mild aversion though to options that are of the "enable this setting to disable this feature/behavior" variety. I feel like they often involve some additional mental logic gymnastics to deduce the meaning when they're used as below in the form of "if not disabled then use the new thing". Is that preferable to an inverse setting like this?:

TF_DEFINE_ENV_SETTING(
    MAYAUSD_ENABLE_VP2_RENDER_DELEGATE,
    true,
    "Use the VP2 render delegate for proxy shape rendering. Disable to switch to legacy mode.");

Contentious though it may be, this follows the example of MAYA_ENABLE_LEGACY_VIEWPORT, which is admittedly disabled by default, but it's a little more straightforward to reason about. Turn the option off to disable the feature, and turn it on (or leave it alone) to enable it. To my eye, MAYAUSD_ENABLE_VP2_RENDER_DELEGATE=0 a bit more clearly suggests that you want the VP2 render delegate disabled.

@kxl-adsk
Copy link
Author

I’m not a big fan of long lasting env variables. They are good for a short period of time but eventually get forgotten.

The main thing we want to communicate with VP2RenderDelegate env variable is that you are going to be off the default rendering path if you opt-in. Additionally, since this path puts you on legacy drawing, you should not expect we will keep it forever. I don’t think the proposed MAYAUSD_ENABLE_VP2_RENDER_DELEGATE does the job to communicate it.

Maya’s MAYA_ENABLE_LEGACY_VIEWPORT fits perfectly above the description. It puts you off the default and it can disappear whenever legacy support will be stopped. We could use a similar name here, but I fear it’s not descriptive enough. This is why we chose to include VP2RenderDelegate in the name.

I do see your point, but I hope my answer is satisfactory.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jan 13, 2021
@kxl-adsk kxl-adsk merged commit 4b521ec into dev Jan 13, 2021
@kxl-adsk kxl-adsk deleted the kxl-adsk/MAYA-108727/enable_vp2renderdelegate_by_default branch January 13, 2021 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants