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

MAYA-133671 - Add textured mode handling to topo grapher #4031

Merged

Conversation

JGamache-autodesk
Copy link
Collaborator

Allows quickly reducing a graph to only a surface for when the viewport is in "untextured" mode.

Allows quickly reducing a graph to only a surface for when the viewport
is in "untextured" mode.
@JGamache-autodesk JGamache-autodesk self-assigned this Dec 5, 2024
frohnej-adsk
frohnej-adsk previously approved these changes Dec 5, 2024
Copy link
Collaborator

@frohnej-adsk frohnej-adsk left a comment

Choose a reason for hiding this comment

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

LGTM! Just one minor comment.

lib/mayaUsd/render/MaterialXGenOgsXml/ShaderGenUtil.cpp Outdated Show resolved Hide resolved
frohnej-adsk
frohnej-adsk previously approved these changes Dec 5, 2024
feldstj
feldstj previously approved these changes Dec 5, 2024
Copy link
Collaborator

@feldstj feldstj left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion you can take or leave.

computeGraph(material, textured);
}

void TopoNeutralGraph::computeGraph(const mx::ElementPtr& material, bool textured)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one comment. Alice had recommended that I move away from having boolean parameters like this in favour of multiple methods. So in this case something like computeGraph and computeTexturedGraph. Just a suggestion though, up to you. I do think its a bit nicer that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I guess the downside of that is you'd need an if statement on the textured parameter to call either computeGraph or computeTexturedGraph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And then the code of both functions end up identical except for three lines of code. I know of other C++ gurus that have a strict policy of using fully specified enums in this case. Especially useful when you start having calls like:

doSomething(true, false, true, true, false, false, false);

Sadly not something I can rework in the mayaUsdApi library as it needs to stay binary compatible. I can add constructors, but that is about it.

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Dec 6, 2024
@JGamache-autodesk
Copy link
Collaborator Author

@seando-adsk ready for merge. Remaining preflight error is a known random failure of the testUsdExportPackage unit test.

@seando-adsk seando-adsk added the workflows Related to in-context workflows label Dec 6, 2024
@seando-adsk seando-adsk merged commit 4e01a71 into dev Dec 6, 2024
12 of 13 checks passed
@seando-adsk seando-adsk deleted the gamaj/MAYA-133671/allow_topo_handler_to_handle_untextured branch December 6, 2024 17:25
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 workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants