Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
sdf -> usd: Materials #831
sdf -> usd: Materials #831
Changes from 29 commits
9dfc9f2
ffaf2f4
2e3a060
0a5a9a7
071dc4b
89daa0d
f48b35b
d932e90
54b35e9
f0331ca
e85f60c
a02a6e1
f898d26
258606c
c43ba24
f816dd4
6f3490e
379defd
0f4075f
a98d481
0cb1cc0
00a55c5
5ac37f7
2085347
94969a0
361c569
c7114f8
7130c1f
14354fb
430579f
e40448f
a603603
67c54df
830c11a
5f33b20
cdc0257
8b97a1c
3095a50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why return a
shared_ptr
here? Is it important that the resultingMaterial
be allocated on the heap? we return ansdf::Material
object from the other functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aunique_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ashared_ptr
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SubMesh::MaterialByIndex
returns ashared_ptr
which mean we need to convert thisshared_ptr
tounique_ptr
to finally get thesdf::Material
. I will bet for theshared_ptr
to simplify things.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.