Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Planner bugfix #11

Merged
merged 5 commits into from
Nov 18, 2020
Merged

Planner bugfix #11

merged 5 commits into from
Nov 18, 2020

Conversation

cookiew
Copy link
Contributor

@cookiew cookiew commented Nov 1, 2020

bug fix to update planners
now the ego vehicle is able to track the given route

- remove uav_game planner node
- remove topic name convention from game-planner
carla uses left hand coordinate but ros uses right hand coordinate
need to flip signs of y coordinate given in the csv file
- takes in path from map_updater
- publishes a trajectory assuming constant reference speed
- set default to use .rviz file from carla_circle package
Copy link
Member

@exoticDFT exoticDFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work @cookiew!!

The code is working well on my local setup and the ego is tracking great!

I've made several specific comments for the committed code. Overall there isn't much to change, and a lot of it are questions for @simon-lc and @dfridovi as well. A couple of larger questions and comments here though.

Are the trivial planner and map updater doing similar things? I see the trivial planner is the replacement for the uav game codebase, but it seems like there might be a bit of overlap in the map updater as well?

Is the uav game code entirely removed at this point? This was the goal of issue #7. If so, that is awesome!

In general, I think it's a good idea to mention some of the issues you are handling in the commit messages. Anytime you use the # symbol followed by the issue number, GitHub will track it for us. That way everyone can follow exactly what issues the commits relate to as well. And more specifically, whenever the PR or commit finalizes an issue (i.e. it can be closed,) you can reference the issue by saying something like closes #7 in the commit message. Doing so will automatically track it in GitHub and close it for us. https://docs.github.com/en/free-pro-team@latest/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword. So long story short, we should update the message for the PR here to reference the issues and specifically mention resolving issue 7 if we have indeed closed that out. I'll update the README as well to no longer require uav game repo.

script/trivial_planner.py Outdated Show resolved Hide resolved
script/visualization_node.py Outdated Show resolved Hide resolved
script/map_updater.py Outdated Show resolved Hide resolved
script/map_updater.py Outdated Show resolved Hide resolved
script/map_updater.py Show resolved Hide resolved
script/map_updater.py Outdated Show resolved Hide resolved
script/map_updater.py Show resolved Hide resolved
script/trivial_planner.py Show resolved Hide resolved
launch/mcity_event1.launch Outdated Show resolved Hide resolved
launch/mcity_event1.launch Show resolved Hide resolved
@dfridovi
Copy link

dfridovi commented Nov 4, 2020

@exoticDFT Were there specific questions for me / @simon-lc?

@exoticDFT
Copy link
Member

exoticDFT commented Nov 11, 2020

@exoticDFT Were there specific questions for me / @simon-lc?

@dfridovi No specific questions per say, just a couple of design related questions I threw into my comments for this PR. I was just hoping everyone would have a chance to look over Mingyu's additions and comment on what they believe is good to go, versus needing some modifications.

- conversion between CARLA left-handed coordinates and ROS right-handed
coordinates. Do the conversion while reading in csv file and keep all
internal code to be right-handed coordinates.
- remove redundant re-definition of odom_state classes
- clean up and add documentation
@cookiew
Copy link
Contributor Author

cookiew commented Nov 14, 2020

Thank you for the comments! I've made the corresponding changes @exoticDFT

Copy link
Member

@exoticDFT exoticDFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, runs great. I think we should be able to merge now. We still want to add some documentation for the functions in trival_planner, but let's just make sure that happens the next time we touch that code.

@exoticDFT exoticDFT merged commit 1a107d5 into update_planners Nov 18, 2020
@cookiew cookiew deleted the planner_bugfix branch November 27, 2020 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants