584 feature visualization of lidar object clusters #585
Conversation
…N parameters for lidar and radar clustering. Solve a bug where old markers did not vanish.
WalkthroughThe pull request introduces enhancements to LiDAR and radar perception modules, focusing on improving data clustering and visualization capabilities. The changes primarily involve modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/perception/src/lidar_distance.py (2)
118-119: Height filtering threshold might be too aggressive.
Currently filtering points below -1.4 on the Z-axis. Verify whether this excludes potentially relevant lower-lying objects, such as curbs or smaller obstacles. Consider making this threshold configurable or adding a margin.
412-412: Marker lifetime is very short.
A 0.1s lifetime for markers might cause flickering in RViz if publish rates drop. If you prefer longer visibility during slower updates, consider a higher value or no expiration (by setting lifetime to 0).- marker.lifetime = rospy.Duration(0.1) + # For a higher or indefinite marker visibility: + marker.lifetime = rospy.Duration(1.0)code/perception/src/radar_node.py (1)
350-352: LGTM: Fixed marker persistence issueThe addition of marker namespace and lifetime properly addresses the issue of radar markers not disappearing in RViz. The 1-second lifetime ensures markers are automatically cleaned up.
Consider making the marker lifetime configurable via ROS parameters for flexibility:
- marker.lifetime = rospy.Duration(1) + marker_lifetime = rospy.get_param('~marker_lifetime', 1.0) # default 1 second + marker.lifetime = rospy.Duration(marker_lifetime)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/perception/src/lidar_distance.py(6 hunks)code/perception/src/radar_node.py(3 hunks)
🔇 Additional comments (6)
code/perception/src/lidar_distance.py (4)
10-11: Imports for visualization look good.
The addition of imports for Marker, MarkerArray, and Point appears consistent with the bounding box visualization logic introduced below.
127-139: Straightforward combination of XYZ and cluster labels.
The logic properly stacks labels with coordinates before generating bounding boxes. This is a clean approach and maintains clarity regarding which point belongs to which cluster.
75-86: Ensure consistency between publishers and parameter names.
The new publishers, including "marker_visualization_lidar_publisher", are properly initialized. Double-check that the parameter "~marker_topic" is set correctly in the ROS parameter server to avoid potential mismatches or typos when launching.
✅ Verification successful
Parameter naming and default values are consistent and well-structured
The ~marker_topic parameter is used consistently in both the radar and lidar nodes, with appropriate default values that follow a clear naming convention:
- Lidar node:
/paf/hero/Lidar/Marker - Radar node:
/paf/hero/Radar/Marker
The parameter doesn't need to be explicitly defined in launch files since both publishers use sensible default values that follow the project's topic naming pattern, making use of ROS's parameter fallback mechanism.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that "~marker_topic" parameter is defined in the relevant launch or parameter files.
# Searching for the param definition.
rg 'marker_topic'
Length of output: 233
Script:
#!/bin/bash
# Let's check the launch files to see if these parameters are defined
fd -e launch | xargs rg 'marker_topic'
# Also check for any yaml config files that might define these parameters
fd -e yaml | xargs rg 'marker_topic'
# Let's also check the full context of both files to understand the parameter usage
echo "=== radar_node.py context ==="
rg -B 5 -A 5 'marker_topic' code/perception/src/radar_node.py
echo "=== lidar_distance.py context ==="
rg -B 5 -A 5 'marker_topic' code/perception/src/lidar_distance.py
Length of output: 1181
565-565: DBSCAN eps parameter changed from 0.3 to 0.4.
Increasing the neighborhood distance may merge clusters that were previously separated. If this is intended, fine. Otherwise, confirm it doesn’t over-cluster objects.
✅ Verification successful
Parameter change to eps=0.4 appears safe to proceed with.
The DBSCAN eps parameter change from 0.3 to 0.4 is used only in one place within the codebase, specifically in the cluster_lidar_data_from_pointcloud function. The function is called from the start_clustering method with default parameters, and there are no test files that directly test this clustering functionality. The parameter change will make the clustering slightly more lenient, potentially merging nearby clusters, but this appears to be an intentional tuning decision for better object detection in the LiDAR processing pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Confirm any references or tests that might rely on a 0.3 EPS value.
rg 'cluster_lidar_data_from_pointcloud'
Length of output: 272
Script:
#!/bin/bash
# Let's check the git history for this specific function to understand the eps parameter change
git log -L :cluster_lidar_data_from_pointcloud:code/perception/src/lidar_distance.py
# Also check for any test files that might be using this function
rg -l "test.*lidar.*distance"
Length of output: 31
Script:
#!/bin/bash
# Let's check the content of the file to understand the usage context
cat code/perception/src/lidar_distance.py
# Also search for test files with a broader pattern
fd "test.*\.py"
Length of output: 22400
code/perception/src/radar_node.py (2)
164-164: Verify clustering behavior with reduced eps value
Reducing eps from 0.8 to 0.4 will create tighter clusters, which could improve object separation but might also split larger objects into multiple clusters. Please verify this change with different types of objects (vehicles, pedestrians, etc.).
Let's check if there are any test files or recorded data that we can analyze:
34-34: LGTM: Refined height filtering parameters
The adjusted height range (-0.40m to 2m) provides better focus on relevant objects while filtering out ground noise and tall irrelevant objects.
Let's verify if these height parameters align with typical object heights in your dataset:
✅ Verification successful
Height filtering parameters are consistent with radar mounting
The filter parameters (min_z=-0.40, max_z=2) are well-aligned with the radar's mounting height of 0.7m. The range allows detection from ~1.1m below the sensor to ~1.3m above it, which is appropriate for detecting relevant objects while filtering out ground noise and tall irrelevant objects.
- Comment in code confirms radar mounting height:
# radar position z=0.7 - Default filter_data parameters show this is a more focused range than the function's default of -1m to 100m
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the height distribution of radar points to validate filter parameters
# Search for any configuration files that might contain radar mounting height or related parameters
rg -g '*.yaml' -g '*.json' -g '*.config' -l 'radar.*height|radar.*z|radar.*mount'
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search to understand the radar height filtering context
# Search for any files containing filter_data function definition or usage
rg -A 5 -B 5 "filter_data.*z" --type python
# Also search for any comments or documentation about height filtering
rg -A 2 -B 2 "height|z.*filter" --type python
Length of output: 155
Script:
#!/bin/bash
# Search for filter_data function definition and usage without file type restriction
rg -A 5 -B 5 "filter_data.*z"
# Look for the filter_data function definition
ast-grep --pattern 'def filter_data($$$)'
Length of output: 719
Script:
#!/bin/bash
# Let's find the filter_data function implementation and any comments about radar mounting
rg -A 10 "def filter_data"
# Also search for any radar mounting height information in comments
rg "radar.*position|radar.*height|radar.*mount"
Length of output: 668
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/perception/src/lidar_distance.py (2)
480-487: Remove commented-out code for 2D boxesThe commented-out code for 2D bounding boxes should be removed if it's no longer needed. If it might be needed in the future, consider moving it to a separate branch or documenting why it's kept.
413-413: Consider increasing marker lifetimeThe current marker lifetime of 0.1 seconds might be too short, potentially causing flickering in the visualization. Consider increasing it to 0.2-0.5 seconds for smoother visualization while maintaining responsiveness.
- marker.lifetime = rospy.Duration(0.1) # Marker visibility duration in seconds + marker.lifetime = rospy.Duration(0.3) # Marker visibility duration in seconds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/perception/src/lidar_distance.py(6 hunks)
🔇 Additional comments (4)
code/perception/src/lidar_distance.py (4)
10-11: LGTM: Visualization message setup
The new imports and publisher setup for marker visualization are well-configured following ROS conventions.
Also applies to: 82-86
122-150: LGTM: Well-structured clustering and visualization implementation
The implementation demonstrates:
- Proper error handling for empty clusters
- Clear separation between clustering and visualization logic
- Efficient use of ROS visualization messages
566-566: Verify the impact of increased DBSCAN epsilon
The DBSCAN epsilon parameter has been increased from 0.3 to 0.4, which will result in more relaxed clustering. Please ensure this change:
- Doesn't lead to over-clustering of separate objects
- Has been validated with different types of objects and scenarios
118-119: Verify the impact of increased height threshold
The minimum height threshold for point filtering has been increased by 0.3 units. Please verify that this change doesn't affect the detection of important low-lying objects.
Description
Added the visualization for lidar clustered objects. Solved a bug where radar markers did not vanish in the rviz visualiziation. Optimized parameters for the dbscan clustering method in the radar_node and the lidar_distance node.
Fixes #584
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
no
Most important changes
Which files functionalities are most important in this PR. On which part should the reviewer be focussed on?
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation