-
Notifications
You must be signed in to change notification settings - Fork 412
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
--[BE] Fix issues with bindings exposed by pybind11-stubgen #2430
Conversation
raw setters and initializers should not be used for attributes, since they may corrupt a required field.
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.
Nice clean-up! LGTM!
[](Configuration& self, const std::string& key, const T val) { | ||
self.set(key, val); | ||
}, | ||
("Set the value specified by given string key to be specified " + |
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'm not sure if this actually works, as far as I remember the reason def()
accepted a plain const char*
was because it just referenced it without actually copying anywhere. So when retrieving documentation of those, you'll get a crash due to a dangling pointer.
I might be wrong, but I personally hope it's not doing a string copy for every docstring. That'd be nasty.
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.
@mosra The __init__.pyi
has an appropriate doc string listed :
@typing.overload
def set(self, key: str, value: str) -> None:
"""
Set the value specified by given string key to be specified string value
"""
Would the problem be somewhere else?
__init__.pyi
also has duplicate entries for the const char* and const str& versions. I going to get rid of the const char* specializations.
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.
Oh, alright, then it works. I wasn't sure.
5cc0927
to
0423a45
Compare
0423a45
to
a86c933
Compare
Current Main's stubgen errors :
The PR in its current state's errors :
|
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.
Did another pass on the latest changes. LGTM! Thanks for doing this.
.value("LINK_VISUALS", | ||
metadata::attributes::ArticulatedObjectRenderMode::LinkVisuals, | ||
"Render the Articulated Object using urdf-defined " | ||
"meshes/primitives to respresent each link.") |
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.
Nit: respresent
R"(Whether or not the asset described by this attributes supports texture-based semantics)") | ||
.def_property_readonly( | ||
"num_regions", &SemanticAttributes::getNumRegionInstances, | ||
R"(The nmumber of semantic regions defined by this Semantic Attributes.)"); |
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.
Nit: nmumber
* configuration is created on LightLayoutAttributes construction. | ||
*/ | ||
std::shared_ptr<Configuration> lightInstConfig_{}; | ||
|
||
/** | ||
* @brief Deque holding all released IDs to consume for light instances when | ||
* @brief Deque holding all released IDs to consume for @ref LightInstanceAttributess when |
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.
Nit: LightInstanceAttributess
[semanticAttribs](bool has_semantic_textures) { | ||
semanticAttribs->setHasSemanticTextures(has_semantic_textures); | ||
}); | ||
|
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.
👍
// "'s articulated link specified by the passed " | ||
// "link_id. Use link_id==-1 to get the base link.") | ||
// .c_str(), | ||
// "link_id"_a) |
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.
Unrelated: Is this planned?
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.
Yes, @aclegg3 and I have been discussing a mechanism for this.
Motivation and Context
This PR adds missing bindings and fixes broken ones as exposed by pybind11-stubgen which is being introduced in this PR
How Has This Been Tested
All local c++ and python tests pass
Types of changes
Checklist