-
Notifications
You must be signed in to change notification settings - Fork 657
Fix parameter passing in generate_state_database.cpp #3545
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
base: main
Are you sure you want to change the base?
Fix parameter passing in generate_state_database.cpp #3545
Conversation
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.
Actually, I don't understand why this function was introduced at all.
Node::get_parameter_or()
is a templated function that should work for uint
as well.
Could you please try to use that function directly?
In this snippet, the author implemented a custom getUintParameterOr utility function instead of calling node->get_parameter_or directly. This approach offers two main advantages: size_t support Seamless conversion Since both samples and edges_per_sample were originally defined as unsigned int, this helper function ensures the values are read and stored correctly in their intended unsigned integer types. |
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.
The native rclcpp::Node::get_parameter_or template supports built-in types (e.g., int, double, bool).
I see. Unsupported parameter types yield an "undefined symbol" error, I guess?
static bool getUintParameterOr(const rclcpp::Node::SharedPtr& node, const std::string& param_name, | ||
size_t&& result_value, const size_t default_value) | ||
static bool get_uint_parameter_or(const rclcpp::Node::SharedPtr& node, const std::string& param_name, | ||
unsigned int& result_value, const size_t default_value) |
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.
You should consistently use size_t
or unsigned int
:
unsigned int& result_value, const size_t default_value) | |
unsigned int& result_value, const unsigned int default_value) |
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.
Thanks for the suggestion! You’re right—I’ll unify all related types in the function signature to unsigned int, including the default value type. I’ll verify everything locally tomorrow and then immediately update the branch and push a new PR. Thank you again for your guidance, and I look forward to any further feedback!
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.
Thanks for the suggestion! I’ve unified the types to unsigned int (including the default value) and pushed the update. Please take another look.
moveit_planners/ompl/ompl_interface/scripts/generate_state_database.cpp
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3545 +/- ##
=======================================
Coverage 46.22% 46.22%
=======================================
Files 720 720
Lines 62713 62713
Branches 7594 7594
=======================================
Hits 28985 28985
+ Misses 33562 33561 -1
- Partials 166 167 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Background
MoveIt 2 provides the “Planning with Approximated Constraint Manifolds” feature to accelerate constrained motion planning. The generate_state_database.cpp file is responsible for building the constraint state database, but due to the parameter-retrieval function’s signature using an rvalue reference, both options.samples and options.edges_per_sample remain at their default value of 0. As a result, the sampling and connection loops never execute.
Problem
In the original implementation, the parameter-retrieval function is declared as:
static bool get_uint_parameter_or(
const rclcpp::Node::SharedPtr& node,
const std::string& param_name,
size_t&& result_value,
const size_t default_value)
{ … }
Passing result_value as a size_t&& rvalue reference prevents the retrieved parameter value from being written back to the caller’s variable.
Consequently, both options.samples and options.edges_per_sample stay at 0, causing the state database generation to fail with:
[INFO] […] Generated 0 states in 0.000005 seconds
[ERROR] […] No StateStoragePtr found – implement better solution here.
Solution
Change the function signature to use an lvalue reference of type unsigned int&, ensuring that the retrieved parameter is correctly assigned:
-static bool get_uint_parameter_or(
+static bool get_uint_parameter_or(
{
int param_value;
if (node->get_parameter(param_name, param_value))
{
if (param_value >= 0)
{
result_value = default_value;
return true;
}
Key Changes
Changed parameter type from size_t&& to unsigned int&.
Cast the retrieved int to unsigned int instead of size_t.
Preserved the original default-value fallback and warning logic.
Verification
After rebuilding and running the generation script locally, you can now observe proper sampling and connection progress:
[INFO] […] 90% complete
[INFO] […] Computed possible connexions in 14.77 seconds. Added 2373 connexions
[INFO] […] Spent 202.56 seconds constructing the database
The Constraint Approximation Database is successfully created and saved to the specified path.
Related Work
There is an existing community PR (see Fixes for using generate-state_database #1412) addressing issues with generate_state_database, but that effort focuses on different aspects. This fix targets only the parameter-retrieval signature, so the two can be merged independently.