631 feature execution check function for planned local trajectory #654
Conversation
…of-lane-markings' into 631-feature-execution-check-function-for-planned-local-trajectory
…sed; [WIP] get entities in specified diameter around the car
…-check-function-for-planned-local-trajectory
WalkthroughThis pull request introduces enhancements to the mapping system's functionality, focusing on trajectory management, lane marking detection, and spatial entity analysis. The Changes
Suggested reviewers
Poem
✨ Finishing Touches
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: 6
🔭 Outside diff range comments (2)
code/perception/src/lidar_distance.py (2)
Line range hint
4-11: Add error handling for external dependencies.The code imports several external libraries but lacks proper error handling for import failures. This could lead to cryptic errors if dependencies are missing.
Add proper error handling:
-from joblib import Parallel, delayed -import rospy -import ros_numpy -import numpy as np -from lidar_filter_utility import bounding_box, remove_field_name -from sensor_msgs.msg import PointCloud2, Image as ImageMsg -from sklearn.cluster import DBSCAN -from cv_bridge import CvBridge +try: + from joblib import Parallel, delayed + import rospy + import ros_numpy + import numpy as np + from lidar_filter_utility import bounding_box, remove_field_name + from sensor_msgs.msg import PointCloud2, Image as ImageMsg + from sklearn.cluster import DBSCAN + from cv_bridge import CvBridge +except ImportError as e: + print(f"Failed to import required packages: {e}") + sys.exit(1)
Missing test coverage for critical LiDAR functionality confirmed
The perception module lacks any test coverage for the LiDAR processing components. Critical untested functionality includes:
- Point cloud processing and filtering
- Image reconstruction from LiDAR data
- DBSCAN clustering operations
- Error handling for edge cases
While other modules like
mappingdemonstrate proper test organization, the perception module needs similar test infrastructure. Consider following the established testing patterns from the mapping module's test structure atcode/mapping/tests/.🔗 Analysis chain
Line range hint
1-700: Add comprehensive test coverage.The file lacks unit tests for critical functionality:
- Point cloud processing
- Image reconstruction
- Clustering operations
- Error handling
Let's check the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze test coverage for LiDAR processing # Look for test files related to LiDAR processing echo "Searching for test files..." fd -e py test_lidar -x cat {} echo "Checking test coverage..." rg -i "test.*lidar" --type pythonLength of output: 226
Script:
#!/bin/bash # Search for Python test files and test-related content echo "=== Searching for Python test files ===" fd -e py "test_.*\.py$" echo -e "\n=== Searching for test configuration files ===" fd "pytest.ini|tox.ini|setup.cfg|conftest.py" echo -e "\n=== Searching for LiDAR-related test content ===" rg -l "class.*Test.*Lidar|def test_.*lidar|from.*lidar.*import" echo -e "\n=== Checking for test framework imports in Python files ===" rg "^import pytest|^from pytest|^import unittest|^from unittest" --type pyLength of output: 1024
Script:
#!/bin/bash # Analyze perception module structure and test files echo "=== Perception Module Structure ===" fd . code/perception -t d echo -e "\n=== Files in Perception Module ===" fd . code/perception -t f echo -e "\n=== Searching for any test-related files in perception ===" rg -l "test|assert|mock" code/perception/ echo -e "\n=== Looking for alternative test locations ===" fd -e py "(test|tests|testing)" --exclude "test_route.py"Length of output: 4123
🧹 Nitpick comments (25)
code/perception/src/lidar_distance.py (1)
Line range hint
297-361: Consider optimizing the image reconstruction process.The
reconstruct_img_from_lidarmethod could benefit from performance improvements:
- Pre-allocate arrays for better memory efficiency
- Vectorize the coordinate transformations
- Use NumPy's masked arrays instead of boolean indexing
Consider this optimization:
def reconstruct_img_from_lidar(self, coordinates_xyz, focus): - img = np.zeros(shape=(720, 1280), dtype=np.float32) - dist_array = np.zeros(shape=(720, 1280, 3), dtype=np.float32) + # Pre-allocate arrays with masked arrays + dist_array = np.ma.zeros(shape=(720, 1280, 3), dtype=np.float32) + dist_array.mask = True # All points are initially masked # Prepare points based on focus if focus in ["Center", "Back"]: - points = np.column_stack(( - coordinates_xyz[:, 1], - coordinates_xyz[:, 2], - coordinates_xyz[:, 0], - np.ones(coordinates_xyz.shape[0]), - )) + # Vectorized transformation + points = np.column_stack([ + coordinates_xyz[:, [1, 2, 0]], + np.ones(len(coordinates_xyz)) + ])code/perception/src/Lanedetection_node.py (4)
30-33: Consider graceful device fallback for non-CUDA environments.
Currently, the code assumes CUDA availability by default. Systems without CUDA support will raise an exception. As a best practice, implement a fallback to CPU if CUDA is not available, and log a warning.self.model = torch.hub.load("hustvl/yolop", "yolop", pretrained=True) -self.device = torch.device("cuda") +if torch.cuda.is_available(): + self.device = torch.device("cuda") +else: + self.device = torch.device("cpu") self.model.to(self.device)
39-40: Parameterize camera angles if needed.
The code only subscribes to "Center" camera images (setup_camera_subscriptions("Center")), but you have a parameter for role name. If you eventually plan to extend to additional camera angles (e.g., left/right), expose “side” as a parameter or handle it dynamically.
103-104: Avoid unnecessary frequent CUDA clear calls
Callingtorch.cuda.empty_cache()in every image callback might degrade performance. Invoke it strategically (e.g., only when memory usage is high or on a timed interval).
47-47: Remove redundant pass statement
Thepassstatement in line 47 is unnecessary sincespin()blocks until shutdown.self.spin() -passcode/mapping/ext_modules/mapping_common/map.py (3)
77-105: Docstring mismatch inentities_in_area
The docstring states it returns entities “without the hero car,” but also discusses “only checks if the first entity is hero.” The function indeed filters out the hero entity. Recommend clarifying the docstring and ensuring consistent behavior.
173-226: Simplify nestedifconditions for lane checks
Static analysis hints (SIM102) suggest combining nested conditions with a singleand. For example, lines 211–214 and 220–223 can be combined as oneif condition.if left_lane is not None: - if shapely.intersection( - left_lane.to_shapely().poly, local_trajectory_buffer - ): - return 2 + if shapely.intersection(left_lane.to_shapely().poly, local_trajectory_buffer): + return 2 if right_lane is not None: - if shapely.intersection( - right_lane.to_shapely().poly, local_trajectory_buffer - ): - return 3 + if shapely.intersection(right_lane.to_shapely().poly, local_trajectory_buffer): + return 3🧰 Tools
🪛 Ruff (0.8.2)
211-214: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
220-223: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
196-197: Consider lane buffer size
Usinglocal_trajectory.buffer(0.5, 3)might cause false positives or negatives, depending on vehicle size and lane width. Validate these parameters in real-world tests or add prompts for dynamic parameterization.code/perception/src/lane_position.py (4)
433-433: Address unused loop control variable
Static analysis flagged thatlabelis unused in this loop. Rename_labelto avoid confusion or to reflect usage if needed.-for label, cluster in clusters.items(): +for _label, cluster in clusters.items(): y = cluster[:, 1] ...🧰 Tools
🪛 Ruff (0.8.2)
433-433: Loop control variable
labelnot used within loop bodyRename unused
labelto_label(B007)
445-452: Combine inlineif-elsefor clarity
As static analysis suggests (SIM108), use a ternary operator for conciseness instead of a multi-lineif-else.-if np.ndim(res) == 2: - std_dev = np.sqrt(np.diag(res)) -else: - std_dev = np.sqrt(res) +std_dev = np.sqrt(np.diag(res)) if np.ndim(res) == 2 else np.sqrt(res)🧰 Tools
🪛 Ruff (0.8.2)
445-452: Use ternary operator
std_dev = np.sqrt(np.diag(res)) if np.ndim(res) == 2 else np.sqrt(res)instead ofif-else-block(SIM108)
628-628: Remove or utilize the unusedunique_labelsvariable
unique_labels = np.unique(clustered_mask)is assigned but never used. Consider removing it if not needed.-unique_labels = np.unique(clustered_mask)🧰 Tools
🪛 Ruff (0.8.2)
628-628: Local variable
unique_labelsis assigned to but never usedRemove assignment to unused variable
unique_labels(F841)
345-366: Cross-check confidence thresholds
The confidence calculation is purely heuristic. If the model or environment changes, these thresholds might become too restrictive or lenient. Provide documentation or adjustable parameters for consistent performance.code/mapping/msg/TypeLanemarking.msg (1)
2-3: Add documentation for new message fields.Please add comments to document:
- The meaning and valid range of
position_index- The semantic meaning of
predictedand when it's set to true/falsecode/mapping/launch/mapping.launch (2)
3-3: Consider making debug port configurable.The debug port (53000) is hardcoded. Consider making it configurable via a launch parameter for flexibility and to avoid potential port conflicts.
- <node pkg="mapping" type="debug_wrapper.py" name="mapping_data_integration" output="screen" args="--debug_node=mapping_data_integration.py --debug_port=53000"> + <arg name="debug_port" default="53000"/> + <node pkg="mapping" type="debug_wrapper.py" name="mapping_data_integration" output="screen" args="--debug_node=mapping_data_integration.py --debug_port=$(arg debug_port)">
14-15: Consider adding configuration parameters for path_test_node.The path_test node might benefit from configurable parameters such as:
- Update rate (currently hardcoded to 0.5s in the implementation)
- Frame ID (currently hardcoded to "hero")
- Path length (currently hardcoded to 10 points)
code/mapping/src/path_test.py (2)
7-10: Remove unused imports.The following imports are not used in the code:
mapping.msg.Mapmapping_common.map.Mapshapely🧰 Tools
🪛 Ruff (0.8.2)
7-7:
mapping.msg.Mapimported but unusedRemove unused import:
mapping.msg.Map(F401)
8-8:
mapping_common.map.Mapimported but unusedRemove unused import:
mapping_common.map.Map(F401)
10-10:
shapelyimported but unusedRemove unused import:
shapely(F401)
32-33: Make update rate configurable via ROS parameter.The trajectory update rate is hardcoded to 0.5 seconds. Consider making it configurable via a ROS parameter.
- self.rate = 0.5 - self.new_timer(0.5, self.generate_trajectory) + self.rate = self.get_parameter('update_rate', 0.5) + self.new_timer(self.rate, self.generate_trajectory)code/requirements.txt (1)
20-31: Consider grouping related dependencies.The new dependencies could be organized better by grouping them based on their purpose (e.g., visualization, ML processing, utilities).
Consider reorganizing like this:
numpy==1.23.5 ultralytics==8.3.32 scikit-learn>=0.18 pandas==2.0.3 debugpy==1.8.7 -tqdm -yacs - -matplotlib>=3.2.2 -Pillow -PyYAML>=5.3 -tensorboardX -seaborn -prefetch_generator -imageio +# ML Processing +tqdm +yacs +tensorboardX +prefetch_generator + +# Visualization +matplotlib>=3.2.2 +seaborn + +# Image Processing +Pillow +imageio + +# Configuration +PyYAML>=5.3doc/perception/Lanedetection_node.md (3)
3-4: Fix typos and improve language.There are several issues in these lines:
- "briev" should be "brief"
- Inconsistent capitalization of "Lane markings" vs "driveable Area"
-This Document gives a briev overview over the Lanedetection Node. -The Lanedetection Node is responsible for detecting the Lane markings and driveable Area in a given Image and publish the results. +This document gives a brief overview of the Lanedetection Node. +The Lanedetection Node is responsible for detecting the lane markings and driveable area in a given image and publishing the results.
63-64: Fix markdown formatting.The bullet point formatting is incorrect in the MarkerArray description.
- - - **Description**: Contains all Lidar points that collide with the lane mask. + - **Description**: Contains all Lidar points that collide with the lane mask.
29-41: Add more technical details to the Image Handler section.The Image Handler section could benefit from more technical details:
- What preprocessing steps are performed?
- What are the input image requirements (size, format)?
- What is the expected performance of the YOLOP model?
Consider expanding this section with these technical details to help future maintainers.
code/mapping/src/mapping_data_integration.py (3)
101-109: Consider parameterizing the trajectory topic name.The topic name "test_trajectory" is hardcoded. Consider using the
get_parammethod for better configurability, similar to other topic subscriptions in the class.- topic="test_trajectory", + topic=self.get_param("~trajectory_topic", "/test_trajectory"),
114-115: Add error handling and documentation to the callback method.The callback method should:
- Include docstring explaining the purpose
- Add type checking for the input data
- Handle potential exceptions
def callback(self, data: Path): + """Store the received trajectory for later use in lane marking predictions. + + Args: + data (Path): The trajectory message to store + """ + if not isinstance(data, Path): + self.logwarn(f"Received invalid trajectory data type: {type(data)}") + return self.local_trajectory = data
309-311: Consider parameterizing the area filter value.The area filter value (10) is hardcoded. Consider making it configurable through ROS parameters for better flexibility.
- map.entities = map.entities_in_area(10) + area_size = self.get_param("~map_area_size", 10) + map.entities = map.entities_in_area(area_size)Also applies to: 331-339
code/mapping/ext_modules/mapping_common/entity.py (1)
452-473: Consider using constants for visualization parameters.The marker visualization code uses magic numbers for colors and transparency. Consider extracting these as class constants for better maintainability.
class Lanemarking(Entity): + # Visualization constants + MARKER_ALPHA = 0.5 + MARKER_HEIGHT = 0.1 + MARKER_COLOR_MAX = 255 + def to_marker(self) -> Marker: m = self.shape.to_marker(self.transform) if self.predicted: - m.color.a = 0.5 - m.color.r = int(255 * self.confidence) + m.color.a = self.MARKER_ALPHA + m.color.r = int(self.MARKER_COLOR_MAX * self.confidence) m.color.g = 0 m.color.b = 0 else: - m.color.a = 0.5 + m.color.a = self.MARKER_ALPHA m.color.r = 0 m.color.g = 0 - m.color.b = int(255 * self.confidence) + m.color.b = int(self.MARKER_COLOR_MAX * self.confidence) - m.scale.z = 0.1 + m.scale.z = self.MARKER_HEIGHT m.pose.position.z = m.scale.z / 2.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
doc/assets/perception/Overview_lanedetection_node.jpgis excluded by!**/*.jpg
📒 Files selected for processing (15)
code/agent/config/rviz_config.rviz(5 hunks)code/mapping/ext_modules/.debug_enabled(1 hunks)code/mapping/ext_modules/mapping_common/entity.py(1 hunks)code/mapping/ext_modules/mapping_common/map.py(4 hunks)code/mapping/launch/mapping.launch(2 hunks)code/mapping/msg/TypeLanemarking.msg(1 hunks)code/mapping/src/mapping_data_integration.py(6 hunks)code/mapping/src/path_test.py(1 hunks)code/mapping/tests/mapping_common/test_entity.py(1 hunks)code/perception/launch/perception.launch(2 hunks)code/perception/src/Lanedetection_node.py(1 hunks)code/perception/src/lane_position.py(1 hunks)code/perception/src/lidar_distance.py(1 hunks)code/requirements.txt(1 hunks)doc/perception/Lanedetection_node.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- code/mapping/ext_modules/.debug_enabled
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/src/path_test.py
7-7: mapping.msg.Map imported but unused
Remove unused import: mapping.msg.Map
(F401)
8-8: mapping_common.map.Map imported but unused
Remove unused import: mapping_common.map.Map
(F401)
10-10: shapely imported but unused
Remove unused import: shapely
(F401)
code/mapping/ext_modules/mapping_common/map.py
211-214: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
220-223: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
code/perception/src/lane_position.py
433-433: Loop control variable label not used within loop body
Rename unused label to _label
(B007)
445-452: Use ternary operator std_dev = np.sqrt(np.diag(res)) if np.ndim(res) == 2 else np.sqrt(res) instead of if-else-block
(SIM108)
628-628: Local variable unique_labels is assigned to but never used
Remove assignment to unused variable unique_labels
(F841)
🔇 Additional comments (15)
code/perception/src/lidar_distance.py (2)
213-213: Validate alignment with lane detection requirements.The removal of the z-axis constraint (
min_z: -np.inf) aligns with the PR's lane detection objectives by potentially including more road surface points. However, ensure this doesn't introduce noise that could affect lane detection accuracy.Let's validate the impact on lane detection:
213-213: Verify the impact of extending the z-axis range.The change from
min_z: -1.6tomin_z: -np.inffor the "Center" direction removes the lower bound constraint on the z-axis. This could significantly impact the point cloud processing by:
- Including ground points and underground structures
- Potentially increasing processing time due to more points
- Affecting the quality of distance images
Let's analyze the impact:
✅ Verification successful
Extending z-axis range for center direction is intentionally designed
The change from
-1.6to-np.inffor the center direction'smin_zis a deliberate design choice that:
- Only affects forward-facing point cloud processing
- Maintains ground filtering (-1.6) for back, left, and right directions
- Allows detection of below-ground features in front of the vehicle while keeping safe filtering elsewhere
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the impact of removing z-axis constraint # 1. Check if there are any critical dependencies on the z-axis filtering # 2. Look for potential performance implications # Check for dependencies on z-axis filtering echo "Checking for z-axis dependencies..." rg -A 5 "min_z" --type python # Look for performance-related comments or issues echo "Checking for performance considerations..." rg -i "performance|processing time|optimization" --type python # Check for test coverage echo "Checking for related tests..." fd -e py test_lidar -x cat {}Length of output: 435
Script:
#!/bin/bash # Search for z-axis dependencies and point cloud processing echo "Checking z-axis dependencies..." rg "min_z" -A 5 echo "Checking point cloud processing..." rg "point_cloud|PointCloud" -A 5 echo "Checking performance considerations..." rg -i "performance|processing time|optimization" echo "Finding and checking test files..." fd "test.*lidar.*\.py$" -x cat {}Length of output: 36082
code/perception/src/Lanedetection_node.py (2)
45-47: Check missing parameter defaults
Parameters like"epsilon_clustering","min_samples_clustering", etc., assume a valid value in the launch/config file. Ensure that sensible defaults are set if they are not provided, or warn the user.
121-123: Validate image encoding
When converting theImageMsg(line 123), consider adding defensive checks (e.g., catch exceptions or verify the encoding). This helps prevent runtime errors if an unexpected image format arrives.code/mapping/ext_modules/mapping_common/map.py (1)
161-171: Check lane marking existence
Bothlane_marking_left()andlane_marking_right()returnNoneif no matching lane marking is found. Ensure callers handleNoneproperly to avoidAttributeError.code/perception/src/lane_position.py (1)
386-391: Handle empty or malformed distance arrays
When no valid distance data arrives,mask_lidarpointsreturns an empty list. Confirm calling methods gracefully handle empty data to prevent index errors or unexpected behaviors down the pipeline.code/mapping/src/path_test.py (1)
35-59: 🛠️ Refactor suggestionImprove trajectory generation implementation.
Several concerns with the current implementation:
- Random trajectory generation might not be suitable for testing deterministic behaviors
- Hardcoded values (10 points, curve factors) should be configurable
- No validation of generated trajectory for feasibility
- Missing documentation for the trajectory generation algorithm
Consider:
- Making the number of points configurable
- Adding different trajectory patterns (straight, circle, figure-8)
- Validating generated paths for maximum curvature
- Documenting the trajectory generation approach
Also, the orientation (quaternion) is set to identity (w=1) without considering the path tangent, which might not be realistic for vehicle motion.
code/mapping/src/mapping_data_integration.py (2)
Line range hint
309-339: Verify map publishing conditions.The map publishing conditions have changed to include lane markings. Ensure that the map is still published correctly when lane markings are not available.
✅ Verification successful
Map publishing conditions are properly handled
The code includes comprehensive null checks for all required components, including lane markings. The map publishing will safely return early if lane markings or any other required component is not available, preventing any potential issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if map publishing works without lane markings # Test: Search for error handling in map publishing rg -A 5 "publish_new_map" | rg "return|None" # Test: Search for map publishing with null checks rg -A 10 "map_publisher.publish"Length of output: 1659
Script:
#!/bin/bash # Find variable initialization and the complete publish_new_map method ast-grep --pattern 'def publish_new_map($$$) { $$$ }' # Search for variable initialization rg "self\.lidar_data =|self\.lanemarkings =|self\.radar_marker_data =" # Search for any error handling patterns rg -B 2 -A 2 "if.*None.*return"Length of output: 576
Script:
#!/bin/bash # Get the complete publish_new_map method implementation rg -U "def publish_new_map.*?(?=def|\z)" -A 50 --multiline # Look for map publishing patterns rg -B 5 -A 5 "map_publisher"Length of output: 3562
22-26: LGTM! The new imports and subscriptions are correctly set up.The changes properly integrate the new lane markings functionality with appropriate ROS message types and subscriptions.
Also applies to: 53-61
code/requirements.txt (2)
21-30: Verify version constraints for critical dependencies.Several new ML and visualization dependencies are added without version constraints. This could lead to compatibility issues or breaking changes in the future.
Let's check for known compatibility issues and latest versions:
✅ Verification successful
Add version constraints for critical dependencies
Based on the analysis of package versions and stability:
- Add minimum version constraints for Pillow (
>=11.0.0) and tensorboardX (>=2.6.0) as they handle critical image processing and visualization tasks- Current packages are stable and actively maintained, except yacs which is stable but unmaintained
- No immediate breaking changes or critical deprecations found in recent versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest versions and compatibility for pkg in tqdm yacs Pillow tensorboardX seaborn prefetch_generator imageio; do echo "Checking $pkg..." curl -s "https://pypi.org/pypi/$pkg/json" | jq -r '. | "Latest version: \(.info.version)\nRelease date: \(.releases[.info.version][0].upload_time)\n"' done # Check for known vulnerabilities pip-audit tqdm yacs Pillow tensorboardX seaborn prefetch_generator imageioLength of output: 2575
Script:
#!/bin/bash # Check for known vulnerabilities using safety pip install safety safety check tqdm yacs Pillow tensorboardX seaborn prefetch_generator imageio # Check for deprecation notices in latest releases for pkg in tqdm yacs Pillow tensorboardX seaborn prefetch_generator imageio; do echo "=== Checking $pkg changelog ===" curl -s "https://pypi.org/pypi/$pkg/json" | jq -r '.info.description' | grep -i -A 5 "deprecat\|breaking" || true doneLength of output: 7766
24-26: Verify compatibility between matplotlib and PyYAML versions.The version constraints
matplotlib>=3.2.2andPyYAML>=5.3need verification against the existing dependencies to ensure no conflicts arise.✅ Verification successful
Dependencies are compatible
The dry-run installation test confirms that matplotlib>=3.2.2 and PyYAML>=5.3 are compatible with each other and with other major dependencies in the project. No version conflicts were detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check compatibility with existing dependencies pip install matplotlib==3.2.2 PyYAML==5.3 torch==2.1.2 numpy==1.23.5 --dry-runLength of output: 6151
doc/perception/Lanedetection_node.md (1)
10-11: Verify the system diagram accessibility.The system diagram is crucial for understanding the node's architecture. Please ensure:
- The image file exists at the specified path
- The image is accessible to all team members
- The diagram is up-to-date with the implementation
✅ Verification successful
System diagram is properly accessible and up-to-date ✓
The diagram file exists with appropriate read permissions for all team members. The recent modification date (Jan 26) and the inclusion of an editable Lucid chart link ensures the team has access to both the current diagram and the ability to collaborate on updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the system diagram exists and is accessible test -f doc/assets/perception/Overview_lanedetection_node.jpg && echo "Image exists" || echo "Image missing" # Get image metadata to verify last modification date stat doc/assets/perception/Overview_lanedetection_node.jpgLength of output: 254
Script:
#!/bin/bash # Get detailed file information including permissions and timestamps ls -la doc/assets/perception/Overview_lanedetection_node.jpg # Check file type file doc/assets/perception/Overview_lanedetection_node.jpg # Get file size in human-readable format du -h doc/assets/perception/Overview_lanedetection_node.jpgLength of output: 444
code/agent/config/rviz_config.rviz (1)
3-9: LGTM! The RViz configuration changes look good.The changes enhance the visualization setup by:
- Adjusting display dimensions for better visibility
- Adding a new Path entry for test trajectory visualization
- Optimizing window layout
Also applies to: 69-69, 212-235, 289-305
code/mapping/ext_modules/mapping_common/entity.py (2)
420-421: LGTM! The new Lanemarking attributes are well-defined.The addition of
position_indexandpredictedattributes with proper initialization looks good.Also applies to: 427-433
437-440: LGTM! The serialization methods are properly updated.The
_extract_kwargsandto_ros_msgmethods correctly handle the new attributes.Also applies to: 445-450
| position_index=1, | ||
| predicted=False, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for new Lanemarking parameters.
The current test only verifies basic conversion with fixed values. Consider adding test cases for:
- Negative position indices
- Edge cases for position_index (-128, 127)
- Both true/false cases for predicted flag
- Invalid position_index values
Example additional test cases:
def test_lanemark_conversion_edge_cases():
# Test negative index
e1 = entity.Lanemarking(
confidence=1.0, priority=1.0, shape=get_shape(),
transform=get_transform(), style=entity.Lanemarking.Style.SOLID,
position_index=-1, predicted=True
)
msg1 = e1.to_ros_msg()
e1_conv = entity.Entity.from_ros_msg(msg1)
assert e1 == e1_conv
# Test maximum index
e2 = entity.Lanemarking(
confidence=1.0, priority=1.0, shape=get_shape(),
transform=get_transform(), style=entity.Lanemarking.Style.SOLID,
position_index=127, predicted=False
)
msg2 = e2.to_ros_msg()
e2_conv = entity.Entity.from_ros_msg(msg2)
assert e2 == e2_conv| <node pkg="perception" type="Lanedetection_node.py" name="Lanedetection" output="screen"> | ||
| <param name="role_name" value="$(arg role_name)" /> | ||
| </node> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add camera configuration parameters to the Lanedetection node.
The node appears to be vision-based but lacks essential camera configuration parameters. Consider adding similar camera parameters as the lane_position node for consistency and configurability.
| <node pkg="perception" type="lane_position.py" name="lane_position" output="screen"> | ||
| <param name="camera_x" value="0.0"/> | ||
| <param name="camera_y" value="0.0"/> | ||
| <param name="camera_z" value="1.7"/> | ||
| <param name="camera_roll" value="0.0"/> | ||
| <param name="camera_pitch" value="0.0"/> | ||
| <param name="camera_yaw" value="0.0"/> | ||
| <param name="camera_width" value="1280"/> | ||
| <param name="camera_height" value="720"/> | ||
| <param name="camera_fov" value="100"/> | ||
| <param name="epsilon_clustering" value="1.8"/> | ||
| <param name="min_samples_clustering" value="4"/> | ||
| <param name="angle_weight" value="0.3"/> | ||
| <param name="size_weight" value="0.3"/> | ||
| <param name="std_dev_weight" value="0.4"/> | ||
| <param name="angle_normalization" value="25"/> | ||
| <param name="size_normalization" value="15"/> | ||
| <param name="std_dev_normalization" value="0.1"/> | ||
| <param name="angle_prediction_threshold" value="5"/> | ||
| <param name="confidence_threshold" value="0.6"/> | ||
| <param name="y_tolerance" value="1.0"/> | ||
| </node> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document parameters and avoid hardcoded values.
The node configuration has several issues:
- Camera parameters are hardcoded without explanation
- Clustering parameters use magic numbers
- No documentation for parameter meanings and valid ranges
Consider:
- Moving parameters to a separate config file
- Adding documentation for each parameter
- Using meaningful variable names for magic numbers
Example config structure:
camera:
position:
x: 0.0 # Distance from vehicle center in meters
y: 0.0 # Lateral offset in meters
z: 1.7 # Height from ground in meters
orientation:
roll: 0.0 # Roll angle in degrees
pitch: 0.0 # Pitch angle in degrees
yaw: 0.0 # Yaw angle in degrees
properties:
width: 1280 # Image width in pixels
height: 720 # Image height in pixels
fov: 100 # Field of view in degrees
clustering:
epsilon: 1.8 # Maximum distance between points in cluster
min_samples: 4 # Minimum points to form a cluster
weights:
angle: 0.3 # Weight for angle-based clustering
size: 0.3 # Weight for size-based clustering
std_dev: 0.4 # Weight for standard deviation| topic="test_trajectory", | ||
| msg_type=Path, | ||
| callback=self.callback, | ||
| qos_profile=1, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Path messages.
The subscription to test_trajectory lacks error handling for invalid messages. Consider adding try-catch blocks and message validation.
def callback(self, data: Path):
+ try:
+ if not data.poses:
+ self.logwarn("Received empty path message")
+ return
self.local_trajectory = data
+ except AttributeError as e:
+ self.logerr(f"Invalid path message: {e}")Committable suggestion skipped: line range outside the PR's diff.
| self.loginfo( | ||
| "Trajectory check: " + str(map.check_trajectory(self.local_trajectory)) | ||
| ) | ||
| if map.check_trajectory(self.local_trajectory) == 2: | ||
| for lane in self.lanemarkings: | ||
| if lane.position_index == 1: | ||
| lane.predicted = True | ||
|
|
||
| if map.check_trajectory(self.local_trajectory) == 3: | ||
| for lane in self.lanemarkings: | ||
| if lane.position_index == -1: | ||
| lane.predicted = True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve trajectory check logic.
The code has several issues:
- Magic numbers (2, 3) in trajectory check comparisons
- No error handling for invalid trajectory checks
- Redundant trajectory check calls
+ # Define trajectory check results as constants
+ LEFT_TRAJECTORY = 2
+ RIGHT_TRAJECTORY = 3
+
+ try:
+ check_result = map.check_trajectory(self.local_trajectory)
self.loginfo(
- "Trajectory check: " + str(map.check_trajectory(self.local_trajectory))
+ f"Trajectory check: {check_result}"
)
- if map.check_trajectory(self.local_trajectory) == 2:
+ if check_result == LEFT_TRAJECTORY:
for lane in self.lanemarkings:
if lane.position_index == 1:
lane.predicted = True
- if map.check_trajectory(self.local_trajectory) == 3:
+ elif check_result == RIGHT_TRAJECTORY:
for lane in self.lanemarkings:
if lane.position_index == -1:
lane.predicted = True
+ except Exception as e:
+ self.logerr(f"Error in trajectory check: {e}")📝 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.
| self.loginfo( | |
| "Trajectory check: " + str(map.check_trajectory(self.local_trajectory)) | |
| ) | |
| if map.check_trajectory(self.local_trajectory) == 2: | |
| for lane in self.lanemarkings: | |
| if lane.position_index == 1: | |
| lane.predicted = True | |
| if map.check_trajectory(self.local_trajectory) == 3: | |
| for lane in self.lanemarkings: | |
| if lane.position_index == -1: | |
| lane.predicted = True | |
| # Define trajectory check results as constants | |
| LEFT_TRAJECTORY = 2 | |
| RIGHT_TRAJECTORY = 3 | |
| try: | |
| check_result = map.check_trajectory(self.local_trajectory) | |
| self.loginfo( | |
| f"Trajectory check: {check_result}" | |
| ) | |
| if check_result == LEFT_TRAJECTORY: | |
| for lane in self.lanemarkings: | |
| if lane.position_index == 1: | |
| lane.predicted = True | |
| elif check_result == RIGHT_TRAJECTORY: | |
| for lane in self.lanemarkings: | |
| if lane.position_index == -1: | |
| lane.predicted = True | |
| except Exception as e: | |
| self.logerr(f"Error in trajectory check: {e}") |
| def lanemarkings_callback(self, data: MapMsg): | ||
| map = Map.from_ros_msg(data) | ||
| self.lanemarkings = map.entities_without_hero() | ||
|
|
||
| self.loginfo( | ||
| "Trajectory check: " + str(map.check_trajectory(self.local_trajectory)) | ||
| ) | ||
| if map.check_trajectory(self.local_trajectory) == 2: | ||
| for lane in self.lanemarkings: | ||
| if lane.position_index == 1: | ||
| lane.predicted = True | ||
|
|
||
| if map.check_trajectory(self.local_trajectory) == 3: | ||
| for lane in self.lanemarkings: | ||
| if lane.position_index == -1: | ||
| lane.predicted = True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve trajectory checking logic and error handling.
The lane marking callback needs:
- Error handling for invalid trajectory data
- Documentation for the trajectory check return values (what do 2 and 3 mean?)
- Consider extracting the prediction logic into a separate method
def lanemarkings_callback(self, data: MapMsg):
+ """Process lane markings and update their predicted status based on trajectory.
+
+ Args:
+ data (MapMsg): The map message containing lane markings
+ """
map = Map.from_ros_msg(data)
self.lanemarkings = map.entities_without_hero()
+ if not hasattr(self, 'local_trajectory') or not self.local_trajectory:
+ self.logwarn("No trajectory data available for checking")
+ return
+
self.loginfo(
"Trajectory check: " + str(map.check_trajectory(self.local_trajectory))
)
+
+ # Extract prediction logic to a separate method for better readability
+ self._update_lane_predictions(map.check_trajectory(self.local_trajectory))
+
+def _update_lane_predictions(self, check_result: int) -> None:
+ """Update lane marking predictions based on trajectory check result.
+
+ Args:
+ check_result (int): The result from trajectory checking
+ 2: Right lane deviation detected
+ 3: Left lane deviation detected
+ """
if map.check_trajectory(self.local_trajectory) == 2:
for lane in self.lanemarkings:
if lane.position_index == 1:
lane.predicted = True
if map.check_trajectory(self.local_trajectory) == 3:
for lane in self.lanemarkings:
if lane.position_index == -1:
lane.predicted = TrueCommittable suggestion skipped: line range outside the PR's diff.
…-check-function-for-planned-local-trajectory
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
code/mapping/ext_modules/mapping_common/map.py (4)
181-214: Unify front/back filtering & fix docstring typo.
- The “entities_in_area” logic is duplicated for front/back. You could unify both loops by conditionally checking
entity.transform.translation().x() > 0and distance in the same loop, reducing duplication.- There’s a small spelling issue in the docstring at line 185 (“returend” should be “returned”).
- behind the car, are returend too. + behind the car, are returned too. # (Optional) Example unification snippet def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: entities = self.entities_without_hero() entities_in_area = [] for e in entities: distance = np.sqrt(e.transform.translation().x()**2 + e.transform.translation().y()**2) - if only_in_front: - if e.transform.translation().x() > 0 and distance < radius: - entities_in_area.append(e) - else: - if distance < radius: - entities_in_area.append(e) + # Combine logic for front/back + if distance < radius and (not only_in_front or e.transform.translation().x() > 0): + entities_in_area.append(e) return entities_in_area
268-273: Return type mismatch.Although this method signature suggests returning an
Entity, you also returnNone. Consider updating the return type toOptional[Entity]or ensuring the docstring explains thatNonemay be returned.- def lane_marking_left(self) -> Entity: + def lane_marking_left(self) -> Optional[Entity]:🧰 Tools
🪛 Ruff (0.8.2)
269-269: Import
entityfrom line 12 shadowed by loop variable(F402)
274-279: Match return type with possibleNone.Same as above: update signature and/or docstring to clarify that
Nonecan be returned.- def lane_marking_right(self) -> Entity: + def lane_marking_right(self) -> Optional[Entity]:🧰 Tools
🪛 Ruff (0.8.2)
275-275: Import
entityfrom line 12 shadowed by loop variable(F402)
280-336: Consolidate nested conditionals & clarify intersection checks.
- Combine nested checks for left/right lane into a single
ifstatement each (e.g.,if lane is not None and shapely.intersection(...):). This addresses the static analysis hint (SIM102).- Be explicit about geometry intersection checks by testing
intersection_result.is_emptyto clarify logic.--- a/map.py +++ b/map.py @@ -317,10 +317,9 @@ left_lane = self.lane_marking_left() if left_lane is not None: - if shapely.intersection( - left_lane.to_shapely().poly, local_trajectory_buffer - ): - return 2 + intersection_result = shapely.intersection(left_lane.to_shapely().poly, local_trajectory_buffer) + if intersection_result and not intersection_result.is_empty: + return 2 # Check right lane right_lane = self.lane_marking_right() @@ -327,10 +326,9 @@ if right_lane is not None: - if shapely.intersection( - right_lane.to_shapely().poly, local_trajectory_buffer - ): - return 3 + intersection_result = shapely.intersection(right_lane.to_shapely().poly, local_trajectory_buffer) + if intersection_result and not intersection_result.is_empty: + return 3🧰 Tools
🪛 Ruff (0.8.2)
307-307: Import
entityfrom line 12 shadowed by loop variable(F402)
318-321: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
327-330: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/ext_modules/mapping_common/map.py(4 hunks)code/mapping/src/mapping_data_integration.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/src/mapping_data_integration.py
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
111-111: SyntaxError: unindent does not match any outer indentation level
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
code/mapping/ext_modules/mapping_common/map.py
269-269: Import entity from line 12 shadowed by loop variable
(F402)
275-275: Import entity from line 12 shadowed by loop variable
(F402)
307-307: Import entity from line 12 shadowed by loop variable
(F402)
318-321: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
327-330: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (4)
code/mapping/src/mapping_data_integration.py (2)
111-113: Add error handling for Path messages.This addition sets
self.local_trajectorybut doesn’t account for invalid or emptyPathmessages. Consider validatingdata.posesor catching attribute errors here.def callback(self, data: Path): + if not data.poses: + self.logwarn("Received empty or invalid Path message.") + return self.local_trajectory = data🧰 Tools
🪛 Ruff (0.8.2)
111-111: SyntaxError: unindent does not match any outer indentation level
345-345: Looks good.No functional changes on this line; the invocation of
self.map_publisher.publish(msg)is clear and consistent with the rest of the method.code/mapping/ext_modules/mapping_common/map.py (2)
6-6: Import usage appears valid.This import for
shapely.plottingmay be used for any advanced debugging or visualization; no issue flagged by static analysis.
19-19: Nav_msgs import is appropriate.The import of
Pathfromnav_msgs.msgis valid and used in the newcheck_trajectorymethod.
| self.lidar_data = None | ||
| >>>>>>>>> Temporary merge branch 2 |
There was a problem hiding this comment.
Resolve merge conflict.
There’s a merge conflict marker at line 58 (>>>>>>>>> Temporary merge branch 2) that breaks Python syntax and needs removal or resolution.
57 self.lidar_data = None
-58 >>>>>>>>> Temporary merge branch 2
+58 # (Resolve or remove the conflict marker)📝 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.
| self.lidar_data = None | |
| >>>>>>>>> Temporary merge branch 2 | |
| self.lidar_data = None | |
| # (Resolve or remove the conflict marker) |
🧰 Tools
🪛 Ruff (0.8.2)
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Expected a statement
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
58-58: SyntaxError: Simple statements must be separated by newlines or semicolons
| @@ -146,6 +151,7 @@ | |||
| def lidar_callback(self, data: PointCloud2): | |||
| self.lidar_data = data | |||
|
|
|||
| <<<<<<<<< Temporary merge branch 1 | |||
There was a problem hiding this comment.
Resolve merge conflict.
Conflicting marker (<<<<<<<<< Temporary merge branch 1) at line 154 breaks Python syntax. Remove or resolve it.
-154 <<<<<<<<< Temporary merge branch 1
+# (Resolve or remove the conflict marker)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Expected a statement
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
154-154: SyntaxError: Simple statements must be separated by newlines or semicolons
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
code/mapping/src/mapping_data_integration.py (2)
110-112:⚠️ Potential issueAdd error handling for Path messages.
The callback lacks error handling for invalid messages. Consider adding try-catch blocks and message validation.
def callback(self, data: Path): + try: + if not data.poses: + self.logwarn("Received empty path message") + return self.local_trajectory = data + except AttributeError as e: + self.logerr(f"Invalid path message: {e}")
153-153:⚠️ Potential issueResolve merge conflict.
There's a merge conflict marker (
<<<<<<<<< Temporary merge branch 1) that breaks Python syntax.-<<<<<<<<< Temporary merge branch 1 +# (Resolve or remove the conflict marker)🧰 Tools
🪛 Ruff (0.8.2)
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
🧹 Nitpick comments (2)
code/mapping/src/mapping_data_integration.py (2)
57-57: Add type hints for consistency.The
lanemarkingsattribute should have type hints to maintain consistency with other class attributes.- self.lanemarkings = None + lanemarkings: Optional[List[Entity]] = None
344-344: Add documentation and validation for lane markings.The map publishing logic should include:
- Documentation about the expected format and structure of lane markings
- Validation of lane marking data before publishing
if self.lanemarkings is not None and self.get_param("~enable_lane_marker"): + # Validate lane markings before publishing + valid_markings = [ + marking for marking in self.lanemarkings + if hasattr(marking, 'position_index') + ] + if len(valid_markings) != len(self.lanemarkings): + self.logwarn("Some lane markings have invalid format") entities.extend(self.lanemarkings)Consider adding docstring:
def publish_new_map(self, timer_event=None): """Publish a new map with hero car, sensor data, and lane markings. Lane markings are expected to have: - position_index: int (-1 for left, 1 for right) - predicted: bool (True if predicted by trajectory) """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/src/mapping_data_integration.py(4 hunks)code/mapping/src/path_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/mapping/src/path_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/src/mapping_data_integration.py
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Expected a statement
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
153-153: SyntaxError: Simple statements must be separated by newlines or semicolons
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/ext_modules/mapping_common/map.py(4 hunks)code/mapping/src/mapping_data_integration.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/mapping/src/mapping_data_integration.py
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/ext_modules/mapping_common/map.py
318-321: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
327-330: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
| def lane_marking_left(self) -> Entity: | ||
| for entry in self.entities: | ||
| if entry.flags._is_lanemark is True and entry.position_index == 1: | ||
| return entry | ||
| return None | ||
|
|
||
| def lane_marking_right(self) -> Entity: | ||
| for entry in self.entities: | ||
| if entry.flags._is_lanemark is True and entry.position_index == -1: | ||
| return entry | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix return type annotation and add documentation.
The methods have incorrect return type annotations and lack proper documentation. Consider consolidating the duplicate logic into a helper method.
- def lane_marking_left(self) -> Entity:
+ def lane_marking_left(self) -> Optional[Entity]:
+ """Returns the left lane marking entity if it exists.
+
+ Returns:
+ Optional[Entity]: The left lane marking entity or None if not found
+ """
+ return self._get_lane_marking(position_index=1)
+
- for entry in self.entities:
- if entry.flags._is_lanemark is True and entry.position_index == 1:
- return entry
- return None
+ def lane_marking_right(self) -> Optional[Entity]:
+ """Returns the right lane marking entity if it exists.
+
+ Returns:
+ Optional[Entity]: The right lane marking entity or None if not found
+ """
+ return self._get_lane_marking(position_index=-1)
+
+ def _get_lane_marking(self, position_index: int) -> Optional[Entity]:
+ """Helper method to get lane marking by position index.
+
+ Args:
+ position_index (int): 1 for left, -1 for right
- def lane_marking_right(self) -> Entity:
+ Returns:
+ Optional[Entity]: The lane marking entity or None if not found
+ """
for entry in self.entities:
- if entry.flags._is_lanemark is True and entry.position_index == -1:
+ if entry.flags._is_lanemark and entry.position_index == position_index:
return entry
return None📝 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 lane_marking_left(self) -> Entity: | |
| for entry in self.entities: | |
| if entry.flags._is_lanemark is True and entry.position_index == 1: | |
| return entry | |
| return None | |
| def lane_marking_right(self) -> Entity: | |
| for entry in self.entities: | |
| if entry.flags._is_lanemark is True and entry.position_index == -1: | |
| return entry | |
| return None | |
| def lane_marking_left(self) -> Optional[Entity]: | |
| """Returns the left lane marking entity if it exists. | |
| Returns: | |
| Optional[Entity]: The left lane marking entity or None if not found | |
| """ | |
| return self._get_lane_marking(position_index=1) | |
| def lane_marking_right(self) -> Optional[Entity]: | |
| """Returns the right lane marking entity if it exists. | |
| Returns: | |
| Optional[Entity]: The right lane marking entity or None if not found | |
| """ | |
| return self._get_lane_marking(position_index=-1) | |
| def _get_lane_marking(self, position_index: int) -> Optional[Entity]: | |
| """Helper method to get lane marking by position index. | |
| Args: | |
| position_index (int): 1 for left, -1 for right | |
| Returns: | |
| Optional[Entity]: The lane marking entity or None if not found | |
| """ | |
| for entry in self.entities: | |
| if entry.flags._is_lanemark and entry.position_index == position_index: | |
| return entry | |
| return None |
| def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | ||
| """ Returns the entities without the hero car | ||
| in a given radius around the hero car. | ||
| Flag only_in_front decides if entities | ||
| behind the car, are returend too. | ||
| Only checks if the first entity is_hero | ||
|
|
||
| Returns: | ||
| List[Entity]: Entities in front and/or back of hero | ||
| without the hero car in given radius | ||
| """ | ||
| entities = self.entities_without_hero() | ||
| entities_in_area = [] | ||
|
|
||
| if only_in_front: | ||
| for entry in entities: | ||
| distance = np.sqrt( | ||
| np.power(entry.transform.translation().x(), 2) | ||
| + np.power(entry.transform.translation().y(), 2) | ||
| ) | ||
| if entry.transform.translation().x() > 0 and distance < radius: | ||
| entities_in_area.append(entry) | ||
| return entities_in_area | ||
| else: | ||
| for entry in entities: | ||
| distance = np.sqrt( | ||
| np.power(entry.transform.translation().x(), 2) | ||
| + np.power(entry.transform.translation().y(), 2) | ||
| ) | ||
| if distance < radius: | ||
| entities_in_area.append(entry) | ||
|
|
||
| return entities_in_area |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to improve code quality and performance.
- Add type hints and validation for the radius parameter
- Remove code duplication in distance calculation
- Consider using numpy vectorization for better performance
- def entities_in_area(self, radius, only_in_front=True) -> List[Entity]:
+ def entities_in_area(self, radius: float, only_in_front: bool = True) -> List[Entity]:
+ """Returns entities within a given radius around the hero car.
+
+ Args:
+ radius (float): The radius to search for entities (must be positive)
+ only_in_front (bool): If True, only return entities in front of the car
+
+ Returns:
+ List[Entity]: Filtered list of entities within the radius
+
+ Raises:
+ ValueError: If radius is negative
+ """
+ if radius < 0:
+ raise ValueError("Radius must be positive")
+
entities = self.entities_without_hero()
- entities_in_area = []
-
- if only_in_front:
- for entry in entities:
- distance = np.sqrt(
- np.power(entry.transform.translation().x(), 2)
- + np.power(entry.transform.translation().y(), 2)
- )
- if entry.transform.translation().x() > 0 and distance < radius:
- entities_in_area.append(entry)
- return entities_in_area
- else:
- for entry in entities:
- distance = np.sqrt(
- np.power(entry.transform.translation().x(), 2)
- + np.power(entry.transform.translation().y(), 2)
- )
- if distance < radius:
- entities_in_area.append(entry)
-
- return entities_in_area
+ # Vectorized distance calculation
+ positions = np.array([[e.transform.translation().x(), e.transform.translation().y()]
+ for e in entities])
+ distances = np.sqrt(np.sum(positions**2, axis=1))
+
+ if only_in_front:
+ mask = (positions[:, 0] > 0) & (distances < radius)
+ else:
+ mask = distances < radius
+
+ return [e for i, e in enumerate(entities) if mask[i]]📝 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 entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | |
| """ Returns the entities without the hero car | |
| in a given radius around the hero car. | |
| Flag only_in_front decides if entities | |
| behind the car, are returend too. | |
| Only checks if the first entity is_hero | |
| Returns: | |
| List[Entity]: Entities in front and/or back of hero | |
| without the hero car in given radius | |
| """ | |
| entities = self.entities_without_hero() | |
| entities_in_area = [] | |
| if only_in_front: | |
| for entry in entities: | |
| distance = np.sqrt( | |
| np.power(entry.transform.translation().x(), 2) | |
| + np.power(entry.transform.translation().y(), 2) | |
| ) | |
| if entry.transform.translation().x() > 0 and distance < radius: | |
| entities_in_area.append(entry) | |
| return entities_in_area | |
| else: | |
| for entry in entities: | |
| distance = np.sqrt( | |
| np.power(entry.transform.translation().x(), 2) | |
| + np.power(entry.transform.translation().y(), 2) | |
| ) | |
| if distance < radius: | |
| entities_in_area.append(entry) | |
| return entities_in_area | |
| def entities_in_area(self, radius: float, only_in_front: bool = True) -> List[Entity]: | |
| """Returns entities within a given radius around the hero car. | |
| Args: | |
| radius (float): The radius to search for entities (must be positive) | |
| only_in_front (bool): If True, only return entities in front of the car | |
| Returns: | |
| List[Entity]: Filtered list of entities within the radius | |
| Raises: | |
| ValueError: If radius is negative | |
| """ | |
| if radius < 0: | |
| raise ValueError("Radius must be positive") | |
| entities = self.entities_without_hero() | |
| # Vectorized distance calculation | |
| positions = np.array([[e.transform.translation().x(), e.transform.translation().y()] | |
| for e in entities]) | |
| distances = np.sqrt(np.sum(positions**2, axis=1)) | |
| if only_in_front: | |
| mask = (positions[:, 0] > 0) & (distances < radius) | |
| else: | |
| mask = distances < radius | |
| return [e for i, e in enumerate(entities) if mask[i]] |
| def check_trajectory(self, local_path: Path) -> int: | ||
| """ | ||
| Checks the calculated local path on multiple conditions | ||
| return -1: local_path is None | ||
| return 0: Path has no obstacles / infringments | ||
| return 1: Path is planned through a gap, | ||
| which is too small for the car (without lanes) | ||
| return 2: Path cuts the left lane marking | ||
| return 3: Path cuts the right lane marking | ||
| """ | ||
|
|
||
| # check if there is a local path | ||
| if local_path.poses is None: | ||
| return -1 | ||
|
|
||
| # transform local path to shapely object | ||
| trajectory_shapely_points = [] | ||
| for pos in local_path.poses: | ||
| trajectory_shapely_points.append( | ||
| shapely.Point(pos.pose.position.x, pos.pose.position.y) | ||
| ) | ||
| local_trajectory = shapely.LineString(trajectory_shapely_points) | ||
| # widen the Linestring to an area representing the cars width | ||
| local_trajectory_buffer = local_trajectory.buffer(1, 3) | ||
|
|
||
| # Check path collision with other entities | ||
| obstacles = self.entities_in_area(10) | ||
| for entry in obstacles: | ||
| if ( | ||
| shapely.intersection(entry.to_shapely().poly, local_trajectory_buffer) | ||
| and entry.flags._is_lanemark is False | ||
| ): | ||
| return 1 | ||
|
|
||
| # decrease buffer size for lane checkking | ||
| local_trajectory_buffer = local_trajectory.buffer(0.5, 3) | ||
| # Check left lane | ||
| left_lane = self.lane_marking_left() | ||
| if left_lane is not None: | ||
| if shapely.intersection( | ||
| left_lane.to_shapely().poly, local_trajectory_buffer | ||
| ): | ||
| # given path crosses lane marking on the left | ||
| return 2 | ||
|
|
||
| # Check right lane | ||
| right_lane = self.lane_marking_right() | ||
| if right_lane is not None: | ||
| if shapely.intersection( | ||
| right_lane.to_shapely().poly, local_trajectory_buffer | ||
| ): | ||
| # given path crosses lane marking on the right | ||
| return 3 | ||
|
|
||
| # given path is okay and is not colliding | ||
| return 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness and maintainability of trajectory checking.
- Extract magic numbers into named constants
- Add validation for empty poses list
- Simplify nested if statements
- Consider making the obstacle detection radius configurable
+ # Class-level constants
+ VEHICLE_WIDTH_BUFFER = 1.0 # meters
+ LANE_CHECK_BUFFER = 0.5 # meters
+ OBSTACLE_DETECTION_RADIUS = 10.0 # meters
+
def check_trajectory(self, local_path: Path) -> int:
"""
Checks the calculated local path on multiple conditions
+
+ Args:
+ local_path (Path): The path to check
+
+ Returns:
+ int: Status code indicating the path's validity:
+ -1: local_path is None or empty
+ 0: Path has no obstacles/infringements
+ 1: Path planned through gap too small for car
+ 2: Path cuts left lane marking
+ 3: Path cuts right lane marking
+
+ Raises:
+ ValueError: If local_path.poses is empty
"""
- if local_path.poses is None:
+ if local_path.poses is None or not local_path.poses:
return -1
# transform local path to shapely object
trajectory_shapely_points = []
for pos in local_path.poses:
trajectory_shapely_points.append(
shapely.Point(pos.pose.position.x, pos.pose.position.y)
)
local_trajectory = shapely.LineString(trajectory_shapely_points)
# widen the Linestring to an area representing the cars width
- local_trajectory_buffer = local_trajectory.buffer(1, 3)
+ local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3)
# Check path collision with other entities
- obstacles = self.entities_in_area(10)
+ obstacles = self.entities_in_area(self.OBSTACLE_DETECTION_RADIUS)
for entry in obstacles:
- if (
- shapely.intersection(entry.to_shapely().poly, local_trajectory_buffer)
- and entry.flags._is_lanemark is False
- ):
+ if (not entry.flags._is_lanemark and
+ shapely.intersection(entry.to_shapely().poly, local_trajectory_buffer)):
return 1
# decrease buffer size for lane checking
- local_trajectory_buffer = local_trajectory.buffer(0.5, 3)
+ local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3)
# Check left lane
left_lane = self.lane_marking_left()
- if left_lane is not None:
- if shapely.intersection(
- left_lane.to_shapely().poly, local_trajectory_buffer
- ):
- return 2
+ if left_lane is not None and shapely.intersection(
+ left_lane.to_shapely().poly, local_trajectory_buffer):
+ return 2
# Check right lane
right_lane = self.lane_marking_right()
- if right_lane is not None:
- if shapely.intersection(
- right_lane.to_shapely().poly, local_trajectory_buffer
- ):
- return 3
+ if right_lane is not None and shapely.intersection(
+ right_lane.to_shapely().poly, local_trajectory_buffer):
+ return 3
return 0📝 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 check_trajectory(self, local_path: Path) -> int: | |
| """ | |
| Checks the calculated local path on multiple conditions | |
| return -1: local_path is None | |
| return 0: Path has no obstacles / infringments | |
| return 1: Path is planned through a gap, | |
| which is too small for the car (without lanes) | |
| return 2: Path cuts the left lane marking | |
| return 3: Path cuts the right lane marking | |
| """ | |
| # check if there is a local path | |
| if local_path.poses is None: | |
| return -1 | |
| # transform local path to shapely object | |
| trajectory_shapely_points = [] | |
| for pos in local_path.poses: | |
| trajectory_shapely_points.append( | |
| shapely.Point(pos.pose.position.x, pos.pose.position.y) | |
| ) | |
| local_trajectory = shapely.LineString(trajectory_shapely_points) | |
| # widen the Linestring to an area representing the cars width | |
| local_trajectory_buffer = local_trajectory.buffer(1, 3) | |
| # Check path collision with other entities | |
| obstacles = self.entities_in_area(10) | |
| for entry in obstacles: | |
| if ( | |
| shapely.intersection(entry.to_shapely().poly, local_trajectory_buffer) | |
| and entry.flags._is_lanemark is False | |
| ): | |
| return 1 | |
| # decrease buffer size for lane checkking | |
| local_trajectory_buffer = local_trajectory.buffer(0.5, 3) | |
| # Check left lane | |
| left_lane = self.lane_marking_left() | |
| if left_lane is not None: | |
| if shapely.intersection( | |
| left_lane.to_shapely().poly, local_trajectory_buffer | |
| ): | |
| # given path crosses lane marking on the left | |
| return 2 | |
| # Check right lane | |
| right_lane = self.lane_marking_right() | |
| if right_lane is not None: | |
| if shapely.intersection( | |
| right_lane.to_shapely().poly, local_trajectory_buffer | |
| ): | |
| # given path crosses lane marking on the right | |
| return 3 | |
| # given path is okay and is not colliding | |
| return 0 | |
| # Class-level constants | |
| VEHICLE_WIDTH_BUFFER = 1.0 # meters | |
| LANE_CHECK_BUFFER = 0.5 # meters | |
| OBSTACLE_DETECTION_RADIUS = 10.0 # meters | |
| def check_trajectory(self, local_path: Path) -> int: | |
| """ | |
| Checks the calculated local path on multiple conditions | |
| Args: | |
| local_path (Path): The path to check | |
| Returns: | |
| int: Status code indicating the path's validity: | |
| -1: local_path is None or empty | |
| 0: Path has no obstacles/infringements | |
| 1: Path planned through gap too small for car | |
| 2: Path cuts left lane marking | |
| 3: Path cuts right lane marking | |
| Raises: | |
| ValueError: If local_path.poses is empty | |
| """ | |
| if local_path.poses is None or not local_path.poses: | |
| return -1 | |
| # transform local path to shapely object | |
| trajectory_shapely_points = [] | |
| for pos in local_path.poses: | |
| trajectory_shapely_points.append( | |
| shapely.Point(pos.pose.position.x, pos.pose.position.y) | |
| ) | |
| local_trajectory = shapely.LineString(trajectory_shapely_points) | |
| # widen the Linestring to an area representing the cars width | |
| local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3) | |
| # Check path collision with other entities | |
| obstacles = self.entities_in_area(self.OBSTACLE_DETECTION_RADIUS) | |
| for entry in obstacles: | |
| if (not entry.flags._is_lanemark and | |
| shapely.intersection(entry.to_shapely().poly, local_trajectory_buffer)): | |
| return 1 | |
| # decrease buffer size for lane checking | |
| local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3) | |
| # Check left lane | |
| left_lane = self.lane_marking_left() | |
| if left_lane is not None and shapely.intersection( | |
| left_lane.to_shapely().poly, local_trajectory_buffer): | |
| return 2 | |
| # Check right lane | |
| right_lane = self.lane_marking_right() | |
| if right_lane is not None and shapely.intersection( | |
| right_lane.to_shapely().poly, local_trajectory_buffer): | |
| return 3 | |
| return 0 |
🧰 Tools
🪛 Ruff (0.8.2)
318-321: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
327-330: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
💡 Codebase verification
Unit tests needed for trajectory validation functionality
The codebase lacks test coverage for the new trajectory validation features. New tests should be added to code/mapping/tests/mapping_common/test_general.py to verify:
check_trajectoryreturn values (-1, 0, 1, 2, 3)entities_in_areawith various scenarios- Lane marking detection logic
- Edge cases with invalid inputs
Follow the existing test patterns in the codebase, particularly the structure in test_entity.py for consistency.
🔗 Analysis chain
Add unit tests for the new functionality.
The new methods need comprehensive unit tests to verify:
- Edge cases in
entities_in_area(empty list, negative radius) - Lane marking detection for various scenarios
- All possible return cases of
check_trajectory
Let me help you verify the test coverage:
Would you like me to help generate comprehensive unit tests for these new methods?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test files
echo "Searching for test files..."
fd "test.*\.py$"
# Look for test cases related to the new functionality
echo -e "\nSearching for related test cases..."
rg -l "test.*(?:entities_in_area|lane_marking|check_trajectory)" --type py
Length of output: 601
Script:
#!/bin/bash
# Check content of test files in mapping_common for related tests
echo "=== Checking test_entity.py ==="
cat code/mapping/tests/mapping_common/test_entity.py
echo -e "\n=== Checking test_general.py ==="
cat code/mapping/tests/mapping_common/test_general.py
# Search for any trajectory or path related tests
echo -e "\n=== Searching for trajectory/path related tests ==="
rg "def test.*(?:trajectory|path)" --type py -A 5
Length of output: 3711
🧰 Tools
🪛 Ruff (0.8.2)
318-321: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
327-330: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/mapping/ext_modules/mapping_common/map.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (4)
code/mapping/ext_modules/mapping_common/map.py (4)
288-291: LGTM! Well-documented constants with clear units.The constants are well-named and documented with their respective units.
293-349: Add comprehensive test coverage.The
check_trajectorymethod lacks test coverage for its various return cases (-1, 0, 1, 2, 3). This is critical functionality that should be thoroughly tested.Would you like me to help generate comprehensive unit tests for this method? The tests should cover:
- Invalid input (None/empty path)
- Valid path with no obstacles
- Path with vehicle width conflicts
- Path crossing left/right lane markings
268-286: 🛠️ Refactor suggestionConsolidate duplicate logic and improve documentation.
The lane marking methods have duplicate logic that can be consolidated into a helper method.
- def lane_marking_left(self) -> Optional[Entity]: + def lane_marking_left(self) -> Optional[Entity]: + """Returns the left lane marking entity if it exists. + + Returns: + Optional[Entity]: The left lane marking entity or None if not found + """ + return self._get_lane_marking(position_index=1) + + def lane_marking_right(self) -> Optional[Entity]: + """Returns the right lane marking entity if it exists. + + Returns: + Optional[Entity]: The right lane marking entity or None if not found + """ + return self._get_lane_marking(position_index=-1) + + def _get_lane_marking(self, position_index: int) -> Optional[Entity]: + """Helper method to get lane marking by position index. + + Args: + position_index (int): 1 for left, -1 for right + + Returns: + Optional[Entity]: The lane marking entity or None if not found + """ for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == 1: - return entry - return None - - def lane_marking_right(self) -> Optional[Entity]: - for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == -1: + if entry.flags._is_lanemark and entry.position_index == position_index: return entry return NoneLikely invalid or redundant comment.
181-213: 🛠️ Refactor suggestionOptimize performance and improve robustness.
The method has several areas for improvement:
- Add validation for negative radius
- Remove code duplication in distance calculation
- Use numpy vectorization for better performance
- Improve documentation
- def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: + def entities_in_area(self, radius: float, only_in_front: bool = True) -> List[Entity]: + """Returns entities within a given radius around the hero car. + + Args: + radius (float): The radius to search for entities (must be positive) + only_in_front (bool): If True, only return entities in front of the car + + Returns: + List[Entity]: Filtered list of entities within the radius + + Raises: + ValueError: If radius is negative + """ + if radius < 0: + raise ValueError("Radius must be positive") + entities = self.entities_without_hero() - entities_in_area = [] - - if only_in_front: - for entry in entities: - distance = np.sqrt( - np.power(entry.transform.translation().x(), 2) - + np.power(entry.transform.translation().y(), 2) - ) - if entry.transform.translation().x() > 0 and distance < radius: - entities_in_area.append(entry) - return entities_in_area - else: - for entry in entities: - distance = np.sqrt( - np.power(entry.transform.translation().x(), 2) - + np.power(entry.transform.translation().y(), 2) - ) - if distance < radius: - entities_in_area.append(entry) - - return entities_in_area + # Vectorized distance calculation + positions = np.array([[e.transform.translation().x(), e.transform.translation().y()] + for e in entities]) + distances = np.sqrt(np.sum(positions**2, axis=1)) + + if only_in_front: + mask = (positions[:, 0] > 0) & (distances < radius) + else: + mask = distances < radius + + return [e for i, e in enumerate(entities) if mask[i]]Likely invalid or redundant comment.
| def check_trajectory(self, local_path: Path) -> int: | ||
| """ | ||
| Checks the calculated local path on multiple conditions | ||
|
|
||
| Args: | ||
| local_path (Path): The path to check | ||
|
|
||
| Returns: | ||
| int: Status code indicating the path's validity: | ||
| -1: local_path is None or empty | ||
| 0: Path has no obstacles/infringements | ||
| 1: Path planned through gap too small for car | ||
| 2: Path cuts left lane marking | ||
| 3: Path cuts right lane marking | ||
|
|
||
| Raises: | ||
| ValueError: If local_path.poses is empty | ||
| """ | ||
| if local_path.poses is None or not local_path.poses: | ||
| return -1 | ||
|
|
||
| # transform local path to shapely object | ||
| trajectory_shapely_points = [] | ||
| for pos in local_path.poses: | ||
| trajectory_shapely_points.append( | ||
| shapely.Point(pos.pose.position.x, pos.pose.position.y) | ||
| ) | ||
| local_trajectory = shapely.LineString(trajectory_shapely_points) | ||
| # widen the Linestring to an area representing the cars width | ||
| local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3) | ||
|
|
||
| # Check path collision with other entities | ||
| obstacles = self.entities_in_area(self.OBSTACLE_DETECTION_RADIUS) | ||
| for entry in obstacles: | ||
| if not entry.flags._is_lanemark and shapely.intersection( | ||
| entry.to_shapely().poly, local_trajectory_buffer | ||
| ): | ||
| return 1 | ||
|
|
||
| # decrease buffer size for lane checking | ||
| local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3) | ||
|
|
||
| # Check left lane | ||
| left_lane = self.lane_marking_left() | ||
| if left_lane is not None and shapely.intersection( | ||
| left_lane.to_shapely().poly, local_trajectory_buffer | ||
| ): | ||
| return 2 | ||
|
|
||
| # Check right lane | ||
| right_lane = self.lane_marking_right() | ||
| if right_lane is not None and shapely.intersection( | ||
| right_lane.to_shapely().poly, local_trajectory_buffer | ||
| ): | ||
| return 3 | ||
|
|
||
| return 0 |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Verification confirms missing test coverage for critical trajectory validation.
The codebase lacks unit tests for the check_trajectory method, which is responsible for crucial safety checks including:
- Path validation
- Obstacle collision detection
- Lane marking violation checks
The magic number '3' in buffer calls is also confirmed to be hardcoded without explanation.
🔗 Analysis chain
Improve robustness and maintainability of trajectory checking.
The method has several areas for improvement:
- Extract magic number '3' in buffer calls into a constant
- Simplify nested conditions
- Add comprehensive unit tests
+ # Additional class constant
+ BUFFER_QUADRANT_SEGMENTS = 3 # Number of segments per quadrant for buffer operations
+
def check_trajectory(self, local_path: Path) -> int:
# ... existing docstring ...
local_trajectory = shapely.LineString(trajectory_shapely_points)
- local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3)
+ local_trajectory_buffer = local_trajectory.buffer(
+ self.VEHICLE_WIDTH_BUFFER,
+ self.BUFFER_QUADRANT_SEGMENTS
+ )
# ... rest of the method ...
- local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3)
+ local_trajectory_buffer = local_trajectory.buffer(
+ self.LANE_CHECK_BUFFER,
+ self.BUFFER_QUADRANT_SEGMENTS
+ )Let me verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test cases
echo "=== Searching for trajectory tests ==="
rg "def test.*trajectory" --type py
# Check content of test files
echo -e "\n=== Checking test files ==="
fd "test_.*\.py$" -x cat {}
Length of output: 18773
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
code/mapping/src/path_test.py (3)
14-20: Update class documentation to match implementation.The current documentation focuses on frame translation, but the class primarily handles trajectory generation and checking. Consider updating the documentation to accurately reflect the class's main responsibilities.
31-36: Consider making the map topic more configurable.The hardcoded default topic path
/paf/hero/mapping/init_datamight limit reusability. Consider moving this to a configuration file or making it more generic.
74-78: Maintain consistency in node naming.The node name in
__main__("path_test_node") differs from the class name ("TestPath"). Consider maintaining consistency to avoid confusion.if __name__ == "__main__": - name = "path_test_node" + name = "test_path_node" # Align with class name TestPath roscomp.init(name) node = TestPath() node.spin()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/launch/mapping.launch(1 hunks)code/mapping/src/path_test.py(1 hunks)
🔇 Additional comments (1)
code/mapping/src/path_test.py (1)
67-67: Add input validation for map data.Consider adding validation for incoming map data to ensure robustness:
- Check for null/empty map data
- Validate map boundaries and coordinates
- Verify essential map features are present
code/mapping/launch/mapping.launch
Outdated
| <node pkg="mapping" type="path_test.py" name="Local_Trajectory_Node" output="screen" /> | ||
| <param name="map_publish_rate" value="$(arg map_publish_rate)" /> | ||
|
|
There was a problem hiding this comment.
Fix parameter indentation within node tags.
The map_publish_rate parameter should be nested within the node tags to properly associate it with the Local_Trajectory_Node.
Apply this diff to fix the indentation:
<node pkg="mapping" type="path_test.py" name="Local_Trajectory_Node" output="screen" />
- <param name="map_publish_rate" value="$(arg map_publish_rate)" />
+ <node pkg="mapping" type="path_test.py" name="Local_Trajectory_Node" output="screen">
+ <param name="map_publish_rate" value="$(arg map_publish_rate)" />
+ </node>Committable suggestion skipped: line range outside the PR's diff.
…-check-function-for-planned-local-trajectory
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
code/mapping/ext_modules/mapping_common/map.py (1)
New methods require test coverage
The codebase lacks dedicated test coverage for the recently added methods. While there are existing test files and patterns, they don't cover:
entities_in_area()method (edge cases, radius checks)lane_marking_left()andlane_marking_right()methodscheck_trajectory()method's various return casesRecommend adding tests in
code/mapping/tests/mapping_common/test_general.pyfollowing existing patterns.🔗 Analysis chain
Line range hint
289-457: Add unit tests for new functionality.The new methods need comprehensive unit tests to verify:
- Edge cases in
entities_in_area(empty list, negative radius)- Lane marking detection for various scenarios
- All possible return cases of
check_trajectoryLet me help verify the test coverage:
Would you like me to help generate comprehensive unit tests for these new methods?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing test files echo "Searching for test files..." fd "test.*\.py$" code/mapping/tests/ # Look for test cases related to the new functionality echo -e "\nSearching for related test cases..." rg -l "test.*(?:entities_in_area|lane_marking|check_trajectory)" --type pyLength of output: 516
Script:
#!/bin/bash # Check content of test files for any related tests or patterns echo "=== Checking test_entity.py ===" cat code/mapping/tests/mapping_common/test_entity.py echo -e "\n=== Checking test_general.py ===" cat code/mapping/tests/mapping_common/test_general.py # Search for Map class tests specifically echo -e "\n=== Searching for Map class tests ===" rg "class.*Test.*Map" --type py # Look for any usage of the new methods in tests echo -e "\n=== Searching for usage of new methods ===" rg "entities_in_area|lane_marking_left|lane_marking_right|check_trajectory" --type pyLength of output: 4491
🧹 Nitpick comments (2)
code/mapping/ext_modules/mapping_common/map.py (2)
6-6: Remove unused import.The
shapely.plottingimport is not used in the code.-import shapely.plotting
401-457: Improve robustness and maintainability of trajectory checking.The method has several areas for improvement:
- Extract magic number 3 in buffer calls
- Add validation for buffer parameters
- Consider breaking down into helper methods
Apply this diff to improve the implementation:
+ # Additional class constant + BUFFER_QUADRANT_SEGMENTS = 3 # Number of segments per quadrant for buffer operations + + def _check_obstacle_collision(self, trajectory_buffer: Polygon) -> bool: + """Check if trajectory collides with any obstacles. + + Args: + trajectory_buffer (Polygon): The buffered trajectory + + Returns: + bool: True if collision detected, False otherwise + """ + obstacles = self.entities_in_area(self.OBSTACLE_DETECTION_RADIUS) + return any( + not entry.flags._is_lanemark and shapely.intersection( + entry.to_shapely().poly, trajectory_buffer + ) + for entry in obstacles + ) + + def _check_lane_violation(self, trajectory_buffer: Polygon) -> Optional[int]: + """Check if trajectory violates lane markings. + + Args: + trajectory_buffer (Polygon): The buffered trajectory + + Returns: + Optional[int]: 2 for left lane violation, 3 for right lane violation, + None if no violation + """ + left_lane = self.lane_marking_left() + if left_lane is not None and shapely.intersection( + left_lane.to_shapely().poly, trajectory_buffer + ): + return 2 + + right_lane = self.lane_marking_right() + if right_lane is not None and shapely.intersection( + right_lane.to_shapely().poly, trajectory_buffer + ): + return 3 + return None + def check_trajectory(self, local_path: Path) -> int: if local_path.poses is None or not local_path.poses: return -1 trajectory_shapely_points = [] for pos in local_path.poses: trajectory_shapely_points.append( shapely.Point(pos.pose.position.x, pos.pose.position.y) ) local_trajectory = shapely.LineString(trajectory_shapely_points) - local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3) + local_trajectory_buffer = local_trajectory.buffer( + self.VEHICLE_WIDTH_BUFFER, + self.BUFFER_QUADRANT_SEGMENTS + ) - # Check path collision with other entities - obstacles = self.entities_in_area(self.OBSTACLE_DETECTION_RADIUS) - for entry in obstacles: - if not entry.flags._is_lanemark and shapely.intersection( - entry.to_shapely().poly, local_trajectory_buffer - ): - return 1 + # Check for obstacle collisions + if self._check_obstacle_collision(local_trajectory_buffer): + return 1 # decrease buffer size for lane checking - local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3) + local_trajectory_buffer = local_trajectory.buffer( + self.LANE_CHECK_BUFFER, + self.BUFFER_QUADRANT_SEGMENTS + ) - # Check left lane - left_lane = self.lane_marking_left() - if left_lane is not None and shapely.intersection( - left_lane.to_shapely().poly, local_trajectory_buffer - ): - return 2 - - # Check right lane - right_lane = self.lane_marking_right() - if right_lane is not None and shapely.intersection( - right_lane.to_shapely().poly, local_trajectory_buffer - ): - return 3 + # Check for lane violations + lane_violation = self._check_lane_violation(local_trajectory_buffer) + if lane_violation is not None: + return lane_violation return 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/mapping/ext_modules/mapping_common/map.py(4 hunks)code/mapping/launch/mapping.launch(1 hunks)code/mapping/src/mapping_data_integration.py(2 hunks)code/mapping/src/path_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- code/mapping/launch/mapping.launch
- code/mapping/src/mapping_data_integration.py
- code/mapping/src/path_test.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (3)
code/mapping/ext_modules/mapping_common/map.py (3)
396-399: LGTM! Well-defined constants with clear documentation.The constants are appropriately named and have reasonable values for their intended use.
289-321: 🛠️ Refactor suggestionOptimize performance and improve code quality.
The method has several areas for improvement:
- Code duplication in distance calculation
- Inefficient iteration and calculation
- Missing type hints
- Docstring formatting issues
Apply this diff to improve the implementation:
- def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: + def entities_in_area(self, radius: float, only_in_front: bool = True) -> List[Entity]: + """Returns entities within a given radius around the hero car. + + Args: + radius (float): The radius to search for entities + only_in_front (bool): If True, only return entities in front of the car + + Returns: + List[Entity]: Filtered list of entities within the radius + """ entities = self.entities_without_hero() - entities_in_area = [] - - if only_in_front: - for entry in entities: - distance = np.sqrt( - np.power(entry.transform.translation().x(), 2) - + np.power(entry.transform.translation().y(), 2) - ) - if entry.transform.translation().x() > 0 and distance < radius: - entities_in_area.append(entry) - return entities_in_area - else: - for entry in entities: - distance = np.sqrt( - np.power(entry.transform.translation().x(), 2) - + np.power(entry.transform.translation().y(), 2) - ) - if distance < radius: - entities_in_area.append(entry) - - return entities_in_area + # Vectorized calculation using numpy + positions = np.array([[e.transform.translation().x(), e.transform.translation().y()] + for e in entities]) + distances = np.sqrt(np.sum(positions**2, axis=1)) + + if only_in_front: + mask = (positions[:, 0] > 0) & (distances < radius) + else: + mask = distances < radius + + return [e for i, e in enumerate(entities) if mask[i]]Likely invalid or redundant comment.
376-394: 🛠️ Refactor suggestionConsolidate duplicate logic and improve documentation.
The lane marking methods have duplicate logic and incomplete documentation.
Apply this diff to improve the implementation:
+ def _get_lane_marking(self, position_index: int) -> Optional[Entity]: + """Helper method to get lane marking by position index. + + Args: + position_index (int): 1 for left, -1 for right + + Returns: + Optional[Entity]: The lane marking entity or None if not found + """ + for entry in self.entities: + if entry.flags._is_lanemark and entry.position_index == position_index: + return entry + return None + def lane_marking_left(self) -> Optional[Entity]: """Returns the left lane marking entity if it exists. + + Returns: + Optional[Entity]: The left lane marking entity or None if not found + """ - for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == 1: - return entry - return None + return self._get_lane_marking(position_index=1) def lane_marking_right(self) -> Optional[Entity]: """Returns the right lane marking entity if it exists. + + Returns: + Optional[Entity]: The right lane marking entity or None if not found + """ - for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == -1: - return entry - return None + return self._get_lane_marking(position_index=-1)Likely invalid or redundant comment.
…-check-function-for-planned-local-trajectory
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/mapping/ext_modules/mapping_common/map.py (1)
6-6: Remove unused import.The
shapely.plottingimport is not used in the code.-import shapely.plottingAlso applies to: 18-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/mapping/ext_modules/mapping_common/map.py(4 hunks)code/mapping/launch/mapping.launch(1 hunks)code/mapping/src/mapping_data_integration.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/mapping/launch/mapping.launch
- code/mapping/src/mapping_data_integration.py
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/ext_modules/mapping_common/map.py
123-123: Undefined name points
(F821)
125-125: Undefined name width
(F821)
129-129: Undefined name points
(F821)
132-132: Undefined name width
(F821)
150-150: Undefined name direction
(F821)
152-152: Undefined name direction
(F821)
154-154: Undefined name direction
(F821)
155-155: Local variable y_axis_line is assigned to but never used
Remove assignment to unused variable y_axis_line
(F841)
157-157: Undefined name entities
(F821)
162-162: Undefined name radius
(F821)
163-163: Undefined name entities_in_area
(F821)
165-165: Undefined name entities_in_area
(F821)
167-167: Redefinition of unused entities_in_area from line 113
(F811)
🔇 Additional comments (2)
code/mapping/ext_modules/mapping_common/map.py (2)
254-272: Refactor lane marking methods to reduce duplication.The lane marking methods share similar logic and can be consolidated into a helper method.
def lane_marking_left(self) -> Optional[Entity]: """Returns the left lane marking entity if it exists. Returns: Optional[Entity]: The left lane marking entity or None if not found """ - for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == 1: - return entry - return None + return self._get_lane_marking(position_index=1) def lane_marking_right(self) -> Optional[Entity]: """Returns the right lane marking entity if it exists. Returns: Optional[Entity]: The right lane marking entity or None if not found """ - for entry in self.entities: - if entry.flags._is_lanemark is True and entry.position_index == -1: - return entry - return None + return self._get_lane_marking(position_index=-1) + + def _get_lane_marking(self, position_index: int) -> Optional[Entity]: + """Helper method to get lane marking by position index. + + Args: + position_index (int): 1 for left, -1 for right + + Returns: + Optional[Entity]: The lane marking entity or None if not found + """ + for entry in self.entities: + if entry.flags._is_lanemark and entry.position_index == position_index: + return entry + return None
274-335: Extract magic number into a constant.The magic number '3' in buffer calls should be extracted into a constant for better maintainability.
# Class-level constants VEHICLE_WIDTH_BUFFER = 1.0 # meters LANE_CHECK_BUFFER = 0.5 # meters OBSTACLE_DETECTION_RADIUS = 10.0 # meters + BUFFER_QUADRANT_SEGMENTS = 3 # Number of segments per quadrant for buffer operations def check_trajectory(self, local_path: Path) -> int: # ... existing docstring ... local_trajectory = shapely.LineString(trajectory_shapely_points) - local_trajectory_buffer = local_trajectory.buffer(self.VEHICLE_WIDTH_BUFFER, 3) + local_trajectory_buffer = local_trajectory.buffer( + self.VEHICLE_WIDTH_BUFFER, + self.BUFFER_QUADRANT_SEGMENTS + ) # ... rest of the method ... - local_trajectory_buffer = local_trajectory.buffer(self.LANE_CHECK_BUFFER, 3) + local_trajectory_buffer = local_trajectory.buffer( + self.LANE_CHECK_BUFFER, + self.BUFFER_QUADRANT_SEGMENTS + )
| def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | ||
| """Returns the entities without the hero car | ||
| in a given radius around the hero car. | ||
| Flag only_in_front decides if entities | ||
| behind the car, are returend too. | ||
| Only checks if the first entity is_hero | ||
|
|
||
| Returns: | ||
| - Polygon: A Shapely Polygon representing the widened curve.""" | ||
|
|
||
| if len(points) < 2: | ||
| raise ValueError("At least two points are required to define a curve.") | ||
| if width <= 0: | ||
| raise ValueError("Width must be a positive value.") | ||
|
|
||
| # Create a LineString from the given points | ||
| curve = LineString(points) | ||
|
|
||
| # Create a buffer around the curve to form a polygon with the given width | ||
| polygon = curve.buffer(width / 2, cap_style="round", join_style="round") | ||
|
|
||
| return polygon | ||
|
|
There was a problem hiding this comment.
Remove incorrect method definition.
This appears to be an incorrect code fragment with undefined variables (points, width) and an incorrect docstring. The correct implementation is defined below at line 167.
- def entities_in_area(self, radius, only_in_front=True) -> List[Entity]:
- """Returns the entities without the hero car
- in a given radius around the hero car.
- Flag only_in_front decides if entities
- behind the car, are returend too.
- Only checks if the first entity is_hero
-
- Returns:
- - Polygon: A Shapely Polygon representing the widened curve."""
-
- if len(points) < 2:
- raise ValueError("At least two points are required to define a curve.")
- if width <= 0:
- raise ValueError("Width must be a positive value.")
-
- # Create a LineString from the given points
- curve = LineString(points)
-
- # Create a buffer around the curve to form a polygon with the given width
- polygon = curve.buffer(width / 2, cap_style="round", join_style="round")
-
- return polygon📝 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 entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | |
| """Returns the entities without the hero car | |
| in a given radius around the hero car. | |
| Flag only_in_front decides if entities | |
| behind the car, are returend too. | |
| Only checks if the first entity is_hero | |
| Returns: | |
| - Polygon: A Shapely Polygon representing the widened curve.""" | |
| if len(points) < 2: | |
| raise ValueError("At least two points are required to define a curve.") | |
| if width <= 0: | |
| raise ValueError("Width must be a positive value.") | |
| # Create a LineString from the given points | |
| curve = LineString(points) | |
| # Create a buffer around the curve to form a polygon with the given width | |
| polygon = curve.buffer(width / 2, cap_style="round", join_style="round") | |
| return polygon | |
| # (The incorrect definition of `entities_in_area` has been removed.) |
🧰 Tools
🪛 Ruff (0.8.2)
123-123: Undefined name points
(F821)
125-125: Undefined name width
(F821)
129-129: Undefined name points
(F821)
132-132: Undefined name width
(F821)
| # Class-level constants | ||
| VEHICLE_WIDTH_BUFFER = 1.0 # meters | ||
| LANE_CHECK_BUFFER = 0.5 # meters | ||
| OBSTACLE_DETECTION_RADIUS = 10.0 # meters |
There was a problem hiding this comment.
These should be input parameters to the function
| def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | ||
| """Returns the entities without the hero car | ||
| in a given radius around the hero car. | ||
| Flag only_in_front decides if entities | ||
| behind the car, are returend too. | ||
| Only checks if the first entity is_hero | ||
|
|
||
| Returns: | ||
| List[Entity]: Entities in front and/or back of hero | ||
| without the hero car in given radius | ||
| """ | ||
| entities = self.entities_without_hero() | ||
| entities_in_area = [] | ||
|
|
||
| if only_in_front: | ||
| for entry in entities: | ||
| distance = np.sqrt( | ||
| np.power(entry.transform.translation().x(), 2) | ||
| + np.power(entry.transform.translation().y(), 2) | ||
| ) | ||
| if entry.transform.translation().x() > 0 and distance < radius: | ||
| entities_in_area.append(entry) | ||
| return entities_in_area | ||
| else: | ||
| for entry in entities: | ||
| distance = np.sqrt( | ||
| np.power(entry.transform.translation().x(), 2) | ||
| + np.power(entry.transform.translation().y(), 2) | ||
| ) | ||
| if distance < radius: | ||
| entities_in_area.append(entry) | ||
|
|
||
| return entities_in_area |
There was a problem hiding this comment.
Several remarks:
- Should be moved into the MapTree class
- This function seems to not take into account the shapes of the entities. It is also a duplicate of the same name function above. It should use the Circle polygon generated there and then filter these entities with MapTree.get_overlapping_entities()
| def entities_in_area(self, radius, only_in_front=True) -> List[Entity]: | ||
| """Returns the entities without the hero car | ||
| in a given radius around the hero car. | ||
| Flag only_in_front decides if entities | ||
| behind the car, are returend too. | ||
| Only checks if the first entity is_hero | ||
|
|
||
| Returns: | ||
| - Polygon: A Shapely Polygon representing the widened curve.""" | ||
|
|
||
| if len(points) < 2: | ||
| raise ValueError("At least two points are required to define a curve.") | ||
| if width <= 0: | ||
| raise ValueError("Width must be a positive value.") | ||
|
|
||
| # Create a LineString from the given points | ||
| curve = LineString(points) | ||
|
|
||
| # Create a buffer around the curve to form a polygon with the given width | ||
| polygon = curve.buffer(width / 2, cap_style="round", join_style="round") | ||
|
|
||
| return polygon |
There was a problem hiding this comment.
Duplicate (namewise) of a function below. I commented there for further steps
| def get_obstacle_on_trajectory( | ||
| self, trajectory, hero_pos, hero_heading, width=1.0 | ||
| ) -> Optional[Entity]: | ||
| """Calculates the closest entity on the given trajectory. Transforms | ||
| trajectory world coordinates into map coordinates based on hero position. | ||
|
|
||
| Args: | ||
| direction (str): lanemarks on "left", "right" or "both" will be checked. | ||
| Other inputs will be ignored | ||
|
|
||
| Returns: | ||
| dict{uuid, coordinate}: dictionary with uuid of lanemark as keys | ||
| and coordinates as according entries | ||
| """ | ||
| if direction == "left": | ||
| y_axis_line = LineString([[0, 0], [0, 8]]) | ||
| elif direction == "right": | ||
| y_axis_line = LineString([[0, 0], [0, -8]]) | ||
| elif direction == "both": | ||
| y_axis_line = LineString([[0, -8], [0, 8]]) | ||
| else: | ||
| for entry in entities: | ||
| distance = np.sqrt( | ||
| np.power(entry.transform.translation().x(), 2) | ||
| + np.power(entry.transform.translation().y(), 2) | ||
| ) | ||
| if distance < radius: | ||
| entities_in_area.append(entry) | ||
|
|
||
| return entities_in_area |
There was a problem hiding this comment.
This function does not seem to use the trajectory and hero data put into it?
Checking for obstacles is already done in the ACC and overtake Behaviors and they use different functions.
| def lane_marking_left(self) -> Optional[Entity]: | ||
| """ | ||
| Returns: | ||
| + Optional[Entity]: The left lane marking entity or None if not found | ||
| """ | ||
| for entry in self.entities: | ||
| if entry.flags._is_lanemark is True and entry.position_index == 1: | ||
| return entry | ||
| return None | ||
|
|
||
| def lane_marking_right(self) -> Optional[Entity]: | ||
| """ | ||
| Returns: | ||
| + Optional[Entity]: The right lane marking entity or None if not found | ||
| """ | ||
| for entry in self.entities: | ||
| if entry.flags._is_lanemark is True and entry.position_index == -1: | ||
| return entry | ||
| return None |
There was a problem hiding this comment.
Combining those functions into one with a flag for left/right would be nicer
There was a problem hiding this comment.
Since this is not a "main" node of the car, I would recommend putting this in a test subfolder inside the src directory
There was a problem hiding this comment.
The changes in this file seem unnecessary. (lidar_data is already set to None as part of the class definition)
|
Too much work to do for a feature that doesn't get used in our project. |
Description
Added mapping functions with the following functionality:
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
No
Most important changes
Only map.py is affected by this change.
All other files are changed temporarly for testing purpose only
Checklist:
Summary by CodeRabbit
Release Notes: Mapping System Update
New Features
Improvements
Testing
These updates significantly enhance the mapping system's ability to detect and analyze spatial relationships, improving overall navigation and path planning capabilities.