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

Opt in to diffuse color multidraw consolidation #468

Merged

Conversation

williamkrick
Copy link
Contributor

This change enables multidraw consolidation for render items using the fallback shader in MayaUSD.

The Maya API this change relies on is a prototype API being introduced in PR115. It is appropriate for testing only.

@williamkrick williamkrick added do-not-merge-yet Development is not finished, PR not ready for merge pending beta This issue or PR will be testable in next beta release vp2renderdelegate Related to VP2RenderDelegate labels Apr 27, 2020
Copy link

@huidong-chen huidong-chen left a comment

Choose a reason for hiding this comment

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

Please protect the prototype API with MAYA_API_VERSION >= 20210000.

@huidong-chen
Copy link

Can we also enable the flag for material-produced shaders?

@williamkrick
Copy link
Contributor Author

Can we also enable the flag for material-produced shaders?

I am not sure. Part of the problem is some materials were getting the flag that didn't support it. We want to prevent that from happening. Can we test those materials in MayaUSD to see if they have a diffuseColor parameter?

@huidong-chen
Copy link

Can we also enable the flag for material-produced shaders?

I am not sure. Part of the problem is some materials were getting the flag that didn't support it. We want to prevent that from happening. Can we test those materials in MayaUSD to see if they have a diffuseColor parameter?

Yes, but I think the API call itself can check the existence of diffuseColor. If not, we could have an explicit lookup with MShaderInstance::parameterList().

@williamkrick
Copy link
Contributor Author

Can we also enable the flag for material-produced shaders?

I am not sure. Part of the problem is some materials were getting the flag that didn't support it. We want to prevent that from happening. Can we test those materials in MayaUSD to see if they have a diffuseColor parameter?

Yes, but I think the API call itself can check the existence of diffuseColor. If not, we could have an explicit lookup with MShaderInstance::parameterList().

Yes you are right. Actually the version I'm already working on does a much better job checking that setting the flag is valid and rejecting invalid calls so we should be able to attempt to mark everything as wantMultiDrawConsolidation(). Do you have a suggestion where I can flag those materials off the top of your head? If not I'll dig though and find a place.

@huidong-chen
Copy link

My first try would be adding the API call here:

https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/render/vp2RenderDelegate/material.cpp#L701

This is where the shader instance has just been produced from the full material network.

@huidong-chen
Copy link

Based on the offline chat, I don't think we have to add color consolidation for material in the same PR. Appoving the PR.

@@ -273,7 +273,8 @@ namespace
shader = shaderMgr->getFragmentShader(
_fallbackShaderNames[index], _structOutputName, true);
#if MAYA_API_VERSION >= 20210000
shader->setWantDiffuseColorMultiDrawConsolidation(true);
MString diffuseColorParamName("diffuseColor");

Choose a reason for hiding this comment

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

Please use _diffuseColorParameterName which has been defined in the file.

@kxl-adsk kxl-adsk removed do-not-merge-yet Development is not finished, PR not ready for merge pending beta This issue or PR will be testable in next beta release labels May 27, 2020
@kxl-adsk kxl-adsk merged commit f34c6ed into dev May 27, 2020
@kxl-adsk kxl-adsk deleted the krickw/MAYA-104223/opt_in_to_diffuseColor_multidraw_consolidation branch June 3, 2020 21:00
ppt-adsk pushed a commit that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants