sdf -> usd: Materials #831
Conversation
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
… ahcorde/sdf_to_usd_materials
Codecov Report
@@ Coverage Diff @@
## sdf12 #831 +/- ##
==========================================
- Coverage 89.34% 88.48% -0.87%
==========================================
Files 90 92 +2
Lines 13744 14190 +446
==========================================
+ Hits 12280 12556 +276
- Misses 1464 1634 +170
Continue to review full report at Codecov.
|
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org>
Signed-off-by: Teo Koon Peng <koonpeng@openrobotics.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
…rapped sdf errors Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
usd/include/sdf/usd/Conversions.hh
Outdated
| /// \param[in] _in SDF material. | ||
| /// \return Ignition Common Material. | ||
| IGNITION_SDFORMAT_USD_VISIBLE | ||
| std::shared_ptr<ignition::common::Material> convert( |
There was a problem hiding this comment.
why return a shared_ptr here? Is it important that the resulting Material be allocated on the heap? we return an sdf::Material object from the other function
There was a problem hiding this comment.
not really sure about how to deal with this, because I can't return a ignition::common::Material because is a non-copyable class (it has a unique_ptr inside). I decided touse the same type in both methods.
@scpeters what do you think is right type ?
There was a problem hiding this comment.
ok, that's reasonable. I'll open an issue in ign-common suggesting that we improve the Material class to make it copyable
There was a problem hiding this comment.
If it has a move ctor, then you should be might be able to return ignition::common::Material and the compiler will use copy elision to construct the object at the call location.
There was a problem hiding this comment.
it doesn't have a move ctor https://github.com/ignitionrobotics/ign-common/blob/ign-common4/graphics/include/ignition/common/Material.hh#L90-L97
@azeey, does it make sense to include it ? or can we keep the shared_ptr ?
There was a problem hiding this comment.
Unfortunately, since it has a user declared destructor, the compiler will not generate a move ctor. Simply removing the destructor would fix this, but I think that will break ABI. So, returning a smart pointer seems to be our best bet, but if we have to do that, my preference would be to use a unique_ptr unless there is a particular reason to use a shared_ptr.
There was a problem hiding this comment.
SubMesh::MaterialByIndex returns a shared_ptr which mean we need to convert this shared_ptr to unique_ptr to finally get the sdf::Material. I will bet for the shared_ptrto simplify things.
There was a problem hiding this comment.
I have an idea for getting around the std::shared_ptr issue - see 3095a50. If you guys think this isn't a good solution, feel free to revert this commit.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
There was a problem hiding this comment.
I made some changes in cdc0257 and 8b97a1c. I also tried to address the std::shared_ptr issue discussed in #831 (comment) with 3095a50, so once someone looks at my workaround, I think this is good to go.
|
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1 |
Signed-off-by: ahcorde ahcorde@gmail.com
🎉 New feature
Summary
Suppor for materials in the sdf to usd converter:
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.