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

EMSUSD-121 - Use display color when texture mode is disabled #3272

Merged

Conversation

samuelliu-adsk
Copy link
Collaborator

@samuelliu-adsk samuelliu-adsk commented Aug 11, 2023

  • Added optionVar "mayaUsd_ShowDisplayColorTextureOff" to toggle display mode
  • The optionVar will be add to user Pref once the feature is ready.
  • To view the change in real time, run ogs -reset. This command will be added to the user pref call as well, so the user can see the difference when toggle the optionVar

@samuelliu-adsk samuelliu-adsk added the vp2renderdelegate Related to VP2RenderDelegate label Aug 11, 2023
@samuelliu-adsk samuelliu-adsk marked this pull request as ready for review August 11, 2023 19:55
@samuelliu-adsk samuelliu-adsk added the adsk Related to Autodesk plugin label Aug 11, 2023
if (!_GetMaterialPrimvars(renderIndex, materialId, requiredPrimvars)) {
// Only load textures when texture mode is selected
if (!_GetMaterialPrimvars(renderIndex, materialId, requiredPrimvars)
|| !(reprToken == HdReprTokens->smoothHull)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks wrong. If your intention here is to choose sFallbackShaderPrimvars for untextured mode then you need to have something like: || (reprToken == HdReprTokens->smoothHullUntextured)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, in case where hasColor is true below, we don't use fallback shader. So I guess this logic should be consistent

|| parameters.indexOf("diffuseColor") != -1
|| parameters.indexOf("color") != -1;
// if not, use the fallback shader, which will use the display color
if (!hasColor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are we supposed to do if hasColor is true? Now we do nothing, but probably we should proceed into the normal material case.

@@ -21,6 +21,7 @@
#include "meshViewportCompute.h"
#include "primvarInfo.h"

#include <mayaUsd/base/tokens.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this include can be in cpp file

Copy link
Collaborator

@vlasovi vlasovi left a comment

Choose a reason for hiding this comment

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

Looks very good now. One tiny thing left: please move the "tokens.h" include into cpp. In general, the less includes we have in header files, the faster the compilation.

@samuelliu-adsk samuelliu-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Aug 22, 2023
@seando-adsk seando-adsk removed the adsk Related to Autodesk plugin label Aug 22, 2023
@seando-adsk seando-adsk merged commit be9ad3f into dev Aug 22, 2023
@seando-adsk seando-adsk deleted the samuelliu-adsk/EMSUSD-121/display_color_for_untextured branch August 22, 2023 20:08
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.

5 participants