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

Lanierd/hydra 1054 #169

Merged
merged 9 commits into from
Sep 16, 2024
Merged

Lanierd/hydra 1054 #169

merged 9 commits into from
Sep 16, 2024

Conversation

lanierd-adsk
Copy link
Collaborator

  • Support material assignment on a usd prim
  • Support default material for usd procedural prims (sphere, capsule, cylinder etc..)

…cone and cilynder_1 are not supported on OSX for USD 24.05
@lanierd-adsk lanierd-adsk self-assigned this Sep 13, 2024
@lanierd-adsk
Copy link
Collaborator Author

FYI : @ppt-adsk @lilike-adsk

Copy link
Collaborator

@debloip-adsk debloip-adsk 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! Just one outdated comment

@@ -68,7 +88,7 @@ HdSceneIndexPrim DefaultMaterialSceneIndex::GetPrim(const SdfPath& primPath) con
bool DefaultMaterialSceneIndex::_ShouldWeApplyTheDefaultMaterial(const HdSceneIndexPrim& prim)const
{
// Only for meshes so far
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment needs to be adjusted

debloip-adsk
debloip-adsk previously approved these changes Sep 13, 2024
Copy link
Collaborator

@ppt-adsk ppt-adsk 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, thanks! Only minor comments.

// Support implicit surfaces from USD as well, not only meshes
bool _IsDefaultMaterialCompliantPrimitive(const TfToken& primType)
{
static VtArray<TfToken> const compliantPrimitives = { HdPrimTypeTokens->cone,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: would be slightly better as an std::set, with the check using either std::set::find() or std::set::count().

@@ -78,6 +78,11 @@ SdfPath DagPathToSdfPath(
const bool stripNamespaces)
{
std::string name = dagPath.fullPathName().asChar();
if ( name.empty() ) {
MFnDependencyNode dep(dagPath.node());
TF_WARN("Empty fullpath name for DAG object : %s of type : %s", dep.name().asChar(), dep.typeName().asChar());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know when this happens? Is this for non-Dag nodes? Does this indicate missing functionality, missing logic, or a bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had this with Bifrost nodes and also when selecting the default material set. But the problem is that wer get dag nodes with no type and no name, we should try detect useful dag nodes that need to be translated as some may not need to be treated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Github is erroring out saying that "You need to add a comment indicating the requested change.", even though I've done exactly that. Hoping that this additional comment will help.

@ppt-adsk
Copy link
Collaborator

Here's a useful reference:
https://stackoverflow.com/questions/17256001/merits-of-stdfind

@lanierd-adsk lanierd-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 16, 2024
@debloip-adsk debloip-adsk merged commit 6756a66 into dev Sep 16, 2024
10 of 11 checks passed
@debloip-adsk debloip-adsk deleted the lanierd/HYDRA-1054 branch September 16, 2024 15:07
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants