524 general distance measurement of not detected objects for lidar data #558
Conversation
- Refactored the existing node code by modularizing it into functions for better readability and maintainability.
…zing it into functions for better readability and maintainability.
|
Warning Rate limit exceeded@michalal7 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes in this pull request involve minor formatting adjustments and comments in the launch configuration for a perception system, alongside significant enhancements to the Changes
Assessment against linked issues
Possibly related issues
Suggested reviewers
Poem
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: 5
🧹 Outside diff range and nitpick comments (8)
code/perception/src/lidar_distance.py (6)
2-9: Import Statements Optimization and Possible Missing ImportsThe newly added imports introduce significant functionalities like parallel processing (
Parallel,delayed), clustering (DBSCAN), and image conversion (CvBridge). Ensure that all these libraries are installed in your environment and included in yourrequirements.txtorsetup.pyto avoid runtime errors.Additionally, if you're using
rospyandros_numpy, it's good practice to group imports from the same library together for readability.
147-166: Optimize Image Calculation FunctionsCurrently, the
start_image_calculationmethod calls multiple image calculation functions sequentially, which can be time-consuming.Consider using multithreading or multiprocessing to parallelize image calculations:
+import threading def start_image_calculation(self, data): ... threads = [] directions = ["Center", "Back", "Left", "Right"] for direction in directions: thread = threading.Thread(target=self.calculate_image, args=(coordinates, direction, processed_images)) threads.append(thread) thread.start() for thread in threads: thread.join() self.publish_images(processed_images, data.header)And then refactor your calculation functions into a single
calculate_imagemethod.
Line range hint
167-244: Reduce Code Duplication in Image Calculation MethodsThe methods
calculate_image_center,calculate_image_back,calculate_image_left, andcalculate_image_rightshare a lot of duplicated code.Refactor these methods into a single method with parameters to handle different focuses.
Apply this diff:
-def calculate_image(self, coordinates, focus): +def calculate_image(self, coordinates, focus): # Set bounding box parameters based on focus if focus == "Center": bounding_params = {"max_x": np.inf, "min_x": 0.0, "min_z": -1.6} elif focus == "Back": bounding_params = {"max_x": 0.0, "min_x": -np.inf, "min_z": -1.6} elif focus == "Left": bounding_params = {"max_y": np.inf, "min_y": 0.0, "min_z": -1.6} elif focus == "Right": bounding_params = {"max_y": -0.0, "min_y": -np.inf, "min_z": -1.6} else: return None # Invalid focus reconstruct_bit_mask = lidar_filter_utility.bounding_box( coordinates, **bounding_params ) reconstruct_coordinates = coordinates[reconstruct_bit_mask] reconstruct_coordinates_xyz = np.array( lidar_filter_utility.remove_field_name( reconstruct_coordinates, "intensity" ).tolist() ) dist_array = self.reconstruct_img_from_lidar( reconstruct_coordinates_xyz, focus=focus ) return dist_arrayThen, update
start_image_calculationto use this method.🧰 Tools
🪛 Ruff (0.8.0)
103-103: Yoda condition detected
Rewrite as
coordinates["x"] >= -2(SIM300)
105-105: Yoda condition detected
Rewrite as
coordinates["y"] >= -1(SIM300)
245-270: Potential Bug in Publishing ImagesIn the
publish_imagesmethod, all theifstatements should beelifto prevent multiple condition checks once a match is found.Apply this diff to improve efficiency:
if direction == "Center": self.dist_array_center_publisher.publish(dist_array_msg) - if direction == "Back": + elif direction == "Back": self.dist_array_back_publisher.publish(dist_array_msg) - if direction == "Left": + elif direction == "Left": self.dist_array_left_publisher.publish(dist_array_msg) - if direction == "Right": + elif direction == "Right": self.dist_array_right_publisher.publish(dist_array_msg)
347-372: Redundant Functionarray_to_pointcloud2The function
array_to_pointcloud2seems redundant sinceros_numpyalready provides a similar utility.Consider removing this function or ensure it provides additional functionality beyond the existing utilities.
464-473: Improve Logging and Error Handling in Clustering FunctionIn
cluster_lidar_data_from_pointcloud, excessive logging at error level can clutter the logs.Change the log level to
rospy.logwarnfor empty input arrays.Also, ensure that you handle cases where
xyzis empty to prevent potential exceptions.code/agent/config/rviz_config.rviz (2)
3-9: Adjust GUI Layout for Optimal VisibilityThe changes in
Help HeightandTree Heightaffect the user interface layout.Ensure that these values provide the best experience for users on different screen resolutions.
Line range hint
342-360: Camera View AdjustmentsThe changes to
Distance,Pitch, andYawadjust the default camera view in RViz.Verify that these settings provide a useful initial view for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
code/agent/config/rviz_config.rviz(7 hunks)code/perception/launch/perception.launch(1 hunks)code/perception/src/lidar_distance.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
code/perception/src/lidar_distance.py
103-103: Yoda condition detected
Rewrite as coordinates["x"] >= -2
(SIM300)
105-105: Yoda condition detected
Rewrite as coordinates["y"] >= -1
(SIM300)
485-485: Ambiguous variable name: l
(E741)
🔇 Additional comments (6)
code/perception/src/lidar_distance.py (2)
133-145:
Ensure Correct Header Assignment in publish_clusters Method
In line 139, you set pointcloud_msg.header = data_header after setting pointcloud_msg.header.stamp. This might overwrite the timestamp you've just set.
Apply this diff to correct the header assignment order:
pointcloud_msg = ros_numpy.point_cloud2.array_to_pointcloud2(combined_clusters)
-pointcloud_msg.header = data_header
pointcloud_msg.header.stamp = rospy.Time.now()
+pointcloud_msg.header = data_headerEnsure that data_header includes the correct timestamp or set the stamp after assigning the header.
Likely invalid or redundant comment.
258-260: Specify Encoding Correctly in cv2_to_imgmsg Conversion
Ensure that the encoding parameter in cv2_to_imgmsg is correct. Using "passthrough" may not be appropriate for all image types.
Run the following script to check the encoding:
Check if the image arrays are compatible with the specified encoding.
✅ Verification successful
Let me gather more context about the image array type and processing to make an informed decision.
Let me try one more search to understand the image array processing better.
Let me check one final thing about the image array's nature by looking at the reconstruction method.
Using "passthrough" encoding is appropriate for this LiDAR distance array
The current encoding is correct because:
- The image array is a reconstructed 2D distance array from LiDAR data, not a standard RGB/BGR image
- "passthrough" is the appropriate encoding when dealing with raw numerical data arrays that don't conform to standard image formats
- Other image conversions in the codebase (in vision_node.py) explicitly use "rgb8" when dealing with actual RGB images, showing intentional encoding choices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the image encoding used in cv2_to_imgmsg.
rg 'cv2_to_imgmsg' code/perception/src/lidar_distance.py -A 2
Length of output: 183
Script:
#!/bin/bash
# Check the image_array creation and type
rg -B 10 "dist_array_msg = self\.bridge\.cv2_to_imgmsg" code/perception/src/lidar_distance.py
Length of output: 610
Script:
#!/bin/bash
# Look for any other cv2_to_imgmsg usage in the codebase for comparison
rg "cv2_to_imgmsg" --type python
Length of output: 63
Script:
#!/bin/bash
# Check the image array processing
ast-grep --pattern 'image_array = $_'
Length of output: 39
Script:
#!/bin/bash
# Check the broader context of the image processing
rg -B 20 -A 10 "dist_array_msg = self\.bridge\.cv2_to_imgmsg" code/perception/src/lidar_distance.py
Length of output: 1416
Script:
#!/bin/bash
# Search for cv2_to_imgmsg with correct file type flag
rg "cv2_to_imgmsg" -t py
Length of output: 352
Script:
#!/bin/bash
# Check the reconstruct_img_from_lidar method to understand the image array type
rg -B 5 -A 10 "def reconstruct_img_from_lidar" code/perception/src/lidar_distance.py
Length of output: 800
code/perception/launch/perception.launch (1)
78-78: Verify the Replacement of lidar_distance.py with debug_wrapper.py
Replacing the node type with debug_wrapper.py changes how the lidar_distance node operates.
Ensure that debug_wrapper.py properly initializes and calls lidar_distance.py with the provided arguments.
Run the following script to check if debug_wrapper.py exists and is executable:
✅ Verification successful
Let me check the content of debug_wrapper.py to verify its functionality.
Let me verify if lidar_distance.py exists in the same directory.
Replacement of lidar_distance.py with debug_wrapper.py is properly implemented
The verification confirms that:
debug_wrapper.pyexists, is executable, and implements a proper debug wrapper that:- Takes the original node file as
--debug_nodeargument - Properly initializes debugpy for remote debugging
- Executes the target node (lidar_distance.py) with all its arguments
- Takes the original node file as
lidar_distance.pyexists and contains the actual node implementation- The launch file correctly configures the wrapper with:
debug_wrapper.pyas the node typelidar_distance.pyas the--debug_nodeargument- Debug port configuration via
--debug_port=53000
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that debug_wrapper.py exists and is executable.
if [ ! -x "code/perception/src/debug_wrapper.py" ]; then
echo "debug_wrapper.py is missing or not executable."
else
echo "debug_wrapper.py is present and executable."
fi
Length of output: 150
Script:
#!/bin/bash
# Check the implementation of debug_wrapper.py
cat code/perception/src/debug_wrapper.py
Length of output: 6056
Script:
#!/bin/bash
# Check if lidar_distance.py exists and check its content
ls -l code/perception/src/lidar_distance.py
cat code/perception/src/lidar_distance.py
Length of output: 19311
code/agent/config/rviz_config.rviz (3)
67-70: Confirm Visibility Settings for PointCloud2 Data
Enabling PointCloud2 and adding PointCloud2 dist_clustered as visible ensures that clustered data is displayed.
75-79: Ensure Proper Configuration of VisonNode Output Properties
The properties Max Value, Median window, and Min Value have been added, and Normalize Range is set to true.
Ensure that these settings are compatible with the data being visualized and that they enhance the visualization appropriately.
258-313: Validate the Added PointCloud2 Displays
Multiple PointCloud2 displays have been added for different topics.
Make sure that the topics /paf/hero/dist_clustered and others are publishing data as expected.
| @@ -18,19 +18,159 @@ class LidarDistance: | |||
| how to configute this node | |||
| """ | |||
|
|
|||
| cluster_buffer = [] | |||
There was a problem hiding this comment.
Avoid Using Mutable Default Class Attributes
Defining cluster_buffer as a class variable can lead to unexpected behaviors, especially in a multithreaded environment or when multiple instances of the class are created. It is better to initialize it within the __init__ method as an instance variable.
Apply this diff to fix the issue:
-class LidarDistance:
+class LidarDistance:
+ def __init__(self):
+ self.cluster_buffer = []
- cluster_buffer = []Committable suggestion skipped: line range outside the PR's diff.
| """ | ||
| Filtert LiDAR-Punktwolken, führt Clustering durch und veröffentlicht die kombinierten Cluster. | ||
|
|
||
| :param data: a PointCloud2 | ||
| :param data: LiDAR-Punktwolken im ROS PointCloud2-Format. | ||
| """ | ||
|
|
||
| # Punktwolken filtern, um irrelevante Daten zu entfernen | ||
| coordinates = ros_numpy.point_cloud2.pointcloud2_to_array(data) | ||
| filtered_coordinates = coordinates[ | ||
| ~( | ||
| (-2 <= coordinates["x"]) | ||
| & (coordinates["x"] <= 2) | ||
| & (-1 <= coordinates["y"]) | ||
| & (coordinates["y"] <= 1) | ||
| ) # Ausschluss von Punkten die das eigene Auto betreffen | ||
| & ( | ||
| coordinates["z"] > -1.7 + 0.05 | ||
| ) # Mindesthöhe in z, um die Straße nicht zu clustern | ||
| ] | ||
|
|
||
| # Cluster-Daten aus den gefilterten Koordinaten berechnen | ||
| clustered_points = cluster_lidar_data_from_pointcloud( | ||
| coordinates=filtered_coordinates | ||
| ) | ||
|
|
||
| # Center | ||
| # Nur gültige Cluster-Daten speichern | ||
| if clustered_points: | ||
| LidarDistance.cluster_buffer.append(clustered_points) | ||
| else: | ||
| rospy.logwarn("Keine Cluster-Daten erzeugt.") | ||
|
|
||
| # Cluster kombinieren | ||
| combined_clusters = combine_clusters(LidarDistance.cluster_buffer) | ||
|
|
||
| LidarDistance.cluster_buffer = [] | ||
|
|
||
| # Veröffentliche die kombinierten Cluster | ||
| self.publish_clusters(combined_clusters, data.header) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Clustering Function Error Handling and Logic
Several issues were identified in the start_clustering method:
-
Yoda Conditions Detected: Lines 103 and 105 use Yoda conditions, which can reduce readability.
-
Static Analysis Warning: As per the static analysis hints, rewrite the conditions to avoid Yoda conditions.
Apply this diff to fix the Yoda conditions:
-(-2 <= coordinates["x"])
+ (coordinates["x"] >= -2)
-(-1 <= coordinates["y"])
+ (coordinates["y"] >= -1)-
Appending to a Class Variable: Using
LidarDistance.cluster_buffer.append(...)can cause issues as mentioned previously. Replace it withself.cluster_buffer. -
Handling Empty Cluster Cases: The else block logs a warning when no clusters are generated, but the code proceeds to combine and publish clusters regardless. Consider adding a condition to skip publishing if there are no clusters.
Apply this diff to address these issues:
-def start_clustering(self, data):
+def start_clustering(self, data):
...
- # Only store valid cluster data
if clustered_points:
- LidarDistance.cluster_buffer.append(clustered_points)
+ self.cluster_buffer.append(clustered_points)
- else:
+ else:
rospy.logwarn("Keine Cluster-Daten erzeugt.")
+ return # Exit the method if no clusters are generated
# Combine clusters
- combined_clusters = combine_clusters(LidarDistance.cluster_buffer)
+ combined_clusters = combine_clusters(self.cluster_buffer)
- LidarDistance.cluster_buffer = []
+ self.cluster_buffer = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| Filtert LiDAR-Punktwolken, führt Clustering durch und veröffentlicht die kombinierten Cluster. | |
| :param data: a PointCloud2 | |
| :param data: LiDAR-Punktwolken im ROS PointCloud2-Format. | |
| """ | |
| # Punktwolken filtern, um irrelevante Daten zu entfernen | |
| coordinates = ros_numpy.point_cloud2.pointcloud2_to_array(data) | |
| filtered_coordinates = coordinates[ | |
| ~( | |
| (-2 <= coordinates["x"]) | |
| & (coordinates["x"] <= 2) | |
| & (-1 <= coordinates["y"]) | |
| & (coordinates["y"] <= 1) | |
| ) # Ausschluss von Punkten die das eigene Auto betreffen | |
| & ( | |
| coordinates["z"] > -1.7 + 0.05 | |
| ) # Mindesthöhe in z, um die Straße nicht zu clustern | |
| ] | |
| # Cluster-Daten aus den gefilterten Koordinaten berechnen | |
| clustered_points = cluster_lidar_data_from_pointcloud( | |
| coordinates=filtered_coordinates | |
| ) | |
| # Center | |
| # Nur gültige Cluster-Daten speichern | |
| if clustered_points: | |
| LidarDistance.cluster_buffer.append(clustered_points) | |
| else: | |
| rospy.logwarn("Keine Cluster-Daten erzeugt.") | |
| # Cluster kombinieren | |
| combined_clusters = combine_clusters(LidarDistance.cluster_buffer) | |
| LidarDistance.cluster_buffer = [] | |
| # Veröffentliche die kombinierten Cluster | |
| self.publish_clusters(combined_clusters, data.header) | |
| """ | |
| Filtert LiDAR-Punktwolken, führt Clustering durch und veröffentlicht die kombinierten Cluster. | |
| :param data: LiDAR-Punktwolken im ROS PointCloud2-Format. | |
| """ | |
| # Punktwolken filtern, um irrelevante Daten zu entfernen | |
| coordinates = ros_numpy.point_cloud2.pointcloud2_to_array(data) | |
| filtered_coordinates = coordinates[ | |
| ~( | |
| (coordinates["x"] >= -2) | |
| & (coordinates["x"] <= 2) | |
| & (coordinates["y"] >= -1) | |
| & (coordinates["y"] <= 1) | |
| ) # Ausschluss von Punkten die das eigene Auto betreffen | |
| & ( | |
| coordinates["z"] > -1.7 + 0.05 | |
| ) # Mindesthöhe in z, um die Straße nicht zu clustern | |
| ] | |
| # Cluster-Daten aus den gefilterten Koordinaten berechnen | |
| clustered_points = cluster_lidar_data_from_pointcloud( | |
| coordinates=filtered_coordinates | |
| ) | |
| # Nur gültige Cluster-Daten speichern | |
| if clustered_points: | |
| self.cluster_buffer.append(clustered_points) | |
| else: | |
| rospy.logwarn("Keine Cluster-Daten erzeugt.") | |
| return # Exit the method if no clusters are generated | |
| # Cluster kombinieren | |
| combined_clusters = combine_clusters(self.cluster_buffer) | |
| self.cluster_buffer = [] | |
| # Veröffentliche die kombinierten Cluster | |
| self.publish_clusters(combined_clusters, data.header) |
🧰 Tools
🪛 Ruff (0.8.0)
103-103: Yoda condition detected
Rewrite as coordinates["x"] >= -2
(SIM300)
105-105: Yoda condition detected
Rewrite as coordinates["y"] >= -1
(SIM300)
| Callback-Funktion, die LiDAR-Punktwolkendaten verarbeitet. | ||
|
|
||
| Führt Clustering und Bildberechnungen für die Punktwolken aus. | ||
|
|
||
| :param data: LiDAR-Punktwolken als ROS PointCloud2-Nachricht. | ||
| """ |
There was a problem hiding this comment.
💡 Codebase verification
Docstrings Should Be Consistent in English
The review comment is correct. The file shows a clear inconsistency in language usage - all docstrings in the file are written in German except for the class docstring. For maintainability and consistency, these should be translated to English to match the codebase's primary language.
Specific docstrings that need translation:
callback()method docstringlistener()method docstringstart_clustering()method docstringpublish_clusters()method docstringstart_image_calculation()method docstringcalculate_image_center()method docstringpublish_images()method docstringreconstruct_img_from_lidar()method docstringarray_to_pointcloud2()function docstringget_largest_cluster()function docstringcombine_clusters()function docstringcluster_lidar_data_from_pointcloud()function docstring
🔗 Analysis chain
Consider Consistent Language in Docstrings
The docstrings in the callback method are in German, whereas other parts of the code may be in English. For consistency and maintainability, it's recommended to use a single language throughout the codebase.
Run the following script to check for non-English words in docstrings:
Let me generate a new script to better analyze the language consistency in docstrings:
This script will help us see the docstrings in the specific file and surrounding context to better assess the language consistency issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find non-English words in docstrings.
rg '"""[^"]*"""' code/ -U -o -r '$0' | grep -E '[^\x00-\x7F]'
Length of output: 98398
Script:
#!/bin/bash
# Search for docstrings in the specific file and its directory to check language consistency
rg -U "\"\"\"[\s\S]*?\"\"\"" code/perception/src/lidar_distance.py -A 2 -B 2
Length of output: 5255
|
|
||
| # Erstelle ein Dictionary mit Cluster-IDs und den zugehörigen Punkten | ||
| clusters = Parallel(n_jobs=-1)( | ||
| delayed(lambda l: (l, xyz[labels == l]))(label) for label in valid_labels |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ambiguous Variable Name: l
As per the static analysis hints, using l as a variable name can be confusing.
Apply this diff to rename the variable:
-clusters = Parallel(n_jobs=-1)(
- delayed(lambda l: (l, xyz[labels == l]))(label) for label in valid_labels
+clusters = Parallel(n_jobs=-1)(
+ delayed(lambda label: (label, xyz[labels == label]))(label) for label in valid_labels📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delayed(lambda l: (l, xyz[labels == l]))(label) for label in valid_labels | |
| delayed(lambda label: (label, xyz[labels == label]))(label) for label in valid_labels |
🧰 Tools
🪛 Ruff (0.8.0)
485-485: Ambiguous variable name: l
(E741)
| @@ -75,7 +75,7 @@ | |||
| <param name="role_name" value="$(arg role_name)" /> | |||
| </node> | |||
|
|
|||
| <node pkg="perception" type="lidar_distance.py" name="lidar_distance" output="screen"> | |||
| <node pkg="perception" type="debug_wrapper.py" name="lidar_distance" output="screen" args="--debug_node=lidar_distance.py --debug_port=53000"> | |||
There was a problem hiding this comment.
Update Parameter Names to Match Node Expectations
The parameters point_cloud_topic and range_topic are being set, but the updated lidar_distance.py may expect different parameter names after refactoring.
Verify that the parameter names match those expected by the node.
…cluster which was not used anywhere.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
code/perception/src/lidar_distance.py (1)
21-22: Add type hints for better code maintainabilityConsider adding type hints to improve code maintainability and IDE support.
Example improvements:
- def __init__(self): - self.cluster_buffer = [] + def __init__(self) -> None: + self.cluster_buffer: List[Dict[int, np.ndarray]] = [] - def start_clustering(self, data): + def start_clustering(self, data: PointCloud2) -> None: """ Filters LiDAR point clouds, performs clustering, and publishes the combined clusters. - :param data: LiDAR point clouds in ROS PointCloud2 format. + :param data: LiDAR point clouds in ROS PointCloud2 format """ - def calculate_image(self, coordinates, focus): + def calculate_image(self, coordinates: np.ndarray, focus: str) -> Optional[np.ndarray]:Don't forget to add the imports:
from typing import Dict, List, OptionalAlso applies to: 92-97, 166-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
code/perception/src/lidar_distance.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
code/perception/src/lidar_distance.py
402-402: Ambiguous variable name: l
(E741)
🔇 Additional comments (2)
code/perception/src/lidar_distance.py (2)
401-403: 🛠️ Refactor suggestion
Replace ambiguous variable name
The variable name 'l' is ambiguous and could be confused with the number '1'.
Apply this diff:
clusters = Parallel(n_jobs=-1)(
- delayed(lambda l: (l, xyz[labels == l]))(label) for label in valid_labels
+ delayed(lambda label_id: (label_id, xyz[labels == label_id]))(label) for label in valid_labels
)🧰 Tools
🪛 Ruff (0.8.0)
402-402: Ambiguous variable name: l
(E741)
204-208: 🛠️ Refactor suggestion
Maintain consistent language in docstrings
Some docstrings are in German while others are in English. For consistency and maintainability, all documentation should be in English.
For example, translate these docstrings:
- """
- Veröffentlicht Distanzbilder für verschiedene Richtungen als ROS-Bildnachrichten.
-
- :param processed_images: Dictionary mit Richtungen ("Center", "Back", etc.) als Schlüssel und Bildarrays als Werte.
- :param data_header: Header der ROS-Bildnachrichten.
- """
+ """
+ Publishes distance images for different directions as ROS image messages.
+
+ :param processed_images: Dictionary with directions ("Center", "Back", etc.) as keys and image arrays as values.
+ :param data_header: Header for the ROS image messages.
+ """- """
- Initialisiert die LidarDistance-Klasse und startet die Listener-Methode.
- """
+ """
+ Initializes the LidarDistance class and starts the listener method.
+ """Also applies to: 411-412
| def start_clustering(self, data): | ||
| """ | ||
| Filters LiDAR point clouds, performs clustering, and publishes the combined clusters. | ||
|
|
||
| :param data: LiDAR point clouds in ROS PointCloud2 format. | ||
| """ | ||
|
|
||
| # Filter point clouds to remove irrelevant data | ||
| coordinates = ros_numpy.point_cloud2.pointcloud2_to_array(data) | ||
| filtered_coordinates = coordinates[ | ||
| ~( | ||
| (coordinates["x"] >= -2) | ||
| & (coordinates["x"] <= 2) | ||
| & (coordinates["y"] >= -1) | ||
| & (coordinates["y"] <= 1) | ||
| ) # Exclude points related to the ego vehicle | ||
| & ( | ||
| coordinates["z"] > -1.7 + 0.05 | ||
| ) # Minimum height in z to exclude the road | ||
| ] | ||
|
|
||
| # Compute cluster data from the filtered coordinates | ||
| clustered_points = cluster_lidar_data_from_pointcloud( | ||
| coordinates=filtered_coordinates | ||
| ) | ||
|
|
||
| # Only store valid cluster data | ||
| if clustered_points: | ||
| self.cluster_buffer.append(clustered_points) | ||
| else: | ||
| rospy.logwarn("No cluster data generated.") | ||
|
|
||
| # Combine clusters | ||
| combined_clusters = combine_clusters(self.cluster_buffer) | ||
|
|
||
| self.cluster_buffer = [] | ||
|
|
||
| # Publish the combined clusters | ||
| self.publish_clusters(combined_clusters, data.header) | ||
|
|
There was a problem hiding this comment.
Missing emergency brake functionality
The PR objectives mention publishing an emergency brake message when a defined threshold is violated, but this functionality is not implemented in the clustering logic.
Consider adding threshold checks and emergency brake message publishing:
def start_clustering(self, data):
# ... existing code ...
# Combine clusters
combined_clusters = combine_clusters(self.cluster_buffer)
+
+ # Check for emergency brake conditions
+ min_safe_distance = rospy.get_param('~min_safe_distance', 2.0) # meters
+ if combined_clusters.size > 0:
+ distances = np.sqrt(combined_clusters['x']**2 + combined_clusters['y']**2)
+ if np.min(distances) < min_safe_distance:
+ emergency_msg = Bool()
+ emergency_msg.data = True
+ self.emergency_brake_publisher.publish(emergency_msg)
self.cluster_buffer = []Don't forget to:
- Add the publisher in
__init__:
self.emergency_brake_publisher = rospy.Publisher(
rospy.get_param('~emergency_brake_topic', '/hero/emergency_brake'),
Bool,
queue_size=1
)- Import the message type:
from std_msgs.msg import Bool| def array_to_pointcloud2(points, header="hero/Lidar"): | ||
| """ | ||
| Converts an array of points into a ROS PointCloud2 message. | ||
|
|
||
| :param points: Array of points with [x, y, z] coordinates. | ||
| :param header: Header information for the ROS PointCloud2 message. | ||
| :return: ROS PointCloud2 message. | ||
| """ |
There was a problem hiding this comment.
Fix header parameter type in array_to_pointcloud2
The header parameter is documented as a string but should be a ROS Header object.
Apply this diff:
-def array_to_pointcloud2(points, header="hero/Lidar"):
+def array_to_pointcloud2(points, header=None):
"""
Converts an array of points into a ROS PointCloud2 message.
:param points: Array of points with [x, y, z] coordinates.
- :param header: Header information for the ROS PointCloud2 message.
+ :param header: ROS Header object (optional).
:return: ROS PointCloud2 message.
"""
+ if header is None:
+ header = Header()
+ header.frame_id = "hero/Lidar"Don't forget to add the import:
from std_msgs.msg import Header| def cluster_lidar_data_from_pointcloud(coordinates, eps=0.3, min_samples=10): | ||
| """ | ||
| Performs clustering on LiDAR data using DBSCAN and returns the clusters. | ||
|
|
||
| :param coordinates: LiDAR point cloud as a NumPy array with "x", "y", "z". | ||
| :param eps: Maximum distance between points to group them into a cluster. | ||
| :param min_samples: Minimum number of points required to form a cluster. | ||
| :return: Dictionary with cluster IDs and their corresponding point clouds. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make clustering parameters configurable
The DBSCAN parameters (eps and min_samples) are hardcoded. These should be configurable via ROS parameters to allow tuning without code changes.
Apply this diff:
-def cluster_lidar_data_from_pointcloud(coordinates, eps=0.3, min_samples=10):
+def cluster_lidar_data_from_pointcloud(coordinates, eps=None, min_samples=None):
"""
Performs clustering on LiDAR data using DBSCAN and returns the clusters.
:param coordinates: LiDAR point cloud as a NumPy array with "x", "y", "z".
- :param eps: Maximum distance between points to group them into a cluster.
- :param min_samples: Minimum number of points required to form a cluster.
+ :param eps: Maximum distance between points (defaults to ROS parameter).
+ :param min_samples: Minimum points per cluster (defaults to ROS parameter).
:return: Dictionary with cluster IDs and their corresponding point clouds.
"""
+ if eps is None:
+ eps = rospy.get_param('~dbscan_eps', 0.3)
+ if min_samples is None:
+ min_samples = rospy.get_param('~dbscan_min_samples', 10)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def cluster_lidar_data_from_pointcloud(coordinates, eps=0.3, min_samples=10): | |
| """ | |
| Performs clustering on LiDAR data using DBSCAN and returns the clusters. | |
| :param coordinates: LiDAR point cloud as a NumPy array with "x", "y", "z". | |
| :param eps: Maximum distance between points to group them into a cluster. | |
| :param min_samples: Minimum number of points required to form a cluster. | |
| :return: Dictionary with cluster IDs and their corresponding point clouds. | |
| """ | |
| def cluster_lidar_data_from_pointcloud(coordinates, eps=None, min_samples=None): | |
| """ | |
| Performs clustering on LiDAR data using DBSCAN and returns the clusters. | |
| :param coordinates: LiDAR point cloud as a NumPy array with "x", "y", "z". | |
| :param eps: Maximum distance between points (defaults to ROS parameter). | |
| :param min_samples: Minimum points per cluster (defaults to ROS parameter). | |
| :return: Dictionary with cluster IDs and their corresponding point clouds. | |
| """ | |
| if eps is None: | |
| eps = rospy.get_param('~dbscan_eps', 0.3) | |
| if min_samples is None: | |
| min_samples = rospy.get_param('~dbscan_min_samples', 10) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
code/perception/src/lidar_distance.py (3)
100-112: Consider extracting filter parameters to configurationThe hardcoded values for filtering point clouds should be configurable parameters to allow easy adjustment without code changes.
def start_clustering(self, data): + # Load filter parameters from ROS params + ego_vehicle_x_min = rospy.get_param('~ego_vehicle_x_min', -2) + ego_vehicle_x_max = rospy.get_param('~ego_vehicle_x_max', 2) + ego_vehicle_y_min = rospy.get_param('~ego_vehicle_y_min', -1) + ego_vehicle_y_max = rospy.get_param('~ego_vehicle_y_max', 1) + min_height = rospy.get_param('~min_height', -1.7) + height_margin = rospy.get_param('~height_margin', 0.05) filtered_coordinates = coordinates[ ~( - (coordinates["x"] >= -2) - & (coordinates["x"] <= 2) - & (coordinates["y"] >= -1) - & (coordinates["y"] <= 1) + (coordinates["x"] >= ego_vehicle_x_min) + & (coordinates["x"] <= ego_vehicle_x_max) + & (coordinates["y"] >= ego_vehicle_y_min) + & (coordinates["y"] <= ego_vehicle_y_max) ) # Exclude points related to the ego vehicle & ( - coordinates["z"] > -1.7 + 0.05 + coordinates["z"] > min_height + height_margin ) # Minimum height in z to exclude the road ]
242-248: Extract camera parameters to configurationThe image dimensions (1280x720) and FOV (100°) should be configurable parameters.
+ # Load camera parameters from ROS params + image_width = rospy.get_param('~image_width', 1280) + image_height = rospy.get_param('~image_height', 720) + fov_degrees = rospy.get_param('~fov_degrees', 100) + # Create the intrinsic camera matrix based on image parameters (FOV, resolution) im = np.identity(3) - im[0, 2] = 1280 / 2.0 # x-offset (image center) - im[1, 2] = 720 / 2.0 # y-offset (image center) + im[0, 2] = image_width / 2.0 # x-offset (image center) + im[1, 2] = image_height / 2.0 # y-offset (image center) - im[0, 0] = im[1, 1] = 1280 / (2.0 * np.tan(100 * np.pi / 360.0)) + im[0, 0] = im[1, 1] = image_width / (2.0 * np.tan(fov_degrees * np.pi / 360.0))
Line range hint
259-304: Optimize point processing loopThe current implementation processes points sequentially. Consider using NumPy's vectorized operations for better performance.
- # Process each point in the point cloud - for c in coordinates_xyz: - if focus == "Center": # Compute image for the center view - point = np.array([c[1], c[2], c[0], 1]) + # Prepare points for vectorized operations + if focus == "Center": + points = np.column_stack((coordinates_xyz[:, 1], + coordinates_xyz[:, 2], + coordinates_xyz[:, 0], + np.ones(len(coordinates_xyz)))) + # Vectorized projection + pixels = (m @ points.T).T + pixels_xy = pixels[:, :2] / pixels[:, 2:3] + valid_mask = ((0 <= pixels_xy[:, 0]) & (pixels_xy[:, 0] <= image_width) & + (0 <= pixels_xy[:, 1]) & (pixels_xy[:, 1] <= image_height)) + + # Update arrays using vectorized operations + valid_pixels = pixels_xy[valid_mask].astype(int) + img[719 - valid_pixels[:, 1], 1279 - valid_pixels[:, 0]] = coordinates_xyz[valid_mask, 0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
code/perception/src/lidar_distance.py(3 hunks)
🔇 Additional comments (2)
code/perception/src/lidar_distance.py (2)
418-420: Translate German docstring to English
For consistency with the rest of the codebase, translate the German docstring to English.
This is a duplicate of a previous review comment about language consistency in docstrings.
92-132:
Critical: Implement emergency brake functionality
The PR objectives require emergency brake message publishing when obstacles are detected within a threshold distance, but this functionality is missing.
This is a duplicate of a previous review comment. Please refer to the implementation suggestion in the past review comments.
code/agent/config/rviz_config.rviz
Outdated
There was a problem hiding this comment.
I recommend leaving the configuration file unchanged for now and revisiting it once the clustering of radar and lidar data is fully operational.
There was a problem hiding this comment.
Please ensure all debugging code is removed before merging.
…cted-objects-for-lidar-data
Description
Fixes #524
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
no
Most important changes
Most important is the function start_clustering which is called in the callback function. It starts the whole clustering process.
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation