Added materials to SDF to USD converter by ahcorde · Pull Request #770 · gazebosim/sdformat

ahcorde · 2021-12-02T23:32:48Z

Signed-off-by: ahcorde ahcorde@gmail.com

🎉 New feature

Summary

Added materials to SDF to USD converter. For now I tried this model https://app.ignitionrobotics.org/chapulina/fuel/models/Extinguisher%20PBR

Test it

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: ahcorde <ahcorde@gmail.com>

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin

When I test this PR on this world, the materials appear to be properly created, bu I don't see any colors on the simple shapes when I run usdview (see image below). Is there something else I need to do to view the materials?

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde · 2021-12-03T12:17:48Z

I was not able to visualize textures with usdview I tried with IssacSim.

By the way, duck.dae contains the material inside the collada file and this will not generate a material (I believe). Can you try with this other model Extinguisher PBR ?

Signed-off-by: ahcorde <ahcorde@gmail.com>

adlarkin · 2021-12-03T20:35:32Z

By the way, duck.dae contains the material inside the collada file and this will not generate a material (I believe)

Is this something that we will need to address later on?

I tried with IssacSim ... Can you try with this other model Extinguisher PBR ?

I tried the extinguisher model in isaac sim, and it looks like I can see the materials for simple shapes, but not the mesh:

Here's how things look in ign-gazebo (I moved the extinguisher to the other side of the simple shapes so that it's easier to see):

…e/sdf_to_usd/materials

Signed-off-by: ahcorde <ahcorde@gmail.com>

… PRs Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin · 2021-12-18T02:09:57Z

@ahcorde since we are still iterating on materials, I have broken out a few things from this PR in 794309d into separate PRs so that we can get them merged now. Would you mind taking a look at the following PRs?

…e/sdf_to_usd/materials

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

…itionrobotics/sdformat into ahcorde/sdf_to_usd/materials

Signed-off-by: ahcorde <ahcorde@gmail.com>

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin

I tried to clean a few things up in f0f0885. @ahcorde, would you mind pulling this commit and testing it to make sure I didn't break anything?

I also have a few more comments/questions for you below

adlarkin · 2021-12-22T21:48:39Z

+    return ignition::common::joinPaths(
+      "materials",
+      "textures",
+      ignition::common::basename(_uri));


It looks like the resulting path from this call is being used as a path in the USD file. USD paths use / as the delimiter, so instead of using ignition::common::joinPaths here, I think we should do string concatenation with a / delimiter. The reason why I say this is because if a user called this method on windows, they would have a delimiter of \\ instead of /, which wouldn't be recognized by USD.

sdf (not sdformat) has a path system as well, maybe we should use SdfPath's concatenation?

adlarkin · 2021-12-22T22:59:03Z

    for (unsigned int i = 0; i < ignMesh->SubMeshCount(); ++i)
    {
      auto subMesh = ignMesh->SubMeshByIndex(i).lock();
-      if (!subMesh)
+
+      if (ignMesh->SubMeshCount() != 1)
      {
-        std::cerr << "Unable to get a shared pointer to submesh at index ["
-                  << i << "] of parent mesh [" << ignMesh->Name() << "]\n";
-        return false;
+        std::string pathLowerCase = ignition::common::lowercase(_path);
+        std::string subMeshLowerCase = ignition::common::lowercase(subMesh->Name());
+
+        if (pathLowerCase.find(subMeshLowerCase) != std::string::npos)
+        {
+          isNameInPath = true;
+        }
      }
+    }


What is the purpose of the isNameInPath check? I'm having a hard time understanding it.

Also, it looks like the isNameInPath check is occurring for every submesh. So, the result of isNameInPath after the loop says whether the last submesh's name is in the path or not, but says nothing about the previous submeshes. Is this the intended behavior?

koonpeng · 2021-12-30T02:16:58Z

+    std::string type = ignition::common::lowercase(tokens[3]);
+    std::string modelName = ignition::common::lowercase(tokens[4]);
+    path = ignition::common::joinPaths(
+      home, ".ignition", "fuel", server, owner, type, modelName);


Is there a more standard way to get the fuel path that does not require us to hardcode a fixed path?

humm, we can add a enviroment variable, how does it sound ?

koonpeng · 2021-12-30T02:19:46Z

      {
-        std::cerr << "Internal error: unable to mark link at path ["
-                  << _path << "] as a rigid body\n";
+        std::cerr << "Internal error: unable to mark link at path [" << _path


Should this, and other std:cerr use ignerr instead?

koonpeng · 2021-12-30T02:31:34Z

+        if (ignMesh->SubMeshCount() != 1 && isNameInPath)
+        {
+          std::string pathLowerCase = ignition::common::lowercase(_path);
+          std::string subMeshLowerCase = ignition::common::lowercase(subMesh->Name());
+
+          if (pathLowerCase.find(subMeshLowerCase) == std::string::npos)
+          {
+            continue;
+          }
+        }


This block is already inside a if (isNameInPath), so the 2nd condition here seems redundant. I'm also having a hard time understanding the purpose of this in combination with the isNameInPath check above.

koonpeng · 2021-12-30T02:45:15Z

+    }
+    else
+    {
+      auto usdXformMesh = pxr::UsdGeomXform::Define(_stage, pxr::SdfPath(_path));


This block here duplicates many of the code in the if block, I think it should be possible to refactor them to keep things DRY.

koonpeng · 2021-12-30T02:49:01Z

+    return ignition::common::joinPaths(
+      "materials",
+      "textures",
+      ignition::common::basename(_uri));


sdf (not sdformat) has a path system as well, maybe we should use SdfPath's concatenation?

koonpeng · 2021-12-30T03:00:07Z

+      if (!_displayName.GetString().empty())
+      {
+        attr.SetDisplayName(_displayName);
+      }
+      if (!_displayGroup.GetString().empty())
+      {
+        attr.SetDisplayGroup(_displayGroup);
+      }
+      if (!_doc.empty())
+      {
+        attr.SetDocumentation(_doc);
+      }
+      if (!_colorSpace.GetString().empty())


These seems to take in a string, why does the function params require these to be TfToken? Also the return value is not checked for errors.

koonpeng · 2021-12-30T03:15:19Z

+    // with the names of the materials
+    static int i = 0;
+
+    const std::string mtl_path = "/Looks/Material_" + std::to_string(i);


Is it possible to build the path based on the model/link/visual/material hierarchy?

…e/sdf_to_usd/materials

Signed-off-by: ahcorde <ahcorde@gmail.com>

…e/sdf_to_usd/materials

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added materials to SDF to USD converter

791d2c3

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from adlarkin December 2, 2021 23:32

ahcorde self-assigned this Dec 2, 2021

ahcorde requested review from azeey and scpeters as code owners December 2, 2021 23:32

adlarkin added 3 commits December 2, 2021 17:22

add collisions and static/non-static models (rigid bodies)

071cef6

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Merge branch 'adlarkin/sdf_to_usd' into ahcorde/sdf_to_usd/materials

afd08ec

cleanup

5e3f5fe

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin force-pushed the ahcorde/sdf_to_usd/materials branch from d3813d3 to 5e3f5fe Compare December 3, 2021 02:02

adlarkin reviewed Dec 3, 2021

View reviewed changes

feedback

fdd049c

Signed-off-by: ahcorde <ahcorde@gmail.com>

Remove traces

f090790

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested a review from adlarkin December 3, 2021 14:41

adlarkin force-pushed the adlarkin/sdf_to_usd branch from 071cef6 to c291638 Compare December 3, 2021 22:31

ahcorde and others added 5 commits December 8, 2021 22:56

Merge remote-tracking branch 'origin/adlarkin/sdf_to_usd' into ahcord…

8cabc3c

…e/sdf_to_usd/materials

Merge remote-tracking branch 'origin/adlarkin/sdf_to_usd' into ahcord…

c509792

…e/sdf_to_usd/materials

redo commit

d9b3675

Signed-off-by: ahcorde <ahcorde@gmail.com>

Added more fixes

25f80dc

Signed-off-by: ahcorde <ahcorde@gmail.com>

Merge branch 'adlarkin/sdf_to_usd' into ahcorde/sdf_to_usd/materials

4c3ca29

This was referenced Dec 18, 2021

Apply UsdPhysicsDriveAPI to revolute joints #791

Merged

apply COM and inertia to links #792

Merged

move joint physicsDrive and link mass/inertial properties to separate…

794309d

… PRs Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin force-pushed the ahcorde/sdf_to_usd/materials branch from c016c78 to 794309d Compare December 18, 2021 02:09

ahcorde and others added 4 commits December 20, 2021 12:59

Merge remote-tracking branch 'origin/adlarkin/sdf_to_usd' into ahcord…

22d4953

…e/sdf_to_usd/materials

fix build

0fa76e1

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Merge branch 'ahcorde/sdf_to_usd/materials' of https://github.com/ign…

704da5d

…itionrobotics/sdformat into ahcorde/sdf_to_usd/materials

reenable collisions

57b80eb

Signed-off-by: ahcorde <ahcorde@gmail.com>

cleanup

f0f0885

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

adlarkin reviewed Dec 22, 2021

View reviewed changes

chapulina added the 🌱 garden Ignition Garden label Dec 30, 2021

koonpeng reviewed Jan 3, 2022

View reviewed changes

ahcorde added 3 commits January 3, 2022 11:50

Merge remote-tracking branch 'origin/adlarkin/sdf_to_usd' into ahcord…

325e60a

…e/sdf_to_usd/materials

Added some fixes

995747e

Signed-off-by: ahcorde <ahcorde@gmail.com>

added feedback

1c19bcb

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde requested review from adlarkin and koonpeng January 4, 2022 18:26

koonpeng reviewed Jan 6, 2022

View reviewed changes

Comment thread src/usd/sdf_usd_parser/geometry.cc

Comment thread src/usd/sdf_usd_parser/sensor.cc

Comment thread src/usd/sdf_usd_parser/light.cc

Comment thread src/usd/sdf_usd_parser/utils.hh

ahcorde added 2 commits January 6, 2022 16:02

Merge remote-tracking branch 'origin/adlarkin/sdf_to_usd' into ahcord…

38132a9

…e/sdf_to_usd/materials

Added feedback

aa4233a

Signed-off-by: ahcorde <ahcorde@gmail.com>

ahcorde merged commit 4e0b9c9 into adlarkin/sdf_to_usd Jan 6, 2022

ahcorde deleted the ahcorde/sdf_to_usd/materials branch January 6, 2022 15:31

Conversation

ahcorde commented Dec 2, 2021

🎉 New feature

Summary

Test it

Checklist

Uh oh!

adlarkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahcorde commented Dec 3, 2021

Uh oh!

adlarkin commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adlarkin commented Dec 18, 2021

Uh oh!

adlarkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adlarkin left a comment •

edited

Loading

adlarkin commented Dec 3, 2021 •

edited

Loading