feature-shape2d-polygon-class-for-intermediate-layer-and-cluster-integration #616
Conversation
…dar and lidar nodes and added entities to publishe map object.
|
Warning Rate limit exceeded@michalal7 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
code/mapping/ext_modules/mapping_common/shape.py (2)
195-195: Translate German comment to English.Replace the German comment "Sicherstellen, dass der Punkt korrekt ist" with its English equivalent "Ensure the point is valid".
187-209: Add color configuration for the marker visualization.The marker visualization is missing color configuration, which is important for distinguishing different types of polygons in RViz.
def to_marker(self, transform: Transform2D) -> Marker: m = super().to_marker(transform) m.type = Marker.LINE_STRIP m.scale.x = 0.05 # Line thickness + # Set default color (can be overridden by the visualization node) + m.color.r = 0.0 + m.color.g = 1.0 + m.color.b = 0.0 + m.color.a = 1.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/ext_modules/mapping_common/shape.py(2 hunks)code/mapping/src/mapping_data_integration.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping/src/mapping_data_integration.py
220-220: SyntaxError: Positional argument cannot follow keyword argument
code/mapping/ext_modules/mapping_common/shape.py
4-4: Redefinition of unused Optional from line 2
Remove definition: Optional
(F811)
⏰ 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/shape.py (2)
159-170: LGTM! Well-structured class implementation.The Polygon class is well-implemented with proper inheritance, documentation, and initialization.
211-211:⚠️ Potential issueRegister Polygon in supported shape classes.
The Polygon class needs to be registered in
_shape_supported_classesto be supported by thefrom_ros_msgfactory method.-_shape_supported_classes = [Rectangle, Circle] +_shape_supported_classes = [Rectangle, Circle, Polygon]Likely invalid or redundant comment.
code/mapping/src/mapping_data_integration.py (1)
34-35: LGTM! Well-typed entity list attributes.The new attributes are properly typed and clearly named.
Fixed case of cluster entities being None. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
code/mapping/tests/mapping_common/test_shape.py (1)
21-30: Add more test cases for polygon conversion.The current test only verifies basic conversion. Consider adding test cases for:
- Empty polygon
- Polygon with minimum points (3)
- Polygon with offset transformation
- Invalid polygon (less than 3 points)
def test_polygon_conversion_edge_cases(): # Test minimum points min_points = [Point2.new(x, y) for x, y in [(0,0), (1,0), (0,1)]] min_polygon = shape.Polygon(min_points) min_msg = min_polygon.to_ros_msg() min_conv = shape.Shape2D.from_ros_msg(min_msg) assert min_polygon == min_conv # Test with offset offset = Transform2D.new_translation(Vector2.new(1.0, 2.0)) offset_polygon = shape.Polygon(min_points, offset) offset_msg = offset_polygon.to_ros_msg() offset_conv = shape.Shape2D.from_ros_msg(offset_msg) assert offset_polygon == offset_conv # Test invalid polygon with pytest.raises(AssertionError): invalid_points = [Point2.new(x, y) for x, y in [(0,0), (1,0)]] invalid_polygon = shape.Polygon(invalid_points) invalid_msg = invalid_polygon.to_ros_msg() shape.Shape2D.from_ros_msg(invalid_msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
code/mapping/ext_modules/mapping_common/shape.py(2 hunks)code/mapping/src/mapping_data_integration.py(4 hunks)code/mapping/tests/mapping_common/test_shape.py(2 hunks)code/perception/src/lidar_distance.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/perception/src/lidar_distance.py
13-13: mapping.ext_modules.mapping_common.shape.Polygon imported but unused
Remove unused import: mapping.ext_modules.mapping_common.shape.Polygon
(F401)
416-416: Local variable shapely_polygon is assigned to but never used
Remove assignment to unused variable shapely_polygon
(F841)
code/mapping/tests/mapping_common/test_shape.py
3-3: numpy imported but unused
Remove unused import: numpy
(F401)
code/mapping/ext_modules/mapping_common/shape.py
4-4: shapely.geometry.Polygon imported but unused
Remove unused import: shapely.geometry.Polygon
(F401)
| def create_polygons_and_entities(label, bbox): | ||
| """ | ||
| Erstelle ein Shapely-Polygon, ein Shape2D-Polygon und ein Entity aus einer Bounding Box. | ||
|
|
||
| Args: | ||
| bbox (tuple): Bounding Box als (x_min, x_max, y_min, y_max). | ||
| entity_id (str): ID der zu erstellenden Entity. | ||
| frame_id (str): Referenzrahmen für die Pose. Default ist "map". | ||
|
|
||
| Returns: | ||
| tuple: (ShapelyPolygon, Shape2DPolygon, Entity) | ||
| """ | ||
| # Schritt 1: Shapely-Polygon aus Bounding Box | ||
| x_min, x_max, y_min, y_max = bbox | ||
| coordinates = [ | ||
| (x_min, y_min), | ||
| (x_max, y_min), | ||
| (x_max, y_max), | ||
| (x_min, y_max), | ||
| (x_min, y_min), # Schließe das Polygon | ||
| ] | ||
| shapely_polygon = ShapelyPolygon(coordinates) | ||
|
|
There was a problem hiding this comment.
Fix documentation and complete the implementation.
The function has several issues:
- Documentation:
- Docstring is in German
- Parameters don't match function signature (
entity_idandframe_idare documented but not used) - Return type hint is missing
- Implementation:
- Function is incomplete (only creates Shapely polygon)
- Unused imports:
Polygonfrommapping.ext_modules.mapping_common.shape - Unused variable:
shapely_polygon
Apply this diff to fix the issues:
-def create_polygons_and_entities(label, bbox):
+def create_polygons_and_entities(label: int, bbox: tuple) -> tuple[ShapelyPolygon, Polygon, Entity]:
"""
- Erstelle ein Shapely-Polygon, ein Shape2D-Polygon und ein Entity aus einer Bounding Box.
+ Create a Shapely polygon, a Shape2D polygon, and an Entity from a bounding box.
Args:
- bbox (tuple): Bounding Box als (x_min, x_max, y_min, y_max).
- entity_id (str): ID der zu erstellenden Entity.
- frame_id (str): Referenzrahmen für die Pose. Default ist "map".
+ label (int): Label for the entity.
+ bbox (tuple): Bounding box as (x_min, x_max, y_min, y_max).
Returns:
- tuple: (ShapelyPolygon, Shape2DPolygon, Entity)
+ tuple[ShapelyPolygon, Polygon, Entity]: A tuple containing:
+ - shapely_polygon: Shapely polygon for geometric operations
+ - shape2d_polygon: Shape2D polygon for visualization
+ - entity: Entity object for publishing
"""
- # Schritt 1: Shapely-Polygon aus Bounding Box
x_min, x_max, y_min, y_max = bbox
coordinates = [
(x_min, y_min),
(x_max, y_min),
(x_max, y_max),
(x_min, y_max),
- (x_min, y_min), # Schließe das Polygon
+ (x_min, y_min), # Close the polygon
]
shapely_polygon = ShapelyPolygon(coordinates)
+
+ # Create Shape2D polygon
+ points = [Point2.new(x, y) for x, y in coordinates[:-1]] # Exclude last point
+ shape2d_polygon = Polygon(points=points)
+
+ # Create entity
+ entity = Entity(
+ id=str(label),
+ pose=Transform2D.identity(),
+ shape=shape2d_polygon
+ )
+
+ return shapely_polygon, shape2d_polygon, entityCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
416-416: Local variable shapely_polygon is assigned to but never used
Remove assignment to unused variable shapely_polygon
(F841)
…ediate-layer-and-cluster-integration
…ediate-layer-and-cluster-integration
michalal7
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/mapping/ext_modules/mapping_common/shape.py (1)
181-188: Optimize to_ros_msg method using list comprehension.The method can be simplified using list comprehension for better readability.
def to_ros_msg(self) -> msg.Shape2D: m = super().to_ros_msg() - dimensions = [] - for p in self.points: - dimensions.append(p.x()) - dimensions.append(p.y()) - m.dimensions = dimensions + m.dimensions = [coord for p in self.points for coord in (p.x(), p.y())] return m
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/mapping/ext_modules/mapping_common/shape.py(2 hunks)code/mapping/src/mapping_data_integration.py(4 hunks)code/mapping/tests/mapping_common/test_shape.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/mapping/tests/mapping_common/test_shape.py
- code/mapping/src/mapping_data_integration.py
🔇 Additional comments (4)
code/mapping/ext_modules/mapping_common/shape.py (4)
3-3: Remove redundant import.The
Optionaltype is already imported on line 2.-from typing import List, Optional +from typing import List
215-215: LGTM!The Polygon class is correctly added to the supported shape classes list.
163-167:⚠️ Potential issueAdd input validation in constructor.
The constructor should validate:
- Points list is not None
- Points list has at least 3 points to form a valid polygon
def __init__(self, points: List[Point2], offset: Optional[Transform2D] = None): + if points is None: + raise ValueError("Points list cannot be None") + if len(points) < 3: + raise ValueError("Polygon requires at least 3 points") if offset is None: offset = Transform2D.identity() super().__init__(offset=offset) self.points = pointsLikely invalid or redundant comment.
156-212: 🛠️ Refactor suggestionAdd equality comparison method.
Implement
__eq__method to enable polygon comparison, which is essential for testing and validation.+ def __eq__(self, other: object) -> bool: + """Compare two polygons for equality. + + Args: + other: Object to compare with + + Returns: + bool: True if other is a Polygon with same points and offset + """ + if not isinstance(other, Polygon): + return NotImplemented + return (self.points == other.points and + self.offset == other.offset)Likely invalid or redundant comment.
…r-and-cluster-integration' of https://github.com/una-auxme/paf into 615-feature-shape2d-polygon-class-for-intermediate-layer-and-cluster-integration
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/mapping/ext_modules/mapping_common/shape.py (1)
158-158: Improve class documentation.The class documentation should be more descriptive and include important details about the polygon representation.
-"""Polygon defined by a list of Point2 objects.""" +"""Polygon defined by a list of Point2 objects. + +The polygon must have at least 3 points to form a valid shape. +Points are stored without redundant start/end point, with the polygon being closed +during visualization by connecting the last point to the first point. +"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/mapping/ext_modules/mapping_common/shape.py(2 hunks)
🔇 Additional comments (6)
code/mapping/ext_modules/mapping_common/shape.py (6)
3-3: Remove redundant import.The
Optionaltype is already imported on line 2.-from typing import List, Optional +from typing import List
Line range hint
215-219: LGTM!The registration of the Polygon class in the supported classes list and dictionary is implemented correctly.
170-170: 🛠️ Refactor suggestionFix incorrect return type hint.
The return type hint should be "Polygon" instead of "Shape2D" as this method specifically returns a Polygon instance.
-def _from_ros_msg(m: msg.Shape2D) -> "Shape2D": +def _from_ros_msg(m: msg.Shape2D) -> "Polygon":Likely invalid or redundant comment.
163-167:⚠️ Potential issueAdd validation for minimum number of points.
The polygon initialization should validate that at least 3 points are provided to form a valid polygon.
def __init__(self, points: List[Point2], offset: Optional[Transform2D] = None): + assert len(points) >= 3, "Polygon requires at least 3 points." if offset is None: offset = Transform2D.identity() super().__init__(offset=offset) self.points = pointsLikely invalid or redundant comment.
201-201:⚠️ Potential issueFix point transformation bug.
The point transformation is not being applied correctly. The transform parameter is being ignored.
- transformed_pt: Point2 = pt # Apply transformation + transformed_pt: Point2 = transform * pt # Apply transformationLikely invalid or redundant comment.
156-213: Verify polygon handling in the mapping system.Since this is a new shape type that will be used for entity visualization, we should verify its integration with the mapping system.
Let's check for proper usage of the Polygon class:
✅ Verification successful
Polygon integration verified successfully
The Polygon class is properly registered in the shape system and follows the established patterns. Test coverage confirms correct handling of:
- Shape registration
- ROS message conversion
- Point list management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Polygon class usage in the mapping system # Check for Polygon shape creation and handling rg -l "Polygon" --type py # Check for potential shape type handling in entity processing rg "type_name.lower\(\)" --type py -A 2Length of output: 819
Script:
#!/bin/bash # Check shape registration and test coverage # Find shape registration rg "_shape_supported_classes_dict\[" --type py -B 2 # Check test coverage for Polygon rg "def test.*polygon" --type py -i -A 2 # Look for shape type registration mechanism ast-grep --pattern 'def register_shape_type($$$): $$$'Length of output: 972
Script:
#!/bin/bash # Final verification of shape registration and test implementation # Check shape class list rg "_shape_supported_classes =" --type py -A 5 # Check polygon test implementation details cat code/mapping/tests/mapping_common/test_shape.pyLength of output: 1365
Co-authored-by: Zelberor <[email protected]>
Co-authored-by: Zelberor <[email protected]>
michalal7
deleted the
615-feature-shape2d-polygon-class-for-intermediate-layer-and-cluster-integration
branch
…dar and lidar nodes and added entities to publishe map object.
Description
Implemented Polygon shape. Implemented Subscribers and handling of entities in the intermediate layer.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
none.
Most important changes
Which files functionalities are most important in this PR. On which part should the reviewer be focussed on?
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Tests
These updates improve shape representation and data integration capabilities, enabling more flexible mapping and entity tracking across different sensor sources.