680 class and velocity intermediate layer #684
Conversation
WalkthroughThe changes update multiple components including RViz configuration, mapping entities, filtering logic, shape representations, and visualization processing. RViz panels are restructured with new marker entries and updated view parameters. In the mapping module, the Changes
Sequence Diagram(s)sequenceDiagram
participant S as Sensor Data
participant MDI as MappingDataIntegrationNode
participant E as Entity (Car/Pedestrian)
participant V as Visualization
S->>MDI: Send clustered points with object_class
MDI->>MDI: Determine entity type (Car, Pedestrian, or generic Entity)
MDI->>E: Instantiate entity and compute velocity
E->>V: Generate marker via to_marker & get_meta_markers
V->>V: Consolidate markers based on show_meta_markers flag
V->>Display: Publish marker array
Assessment against linked issues
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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: 0
🧹 Nitpick comments (9)
code/mapping/src/mapping_data_integration.py (1)
331-366: Use enums or constants for object_class values and reduce duplication.
Hardcoding class identifiers (4 for Pedestrian, 10 for Car) makes the code harder to maintain. Consider using an enum-like structure for readability and future extensibility. Additionally, creating each entity type repeats many lines of configuration (e.g.,shape,transform,timestamp, etc.). Refactoring into a small helper method or factory could reduce duplication.code/mapping/launch/mapping.launch (1)
24-25: Ensure debug settings are not deployed to production.
Switching the node todebug_wrapper.pyis helpful for local debugging. However, confirm that these debugging arguments (e.g.,--debug_port) are disabled or removed in production environments to avoid exposing debug ports.code/mapping_visualization/src/visualization.py (3)
12-12: Remove unusedrospyimport.
Static analysis indicates thatrospyis never referenced. Deleting this unused import will keep the code clean.- import rospy🧰 Tools
🪛 Ruff (0.8.2)
12-12:
rospyimported but unusedRemove unused import:
rospy(F401)
14-14: Remove unusedCarandPedestrianimports.
Currently, you don’t appear to differentiate these specific classes in this file. If you only need the genericEntity, removing the extra imports will simplify the code.- from mapping_common.entity import Entity, Car, Pedestrian + from mapping_common.entity import Entity🧰 Tools
🪛 Ruff (0.8.2)
14-14:
mapping_common.entity.Entityimported but unusedRemove unused import
(F401)
14-14:
mapping_common.entity.Carimported but unusedRemove unused import
(F401)
14-14:
mapping_common.entity.Pedestrianimported but unusedRemove unused import
(F401)
16-16: Remove unusedPoint2import.
Point2is not used in this file. Removing it reduces clutter.- from mapping_common.transform import Point2🧰 Tools
🪛 Ruff (0.8.2)
16-16:
mapping_common.transform.Point2imported but unusedRemove unused import:
mapping_common.transform.Point2(F401)
code/mapping/ext_modules/mapping_common/filter.py (1)
10-10: Consider removing unused imports if not needed for type checking.The imports
CarandPedestrianfrom.entityare currently unused in this file. If they're not needed for type checking, consider removing them.-from .entity import ShapelyEntity, Entity, FlagFilter, Car, Pedestrian +from .entity import ShapelyEntity, Entity, FlagFilter🧰 Tools
🪛 Ruff (0.8.2)
10-10:
.entity.Carimported but unusedRemove unused import
(F401)
10-10:
.entity.Pedestrianimported but unusedRemove unused import
(F401)
code/mapping/ext_modules/mapping_common/shape.py (1)
267-286: Consider extracting magic numbers to named constants.The marker configuration uses magic numbers for scale and dimensions. Consider extracting these to named constants for better maintainability.
+# At the top of the file +LINE_THICKNESS = 0.05 + def to_marker( self, transform: Optional[Transform2D] = None, marker_style: MarkerStyle = MarkerStyle.CLOSED_OBJECT, ) -> Marker: m = super().to_marker(transform) if marker_style == MarkerStyle.LINESTRING: m.type = Marker.LINE_STRIP - m.scale.x = 0.05 # Line thickness + m.scale.x = LINE_THICKNESScode/mapping/ext_modules/mapping_common/entity.py (2)
387-408: Consider adding validation for motion magnitude.The motion marker generation could benefit from validation of the motion vector magnitude to avoid creating arrows for negligible motion.
def to_motion_marker(self) -> Marker: assert self.motion is not None + # Skip arrows for negligible motion + if self.motion.linear_motion.length() < 0.1: # m/s + return None m = Marker() m.type = Marker.ARROW
553-559: Consider extracting color values to constants.The marker colors for Car and Pedestrian classes use magic numbers. Consider extracting these to named constants for better maintainability.
+# At the top of the file +CAR_COLOR = (0, 0, 255) # RGB +PEDESTRIAN_COLOR = (220, 20, 60) # RGB + class Car(Entity): def to_marker(self) -> Marker: m = super().to_marker() - # [0, 0, 255], # 10: Vehicles - m.color.r = 0 - m.color.g = 0 - m.color.b = 255 / 255 + m.color.r = CAR_COLOR[0] / 255 + m.color.g = CAR_COLOR[1] / 255 + m.color.b = CAR_COLOR[2] / 255Also applies to: 665-671
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
code/agent/config/rviz_config.rviz(6 hunks)code/mapping/ext_modules/mapping_common/entity.py(7 hunks)code/mapping/ext_modules/mapping_common/filter.py(2 hunks)code/mapping/ext_modules/mapping_common/shape.py(6 hunks)code/mapping/launch/mapping.launch(1 hunks)code/mapping/src/mapping_data_integration.py(3 hunks)code/mapping_visualization/src/visualization.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
code/mapping_visualization/src/visualization.py
12-12: rospy imported but unused
Remove unused import: rospy
(F401)
14-14: mapping_common.entity.Entity imported but unused
Remove unused import
(F401)
14-14: mapping_common.entity.Car imported but unused
Remove unused import
(F401)
14-14: mapping_common.entity.Pedestrian imported but unused
Remove unused import
(F401)
16-16: mapping_common.transform.Point2 imported but unused
Remove unused import: mapping_common.transform.Point2
(F401)
code/mapping/ext_modules/mapping_common/filter.py
10-10: .entity.Car imported but unused
Remove unused import
(F401)
10-10: .entity.Pedestrian imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and push image
🔇 Additional comments (10)
code/mapping/src/mapping_data_integration.py (3)
12-12: Imports look correct and complete.
All imported classes and functions (Entity,Flags,Car,Motion2D,Pedestrian) appear to be used throughout this file, so there’s no immediate concern about unused imports. Good job!
281-283: Verify array length consistency.
When de-referencingobjectclassarray[indexarray == label][0], ensure thatobjectclassarrayandindexarrayalways match in length and indexes. If there's a mismatch or empty slice, an index out-of-bounds error could occur.
327-328: Confirm the updated motion logic.
Adding the hero speed to the cluster’s motion vector may alter your object-tracking behavior. Verify that this correctly reflects your intended relative or absolute motion modeling.code/mapping_visualization/src/visualization.py (2)
39-39: Good use of a configurable toggle for meta markers.
Introducingself.show_meta_markersfacilitates flexible visualization. Nicely done!
60-75: Marker creation flow looks solid.
Using a single loop to invoketo_marker()and optional meta markers is straightforward and readable. Great job!code/agent/config/rviz_config.rviz (3)
7-8: LGTM! Display configuration updated for new visualization features.The display configuration has been updated to show intermediate visualization and ACC debug markers, which aligns with the new visualization capabilities.
109-110: Verify if disabling Lidar visualization is intentional.The Lidar visualization has been disabled. While this may reduce visual clutter, it could impact debugging capabilities.
Please confirm if disabling the Lidar visualization was intentional and won't impact debugging workflows.
Also applies to: 124-124
220-221: LGTM! Camera view settings optimized.The camera view has been changed to ThirdPersonFollower with adjusted pitch and yaw settings, which should provide better visualization of the scene.
Also applies to: 237-239
code/mapping/ext_modules/mapping_common/shape.py (1)
20-24: LGTM! Well-structured enum for marker styles.The MarkerStyle enum provides a clean abstraction for different marker visualization styles.
code/mapping/ext_modules/mapping_common/entity.py (1)
372-385: LGTM! Well-implemented meta marker generation.The get_meta_markers method provides a clean way to generate additional visualization elements, including motion indicators and speed information.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/mapping/ext_modules/mapping_common/filter.py (1)
49-49: Add type annotations to the filter method.For consistency with the base class and improved type safety, add type annotations to the method signature.
- def filter(self, map): + def filter(self, map: Map) -> Map:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/mapping/ext_modules/mapping_common/filter.py(1 hunks)code/mapping_visualization/src/visualization.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/mapping_visualization/src/visualization.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/filter.py (3)
220-225: LGTM! Code simplification without functional changes.The removal of explicit type annotations makes the code cleaner while maintaining the same functionality.
183-343: Consider adding unit tests for geometric edge cases.The helper functions handle complex geometry operations with proper error handling. To ensure robustness, consider adding unit tests for edge cases such as:
- Invalid polygons
- Single point polygons after growing/shrinking
- Counter-clockwise vs clockwise polygon orientations
Would you like me to help generate comprehensive unit tests for these edge cases?
229-239: Verify velocity handling in entity merging.The code handles merging of various entity attributes but doesn't explicitly handle velocity. Given that this PR adds velocity functionality, consider how velocity should be handled during entity merging.
Let's check if velocity handling is implemented elsewhere:
Description
Added class and velocity into intermediate layer inclusive visualization.
Fixes #680
Type of change
Please delete options that are not relevant.
Does this PR introduce a breaking change?
Nope
Most important changes
Visualization is working, merging is working for cars and non typed objects.
Checklist:
Summary by CodeRabbit
New Features
Refactor