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

features, fixes and style changes #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dringakn
Copy link

I have add some new features, minor adjustments and improved code readability.

dringakn and others added 7 commits July 15, 2020 11:10
Fix the checking of costmap_clearing_threshold parameter if the frontier grid cell value is updated. The goal is cancelled. Minor timing adjustments are also added during infinite loops, in order to not hog the processor. Furthermore formatting is improved.
A small delay is added inside infinite while loop in order to not hog the processor. Furthermore, code is formatted.
…ition

The function is modified without changing it's name to also return the robot orientation, apart from the position.
When the user specify the exploration region using Rviz by clicking on the map, the clicked points are displayed on the terminal. Furthermore, code is formatted.
@hasauino
Copy link
Owner

hasauino commented Jul 15, 2020

Thank you very much for your changes and the time you put in it, I appreciate that a lot.

I still haven't checked all your changes, it is hard to track the code changes because they are mixed with formatting changes, when I check file changes, Git marks all the lines as changed, and now it's very hard to check which lines have actually been changed. That's not a problem though, but would appreciate in future PRs (I hope 😉 ), if you separate code changes from formatting...

I run your changes, and there are few problems:

  • local detector does not start, I published many points, it shows them on the terminal (which is a nice addition 😃 ), but it does not start...

  • I saw you made initializing the detector automatic, but now the exploration area is fixed to 20x20 (which might be unnecessarily large), also the tree now can start from anywhere, it's better if the tree only starts from free known space, because if the tree starts from an unknown point, then if this point happens to be in an unreachable space (surrounded by occupied cells in all directions, which happens!, example: if the point lies outside a closed room or house), then the tree will be useless!

  • In the robot class in the functions.py, you added yaw to the returned data by the get_position() function, but yaw is not used anywhere, so why is it important?

@dringakn
Copy link
Author

dringakn commented Jul 15, 2020

Thank you very much for your changes and the time you put in it, I appreciate that a lot.

I still haven't checked all your changes, it is hard to track the code changes because they are mixed with formatting changes, when I check file changes, Git marks all the lines as changed, and now it's very hard to check which lines have actually been changed. That's not a problem though, but would appreciate in future PRs (I hope wink ), if you separate code changes from formatting...

I run your changes, and there are few problems:

* local detector does not start, I published many points, it shows them on the terminal (which is a nice addition smiley ), but it does not start...

* I saw you made initializing the detector automatic, but now the exploration area is fixed to 20x20 (which might be unnecessarily  large), also the tree now can start from anywhere, it's better if the tree only starts from free known space, because if the tree starts from an unknown point, then if this point happens to be in an unreachable space (surrounded by occupied cells in all directions, which happens!,  example: if the point lies outside a closed room or house), then the tree will be useless!

* In the robot class in the `functions.py`, you added yaw to the returned data by the `get_position()` function, but yaw is not used anywhere, so why is it important?

I had made changes in my local repo. Since, I was waiting for the previous pull request to be finished and was not able to push further changes, therefore, today I have tried to break them as much as possible (:smile: ).

  1. It's working on my end, but, I shall look further for the local_detector.
  2. I was using it as a auto-start method, however, forget to disable it. If you want, you may remove/disable it.
  3. Actually, the idea is to consider the robot orientation during frontiers selection. We may create another function, such as getPose, for that purpose!

@hasauino
Copy link
Owner

Once the PR is ready, I will merge it 👍
Regarding the orientation, that's a great idea indeed, but to be honest, I left this work and was not planning to modify on it, especially that I don't have access to physical robots anymore.. I know there are plenty of room for improvements especially with my poor implementation 😄

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.

2 participants