Remove custom message dependencies#3
Conversation
|
Hi @ekampourakis, |
|
Hello @MaxMagazin , I'm posting this comment as a reminder for you to review the code changes and notify me about accepting or rejecting them. Thanks. |
| @@ -1,433 +0,0 @@ | |||
| """ | |||
There was a problem hiding this comment.
It looks like you wanted to move this file to src folder, but you commited the deleting of this file. Are you sure the package works as it is in your fork (https://github.com/ekampourakis/ma_rrt_path_plan)?
There was a problem hiding this comment.
It should complain in line 172 of MaRRTPathPlanNode.py, where
rrt = ma_rrt.RRT
There was a problem hiding this comment.
Yes you are absolutely right. It somehow got deleted. I removed the inc folder and added the file in the src folder directly because it was complaining. I fixed it in my fork.
|
|
||
| #### Inputs | ||
| - rospy.Subscriber("/map", Map, ...) | ||
| - rospy.Subscriber("/map", Track, ...) |
There was a problem hiding this comment.
Lets have it consistent:
rospy.Subscriber("/track", Track, ...)
|
|
||
| #### Parameters to tune (main) | ||
| - `odom_topic` = `/odometry` - The topic to get the odometry information from | ||
| - `world_frame` = `world` - The world frame to use for the visualizations |
There was a problem hiding this comment.
This parameters list was intended for algorithm parameters. I would leave this info out, or if you think it should be there in ReadMe, please move it to another section called Frames
|
|
||
| <!-- TODO: Comment me out --> | ||
| <!-- <param name="odom_topic" value="/odometry/real"/> | ||
| <param name="world_frame" value="map"/> --> |
There was a problem hiding this comment.
These three lines are confusing.
- For who is this TODO then if it is there
- Generally having the frames specified in launch files is cool, but the frames that you specified are unique to your system (again). For this purposes you could create a separate launch file, but I dont see a reason to have it this repo.
| rospy.Subscriber("/map", Map, self.mapCallback) | ||
| rospy.Subscriber("/odometry", Odometry, self.odometryCallback) | ||
| # rospy.Subscriber("/car_sensors", CarSensors, self.carSensorsCallback) | ||
| rospy.Subscriber("/track", Track, self.mapCallback) |
There was a problem hiding this comment.
Once it is all about track (instead of map), lets have a new callback name too (self.trackCallback)
|
|
||
| def mapCallback(self, map): | ||
| self.map = map | ||
| def mapCallback(self, track): |
| def mapCallback(self, map): | ||
| self.map = map | ||
| def mapCallback(self, track): | ||
| self.map = track.cones |
There was a problem hiding this comment.
Lets have self.track (instead self.map)
and initialize it with received track object (not with cones directly)
|
|
||
| frontConeList = [] | ||
| for cone in map.cones: | ||
| for cone in map: |
There was a problem hiding this comment.
based on my other comments here will be:
for cone in track.cones
note, additional renamings are needed
| coneList = [] | ||
| radiusSq = radius * radius | ||
| for cone in map.cones: | ||
| for cone in map: |
There was a problem hiding this comment.
see above:
for cone in track.cones
| # rospy.Subscriber("/car_sensors", CarSensors, self.carSensorsCallback) | ||
| rospy.Subscriber("/track", Track, self.mapCallback) | ||
| rospy.Subscriber(self.odometry_topic, Odometry, self.odometryCallback) | ||
| rospy.Subscriber("/dvsim/cmd", Command, self.carSensorsCallback) |
There was a problem hiding this comment.
I don't really use the steering angle of the car for path planning (this subscriber was even commented out, but I tried to use this information during my development).
But in your version it appears to be confusing: you subscribe to a topic (which generally means to get something), and you receive a command (which generally would be understood as control request).
Maybe message should be named a bit differently, what about CarState?
|
Hey @ekampourakis, I added a few comments about the changes. My main concern is that some your changes are still unique and specific to your system, which do not help to improve generalization of the package. Although, the messages you created are the nice contribution, thank you for that. |
|
@MaxMagazin Thanks for all the comments. I will look at them all and try to correct them but it will take some time as I'm working on many other projects as well. Don't close the PR just yet if you don't need to and I'll keep updating my fork until all your suggestions are implemented. |
2 participants
As mentioned in this issue:
Please review this PR and let me know.
Best regards,
Manos