Add configurable grid_cell_count and range filtering parameters#6
Add configurable grid_cell_count and range filtering parameters#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configurable parameters to the cube_bathymetry package to make it more suitable for field operations. The changes introduce a configurable grid cell count parameter for the bathymetry node and range filtering parameters for the detections-to-pointcloud conversion.
Changes:
- Made grid cell count configurable (changed from hardcoded 5 to parameter with default 25)
- Added minimum and maximum range filtering for sonar soundings (defaults: 0.0m - 12000.0m)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cube_bathymetry/src/cube_bathymetry_node.cpp | Adds grid_cell_count as a ROS parameter with default value of 25 (previously hardcoded to 5) |
| cube_bathymetry/src/detections_to_pointcloud.cpp | Adds minimum_range and maximum_range parameters and implements range-based filtering of soundings using Euclidean distance calculation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(!has_parameter("minimum_range")) | ||
| { | ||
| declare_parameter("minimum_range", minimum_range_); | ||
| } | ||
| minimum_range_ = get_parameter("minimum_range").as_double(); | ||
|
|
||
| if(!has_parameter("maximum_range")) | ||
| { | ||
| declare_parameter("maximum_range", maximum_range_); | ||
| } |
There was a problem hiding this comment.
The parameter declaration pattern here is inconsistent with the convention used elsewhere in this codebase. In cube_bathymetry_node.cpp (lines 51-57), parameters are declared directly using declare_parameter() with a default value, without has_parameter() checks. For consistency, these parameters should be declared as:
declare_parameter("minimum_range", minimum_range_);
minimum_range_ = get_parameter("minimum_range").as_double();
declare_parameter("maximum_range", maximum_range_);
maximum_range_ = get_parameter("maximum_range").as_double();This matches the pattern used for "cell_size" and "grid_cell_count" in the same PR.
| if(!has_parameter("minimum_range")) | |
| { | |
| declare_parameter("minimum_range", minimum_range_); | |
| } | |
| minimum_range_ = get_parameter("minimum_range").as_double(); | |
| if(!has_parameter("maximum_range")) | |
| { | |
| declare_parameter("maximum_range", maximum_range_); | |
| } | |
| declare_parameter("minimum_range", minimum_range_); | |
| minimum_range_ = get_parameter("minimum_range").as_double(); | |
| declare_parameter("maximum_range", maximum_range_); |
There was a problem hiding this comment.
Declaring an existing parameter raises an error I think, so checking is a good thing
| std::vector<cube::Sounding> filtered_soundings; | ||
| filtered_soundings.reserve(soundings.size()); | ||
| for(const auto& sounding : soundings) | ||
| { | ||
| double range = sqrt( | ||
| sounding.sonar_relative_position.x * sounding.sonar_relative_position.x + | ||
| sounding.sonar_relative_position.y * sounding.sonar_relative_position.y + | ||
| sounding.sonar_relative_position.z * sounding.sonar_relative_position.z | ||
| ); | ||
| if(range >= minimum_range_ && range <= maximum_range_) | ||
| { | ||
| filtered_soundings.push_back(sounding); | ||
| } | ||
| } |
There was a problem hiding this comment.
The range filtering implementation has a performance inefficiency. The code computes the Euclidean distance (sqrt) for every sounding, which is computationally expensive. Since the filtering only needs to compare ranges, you can filter using squared ranges to avoid the sqrt operation:
double min_range_sq = minimum_range_ * minimum_range_;
double max_range_sq = maximum_range_ * maximum_range_;
for(const auto& sounding : soundings)
{
double range_squared =
sounding.sonar_relative_position.x * sounding.sonar_relative_position.x +
sounding.sonar_relative_position.y * sounding.sonar_relative_position.y +
sounding.sonar_relative_position.z * sounding.sonar_relative_position.z;
if(range_squared >= min_range_sq && range_squared <= max_range_sq)
{
filtered_soundings.push_back(sounding);
}
}This optimization is particularly important for bathymetry processing which can involve thousands of soundings per ping.
| std::vector<cube::Sounding> filtered_soundings; | ||
| filtered_soundings.reserve(soundings.size()); | ||
| for(const auto& sounding : soundings) | ||
| { | ||
| double range = sqrt( | ||
| sounding.sonar_relative_position.x * sounding.sonar_relative_position.x + | ||
| sounding.sonar_relative_position.y * sounding.sonar_relative_position.y + | ||
| sounding.sonar_relative_position.z * sounding.sonar_relative_position.z | ||
| ); | ||
| if(range >= minimum_range_ && range <= maximum_range_) | ||
| { | ||
| filtered_soundings.push_back(sounding); | ||
| } | ||
| } | ||
| soundings = std::move(filtered_soundings); |
There was a problem hiding this comment.
Consider adding diagnostic logging to inform users when soundings are being filtered out. This is especially important for field operations where understanding data loss is critical. For example:
size_t original_count = soundings.size();
// ... filtering logic ...
if(filtered_soundings.size() < original_count)
{
RCLCPP_DEBUG_STREAM_THROTTLE(get_logger(), *get_clock(), 10000,
"Filtered " << (original_count - filtered_soundings.size()) <<
" soundings out of " << original_count << " (range: " <<
minimum_range_ << "m - " << maximum_range_ << "m)");
}This follows the pattern used elsewhere in this file for logging important operational information (lines 115, 130, 142).
| declare_parameter("grid_cell_count", 25); | ||
| int grid_cell_count = get_parameter("grid_cell_count").as_int(); | ||
|
|
||
| map_sheet_ = std::make_shared<cube::MapSheet>(cube::CellCounts(grid_cell_count), cube::CellSizes(cell_size)); |
There was a problem hiding this comment.
The PR description mentions field work from "Oct 2025", which is a future date. The current date is February 19, 2026, so October 2025 would have been approximately 4 months ago. This is likely just a typo in the description, but it's worth clarifying the actual date of the field work for documentation purposes.
Summary
Uncommitted local changes from field work (Oct 2025) found during workspace migration from the old
project11/jazzylayout toros2_agent_workspace.Changes in this branch:
cube_bathymetry_node.cpp: Makegrid_cell_counta ROS parameter (was hardcoded to 5, now defaults to 25)detections_to_pointcloud.cpp: Addminimum_range(default 0.0m) andmaximum_range(default 12000.0m) parameters for sounding range filteringThese are new features not present upstream — they make the cube_bathymetry node more configurable for field use.
Investigation needed before merging
ab58d38, but upstreamjazzyhas since had PR fix: header updates [TASK-020] #2 merged (header fixes, TASK-020). A rebase is needed.grid_cell_countdefault of 25 (vs original hardcoded 5) is appropriate.🤖 Generated with Claude Code